-
Notifications
You must be signed in to change notification settings - Fork 83
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Reduce allocations..? #313
base: main
Are you sure you want to change the base?
Conversation
Can one of the admins verify this patch? |
9 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
In practical cases this may reduce allocations. We'll get no more than one actual read per read cycle if the NIOSSLHandler is in front of us in the pipeline, in which case this can reduce the allocations. Did you profile your allocations? If so, can you attach an Instruments trace? We can likely track down exactly where those allocations are coming from and work out if they're necessary. |
Note also that we accumulate bytes in more than one place, so you'd want to tweak almost all of these. |
I don't have the machine in front of me right now, but from memory this was consuming about 2-3% of CPU, and after this update it wasn't showing up in the instruments trace at all. I addressed that specific struct as that was the only one that was registering some CPU pressure. This might not be an issue under normal use, I'm just specifically profiling long-running file uploads locally (e.g. uploading a 1GB file) |
I'd really like to see an instruments trace if we can. Out of curiosity, were you running in release mode or debug mode? |
Incidentally, one reason I'm asking for this is that this change could just move the work from this function to a different one. NIO attempts to re-use read buffers, and depending on the I/O pattern this can prevent that from happening. |
I've been profiling large HTTP upload requests in swift-nio, and have noticed a lot of copying/allocations going on in
HTTPFrameDecoder
.Is it possible in this instance to prevent the copy by just switching the accumulating byte buffer to the incoming one if the accumulate bytes are still zero? I don't know NIO well enough yet to understand what side effects this may have.