Improve memory use and performance of multipart parsing #745
Conversation
Memory usage and performance noted in https://discourse.julialang.org/t/http-parse-multipart-form-does-a-lot-of-memory-allocations/66661. This uses some of the same regex tricks we use in the other http parsing code that operates on SubStrings and avoids excessive allocations. We also streamline things to avoid allocating when comparing byte buffers and the unnecessary allocated arrays of the content disposition processing. On my machine for current master, I see `35.022 μs (866 allocations: 70.27 KiB)` for parsing a simple multipart request from the tests, and with this PR, I see `3.603 μs (42 allocations: 2.55 KiB)`.
|
|
||
| """ | ||
| find_multipart_boundary(bytes, boundaryDelimiter; start::Int=1) | ||
|
|
||
| Find the first and last index of the next boundary delimiting a part, and if | ||
| the discovered boundary is the terminating boundary. | ||
| """ | ||
| function find_multipart_boundary(bytes::AbstractVector{UInt8}, boundaryDelimiter::AbstractVector{UInt8}; start::Int=1) | ||
| @inline function find_multipart_boundary(bytes::AbstractVector{UInt8}, boundaryDelimiter::AbstractVector{UInt8}; start::Int=1) |
There was a problem hiding this comment.
I'm kind of amazed that inlining this function has any performance improvement.
There was a problem hiding this comment.
Oh, it doesn't. This was from an iteration where I was returning a SubString instead of indices and it could avoid the allocation if it was inlined.
There was a problem hiding this comment.
So should it have been removed?
|
I can confirm this significantly improves the original request that was being tested/reported on Discourse — it is much larger and has an even larger benefit. Before: julia> @benchmark HTTP.parse_multipart_form($r)
BenchmarkTools.Trial: 26 samples with 1 evaluation.
Range (min … max): 181.312 ms … 235.950 ms ┊ GC (min … max): 3.11% … 3.33%
Time (median): 195.296 ms ┊ GC (median): 3.16%
Time (mean ± σ): 197.687 ms ± 13.029 ms ┊ GC (mean ± σ): 3.20% ± 0.16%
Memory estimate: 584.19 MiB, allocs estimate: 4785830.After: julia> @benchmark HTTP.parse_multipart_form($r)
BenchmarkTools.Trial: 917 samples with 1 evaluation.
Range (min … max): 5.131 ms … 9.564 ms ┊ GC (min … max): 0.00% … 0.00%
Time (median): 5.309 ms ┊ GC (median): 0.00%
Time (mean ± σ): 5.447 ms ± 448.252 μs ┊ GC (mean ± σ): 0.00% ± 0.00%
Memory estimate: 2.03 KiB, allocs estimate: 32. |
src/parsemultipart.jl
Outdated
| const CR_BYTE = 0x0d # \r | ||
| const LF_BYTE = 0x0a # \n | ||
| const DASH_BYTE = 0x2d # - | ||
| const HTAB_BYTE = 0x09 # \t | ||
| const SPACE_BYTE = 0x20 | ||
| const RETURN_BYTES = [CR_BYTE, LF_BYTE] | ||
| const SEMICOLON_BYTE = UInt8(';') | ||
| const CRLFCRLF = [CR_BYTE, LF_BYTE, CR_BYTE, LF_BYTE] |
There was a problem hiding this comment.
Make into a tuple (allows the compiler to know it's length etc)? I don't think it is mutated anywhere?
|
Ok, I unexported |
Codecov Report
@@ Coverage Diff @@
## master #745 +/- ##
==========================================
- Coverage 76.89% 76.84% -0.05%
==========================================
Files 38 38
Lines 2445 2514 +69
==========================================
+ Hits 1880 1932 +52
- Misses 565 582 +17
Continue to review full report at Codecov.
|
quinnj
force-pushed
the
jq/multipart_parsing_perf
branch
from
4d8c65f
to
a52b672
Compare
Also clean up usages of startread, closeread, startwrite, closewrite to quality to the IOExtras module. Also fix the websocket tests that were hanging since echo.websocket.org doesn't exist anymore. I found pie socket that offers a free websocket testing server if you sign up and get an api key, so that's been added as a repository secret now. If anyone wants the api key to run the tests on their local machine, I'm happy to share.
| closeread(io) = nothing | ||
| if isdefined(Base, :startwrite) | ||
| "$_doc" | ||
| Base.startwrite(io) = nothing |
There was a problem hiding this comment.
Yeah, we'll need to make a plan around this since Base doesn't define a fallback for closewrite. Either we add closewrite(io) = nothing to Base and we can check if it's defined here, or it's also possible we don't need the fallback anymore. I left this as-is for now since it ensures non-breaking.
There was a problem hiding this comment.
Why extend the function at all? You can just keep using it as a separate function and qualify it (or the Base one).
There was a problem hiding this comment.
Because then that's breaking; we have all these code examples all over the package where we tell people to write streaming servers doing closewrite(stream).
There was a problem hiding this comment.
You can still export it. Downstreams users just must then decide if they want to use Base.closewrite or HTTP.closewrite.
There was a problem hiding this comment.
Personally, I would strongly prefer that we remove the type piracy and just make a breaking release of HTTP.jl.
(I don't think it's actually breaking. I think the burden is on the downstream users to make sure that they properly qualify any names that they use.
But if this is considering breaking, then I'd rather just tag the breaking release so that we can get rid of the type piracy.)
Memory usage and performance noted in
https://discourse.julialang.org/t/http-parse-multipart-form-does-a-lot-of-memory-allocations/66661.
This uses some of the same regex tricks we use in the other http parsing
code that operates on SubStrings and avoids excessive allocations. We
also streamline things to avoid allocating when comparing byte buffers
and the unnecessary allocated arrays of the content disposition
processing.
On my machine for current master, I see
35.022 μs (866 allocations: 70.27 KiB)for parsing a simple multipart request from the tests, andwith this PR, I see
3.603 μs (42 allocations: 2.55 KiB).