add fast-path optimizations for reading LibuvStreams #14667
Conversation
fixes #14467 performance regression (and some)
|
👍 Does @nanosoldier have any IO performance tests now? |
Not yet, but it will soon. The |
add fast-path optimizations for reading LibuvStreams
| wait_readnb(s, nb) | ||
| nr = nb_available(s) | ||
| resize!(b, nr) # shrink to just contain input data if was resized | ||
| read!(s.buffer, b) |
There was a problem hiding this comment.
this won't work for UDPSocket or anything else without a buffer field
There was a problem hiding this comment.
it also doesn't support read or write (it uses send and recv), so i'm not too worried about that. we could extend the type-hierarchy to exclude it (because it isn't a stream) explicitly, but to what end?
There was a problem hiding this comment.
it's a buggy type because of this field assumption. I forget the issue number, but sounds like this abstract type isn't appropriate for it
There was a problem hiding this comment.
I'm not sure why something should be an IO type if it doesn't have read and write.
tkelman
added
needs tests
| @@ -87,8 +87,13 @@ end | |||
| nb_available(s::LibuvStream) = nb_available(s.buffer) | |||
|
|
|||
| function eof(s::LibuvStream) | |||
| if isopen(s) # fast path | |||
| nb_available(s) > 0 && return true | |||
There was a problem hiding this comment.
Should this return false instead?
There was a problem hiding this comment.
do add a test when you fix this
|
👍 |
|
@vtjnash what is a "fast path optimization" ? |
|
in this case, copying the cheap epilogue of the function into the prologue, to detect when we don't need to call the expensive code in the middle |
| @@ -876,6 +894,14 @@ function stop_reading(stream::LibuvStream) | |||
| end | |||
| end | |||
|
|
|||
| function readbytes!(s::LibuvStream, b::AbstractArray{UInt8}, nb=length(b)) | |||
| wait_readnb(s, nb) | |||
There was a problem hiding this comment.
This line breaks MbedTLS and everything that depends on it. I think wait_readnb should probably have a wider signature, Integer instead of Int?
Line 358 in 0a00109
(also in need of tests)
| function readbytes!(s::LibuvStream, b::AbstractArray{UInt8}, nb=length(b)) | ||
| wait_readnb(s, nb) | ||
| nr = nb_available(s) | ||
| resize!(b, nr) # shrink to just contain input data if was resized |
There was a problem hiding this comment.
And this line won't work if you try to give it an array that shares data, cannot resize array with shared data - MbedTLS.jl does this via pointer_to_array with an array that is managed by the C library.
fixes #14467 performance regression (and some).
these functions aren't supposed to be on the fast-path, but they were. adding appropriate short-circuit paths to those fixes the performance regression noted in #14467. moving them off of the fast-path (by providing a more optimal implementation of
readbytes!) makes the simple echo server twice as fast. we should now be about 2x the 0mq speed on the provided test cases (about 10x the super-simple blocking C code).