rectify control of easy-handle callbacks#155
Conversation
Codecov Report
@@ Coverage Diff @@
## master #155 +/- ##
==========================================
+ Coverage 91.15% 91.46% +0.30%
==========================================
Files 5 5
Lines 475 445 -30
==========================================
- Hits 433 407 -26
+ Misses 42 38 -4
Continue to review full report at Codecov.
|
| n = min(size * count, length(buf)) | ||
| ccall(:memcpy, Ptr{Cvoid}, (Ptr{Cvoid}, Ptr{Cvoid}, Csize_t), data, buf, n) | ||
| deleteat!(buf, 1:n) | ||
| eof(easy.input) && return 0 |
There was a problem hiding this comment.
This can block, so we'll eventually want to add flow-control with something like:
n = UInt(bytesavailable(easy.input))
if n == 0
errormonitor(@async begin
eof(easy.input) ? #= notify curl of EOF =# : #= notify curl of more data =#
end)
return CURL_READFUNC_PAUSE
end
n = min(n, UInt(size * count))
unsafe_read(easy.input, data, n)
return nThere was a problem hiding this comment.
I mean, most of these implementations can block... that's one of the main drawbacks noted in the PR notes.
src/Curl/Easy.jl
Outdated
| buf = unsafe_wrap(Vector{Cchar}, data, size*count) | ||
| write(easy.output, buf) |
There was a problem hiding this comment.
| buf = unsafe_wrap(Vector{Cchar}, data, size*count) | |
| write(easy.output, buf) | |
| unsafe_write(easy.output, data, UInt(size*count)) |
| )::Cint | ||
| easy = unsafe_pointer_to_objref(easy_p)::Easy | ||
| put!(easy.progress, (dl_total, dl_now, ul_total, ul_now)) | ||
| easy.progress(dl_total, dl_now, ul_total, ul_now) |
There was a problem hiding this comment.
To avoid the callback, I'd suggest here doing:
easy.dl_now = dl_now
easy.dl_total = dl_total
easy.ul_total = ul_total
easy.ul_now = ul_now
notify(easy.progress::Event)
I think I've measured this before to be false: a generic function dispatch is expected to be much faster than a task switch, and not much slower than a non-generic function dispatch (though it may allocate slightly more memory and doesn't inline). |
src/Curl/Easy.jl
Outdated
| @@ -1,26 +1,29 @@ | |||
| mutable struct Easy | |||
| mutable struct Easy{Input<:IO, Output<:IO, Progress<:Function} | |||
There was a problem hiding this comment.
| mutable struct Easy{Input<:IO, Output<:IO, Progress<:Function} | |
| mutable struct Easy{Input<:IO, Output<:IO} |
There was a problem hiding this comment.
Actually, I suspect there's no performance advantage to giving any of these explicit types, and it could simply be mutable struct Easy as it was before
There was a problem hiding this comment.
Slight additional question: do we need to inform the user that, since we are using IO directly now instead of Channel from a pinned Task, that those objects must now be thread-safe?
src/Curl/Easy.jl
Outdated
| output :: Channel{Vector{UInt8}} | ||
| progress :: Channel{NTuple{4,Int}} | ||
| output :: Output | ||
| progress :: Progress |
There was a problem hiding this comment.
| progress :: Progress | |
| progress :: Any |
Also eliminate try/catch around `wait(easy.done)` since it can't throw.
f448eb0 to
2af290f
Compare
This PR changes the complex easy handle callback flow to a more straightforward version where we register actual callbacks (or IO objects to read from or write to) with the
Easyobjects, which are then directly used in the easy handle callbacks. The steps:There are a few issues with this though:
When an easy-handle callback blocks, it now blocks any progress on anything the multi-handle is working on, even for other downloads.
If the
Easytype isn't parametric then itsinput,outputandprogressfields are abstract, which may cause performance issues. We can fix this by makingEasyparametric, but that may lead to excess compilation and specialization.The first point wasn't an issue with the old, convoluted version of the callbacks because the easy handle callbacks were designed to never block. The second point wasn't an issue because the callbacks always did the same thing and the variation in types was isolated to the
requestfunction, which would be specialized on different argument types, which would, of course cause specialization, but at least that was contained to one function.So, overall, it's unclear that this is an improvement. The main issue with the status quo is that the
outputandprogresschannels can grow unbounded, but in practice that seems unlikely to be a real issue.