Skip to content

Reduce allocations..?#313

Open
lmcd wants to merge 1 commit into
apple:mainfrom
lmcd:reduce-allocations
Open

Reduce allocations..?#313
lmcd wants to merge 1 commit into
apple:mainfrom
lmcd:reduce-allocations

Conversation

@lmcd

@lmcd lmcd commented Nov 24, 2021

Copy link
Copy Markdown

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
Copy Markdown

Can one of the admins verify this patch?

9 similar comments
@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@swift-server-bot

Copy link
Copy Markdown

Can one of the admins verify this patch?

@Lukasa

Lukasa commented Nov 25, 2021

Copy link
Copy Markdown
Contributor

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

Lukasa commented Nov 25, 2021

Copy link
Copy Markdown
Contributor

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

@lmcd

lmcd commented Nov 25, 2021

Copy link
Copy Markdown
Author

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

Lukasa commented Nov 25, 2021

Copy link
Copy Markdown
Contributor

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

Lukasa commented Nov 25, 2021

Copy link
Copy Markdown
Contributor

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