Skip to content
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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reduce allocations..? #313

wants to merge 1 commit into from

Conversation

lmcd
Copy link

@lmcd lmcd commented Nov 24, 2021

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.

@swift-server-bot
Copy link

Can one of the admins verify this patch?

9 similar comments
@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@swift-server-bot
Copy link

Can one of the admins verify this patch?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2021

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.

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2021

Note also that we accumulate bytes in more than one place, so you'd want to tweak almost all of these.

@lmcd
Copy link
Author

lmcd commented Nov 25, 2021

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)

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2021

I'd really like to see an instruments trace if we can. Out of curiosity, were you running in release mode or debug mode?

@Lukasa
Copy link
Contributor

Lukasa commented Nov 25, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants