Reduce slightly allocations when reading/writing HTTP messages #950
Conversation
| @@ -205,7 +205,7 @@ function parse_status_line!(bytes::AbstractString, response)::SubString{String} | |||
| if !exec(re, bytes) | |||
| throw(ParseError(:INVALID_STATUS_LINE, bytes)) | |||
| end | |||
| response.version = VersionNumber(group(1, re, bytes)) | |||
| response.version = HTTPVersion(group(1, re, bytes)) | |||
There was a problem hiding this comment.
here, and a few lines above, is where we were allocating as part of construction VersionNumber
| @@ -1,9 +1,98 @@ | |||
| module Strings | |||
|
|
|||
| export escapehtml, tocameldash, iso8859_1_to_utf8, ascii_lc_isequal | |||
| export HTTPVersion, escapehtml, tocameldash, iso8859_1_to_utf8, ascii_lc_isequal | |||
There was a problem hiding this comment.
this is in src/Strings.jl because it was the submodule that was imported in exactly the same places we need HTTPVersion
happy to move it somewhere else
| Base.:(==)(va::VersionNumber, vb::HTTPVersion) = va == VersionNumber(vb) | ||
| Base.:(==)(va::HTTPVersion, vb::VersionNumber) = VersionNumber(va) == vb | ||
|
|
||
| Base.isless(va::VersionNumber, vb::HTTPVersion) = isless(va, VersionNumber(vb)) |
There was a problem hiding this comment.
we either need to define these methods, or else overload getproperty such that r.version returns a VersionNumber and we do all comparisons with VersionNumbers. Either seems fine to me.
There was a problem hiding this comment.
This is not that much code, so this seems fine
Codecov Report
@@ Coverage Diff @@
## master #950 +/- ##
==========================================
- Coverage 79.85% 79.23% -0.63%
==========================================
Files 36 36
Lines 2969 3010 +41
==========================================
+ Hits 2371 2385 +14
- Misses 598 625 +27
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/Messages.jl
Outdated
|
|
||
| """ | ||
| writestartline(::IO, ::Message) | ||
|
|
||
| e.g. `"GET /path HTTP/1.1\\r\\n"` or `"HTTP/1.1 200 OK\\r\\n"` | ||
| """ | ||
| function writestartline(io::IO, r::Request) | ||
| write(io, "$(r.method) $(r.target) $(httpversion(r))\r\n") | ||
| write(io, r.method, " ", r.target, " ", r.version, "\r\n") |
There was a problem hiding this comment.
So instead of creating a big string and writing that, we iterate over the args and each is written to io?
There was a problem hiding this comment.
yeah, exactly, we save on allocating that big string before writing it out
https://docs.julialang.org/en/v1/manual/performance-tips/#Avoid-string-interpolation-for-I/O
and we overload Base.write for version so it also writes what we want straight to IO
so each saving 1 string allocation
|
tests pass locally. not sure if we want to resolve #951 and get a clean CI run before merging here? |
nickrobinson251
force-pushed
the
npr/allocs
branch
from
51e85ad
to
2db81ae
Compare
Co-authored-by: Jacob Quinn <[email protected]>
Co-authored-by: Jacob Quinn <[email protected]>
nickrobinson251
force-pushed
the
npr/allocs
branch
from
2db81ae
to
7e9b0fd
Compare
- ...before flushing to socket, to avoid task switching mid writing to the socket itself.
- unnecessary and was leading to occasional DNS Errors
stringany numbers before doing this, but doing this selectively still reduces allocationsVersionNumberevery time we parse a message (inparse_status_line!/parse_request_line!), as theVersionNumberconstructor relies on regex matching which allocates, instead we can define our own much simpler type with allocation-free parsingTogether this takes a simple GET request from 170 -> 155 allocations