Allow passing pre-allocated buffer for response body #984
Conversation
|
Notes to myself before I fall asleep:
|
Codecov Report
@@ Coverage Diff @@
## master #984 +/- ##
==========================================
- Coverage 82.58% 82.40% -0.19%
==========================================
Files 36 36
Lines 3026 3029 +3
==========================================
- Hits 2499 2496 -3
- Misses 527 533 +6
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/Streams.jl
Outdated
| @@ -288,8 +288,7 @@ function Base.readbytes!(http::Stream, buf::IOBuffer, n=bytesavailable(http)) | |||
| buf.size += n | |||
| end | |||
|
|
|||
| function Base.read(http::Stream) | |||
| buf = PipeBuffer() | |||
| function Base.read(http::Stream, buf::IOBuffer=PipeBuffer()) | |||
There was a problem hiding this comment.
If we make buf a Base.GenericIOBuffer then we can pass views as well. Same applies fo Base.readbytes!(http::Stream, buf::IOBuffer)
|
This looks great! As per our discussion, I think it would be great if we also have a non-allocating path for GenericIOBuffers (for JuliaServices/CloudStore.jl#24) |
This is one piece in some efforts we're going to make to drastically
reduce the # of allocations required for making HTTP requests.
This PR allows the user to pass a pre-allocated `Vector{UInt8}`
via the `response_stream` keyword arg (previously not allowed),
which will then be used directly for writing the response body.
quinnj
force-pushed
the
jq-user-provided-response-body-buffer
branch
from
80ab666
to
e448d96
Compare
| # check if there's enough room in buf to write n bytes | ||
| bufcheck(buf, n) | ||
| data = buf.data | ||
| GC.@preserve data unsafe_read(http, pointer(data, buf.size + 1), n) |
There was a problem hiding this comment.
Sleepy drive by review alert
Should this be Base.ensureroom(buf, buf.ptr + n - 1) and pointer(data, buf.ptr) and similarly for bufcheck?
There was a problem hiding this comment.
Well, perhaps, but we're updating buf.size ourselves (and not buf.ptr), so buf.size is correct for this case. From what I can tell, buf.size tracks the last byte written to the IOBuffer, whereas buf.ptr tracks the next by to read from the IOBuffer. So if that's right, then I think tracking buf.size is correct here.
There was a problem hiding this comment.
AFAIK, writing to IOBuffer should advance both ptr and size:
julia> io = IOBuffer(UInt8[1, 2, 3, 4, 5, 6], write=true)
IOBuffer(data=UInt8[...], readable=false, writable=true, seekable=true, append=false, size=0, maxsize=Inf, ptr=1, mark=-1)
julia> write(io, "aaaa")
4
julia> io
IOBuffer(data=UInt8[...], readable=false, writable=true, seekable=true, append=false, size=4, maxsize=Inf, ptr=5, mark=-1)
julia> seekstart(io)
IOBuffer(data=UInt8[...], readable=false, writable=true, seekable=true, append=false, size=4, maxsize=Inf, ptr=1, mark=-1)
julia> write(io, "aa")
2
julia> io
IOBuffer(data=UInt8[...], readable=false, writable=true, seekable=true, append=false, size=4, maxsize=Inf, ptr=3, mark=-1)Note that seekstart will allow us reuse the parts of the buffer that were pre-allocated earlier. So I think we should be working with ptr here, as it says which part of buffer is free (>= ptr) and which is not (< ptr)
| else | ||
| res.body = read(buf_or_stream) | ||
| end | ||
| elseif (res.body isa IOBuffers || res.body isa Base.GenericIOBuffer) && buf_or_stream isa Stream |
There was a problem hiding this comment.
I think this could be simplified, since:
julia> IOBuffers <: Base.GenericIOBuffer
true
This is one piece in some efforts we're going to make to drastically reduce the # of allocations required for making HTTP requests.
This PR allows the user to pass a pre-allocated
Vector{UInt8}via theresponse_streamkeyword arg (previously not allowed), which will then be used directly for writing the response body.cc: @nickrobinson251, @Drvi
Haven't added tests/docs yet, but wanted to at least get this up in case it overlaps w/ @Drvi's allocation hunting.