Skip to content

Fix CoW in HTTPResponseAggregator #345

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

Merged
merged 1 commit into from
Mar 8, 2021

Conversation

Lukasa
Copy link
Collaborator

@Lukasa Lukasa commented Mar 8, 2021

Motivation:

HTTPResponseAggregator attempts to build a single, complete response
object. This necessarily means it loads the entire response payload into
memory. It wants to provide this payload as a single contiguous buffer
of data, and it does so by aggregating the data into a single contiguous
buffer as it goes.

Because ByteBuffer does exponential reallocation, the cost of doing this
should be amortised constant-time, even though we do have to copy some
data sometimes. However, if this operation triggers a copy-on-write then
the operation will become quadratic. For large buffers this will rapidly
come to dominate the runtime.

Unfortunately in at least Swift 5.3 Swift cannot safely see that during
the body stanza the state variable is dead. Swift is not necessarily
wrong about this: there's a cross-module call to ByteBuffer.writeBuffer
in place and Swift cannot easily prove that that call will not lead to a
re-entrant access of the HTTPResponseAggregator object. For this
reason, during the call to didReceiveBodyPart there will be two copies
of the body buffer alive, and so the write will CoW.

This quadratic behaviour is a nasty performance trap that can become
highly apparent even at quite small body sizes.

Modifications:

While Swift can't prove that the self.state variable is dead, we can!
To that end, we temporarily set it to a different value that does not
store the buffer in question. This will force Swift to drop the ref on
the buffer, making it uniquely owned and avoiding the CoW.

Sadly, it's extremely difficult to test for "does not CoW", so this
patch does not currently come with any tests. I have experimentally
verified the behaviour.

Result:

No copy-on-write in the HTTPResponseAggregator during body aggregation.

Motivation:

HTTPResponseAggregator attempts to build a single, complete response
object. This necessarily means it loads the entire response payload into
memory. It wants to provide this payload as a single contiguous buffer
of data, and it does so by aggregating the data into a single contiguous
buffer as it goes.

Because ByteBuffer does exponential reallocation, the cost of doing this
should be amortised constant-time, even though we do have to copy some
data sometimes. However, if this operation triggers a copy-on-write then
the operation will become quadratic. For large buffers this will rapidly
come to dominate the runtime.

Unfortunately in at least Swift 5.3 Swift cannot safely see that during
the body stanza the state variable is dead. Swift is not necessarily
wrong about this: there's a cross-module call to ByteBuffer.writeBuffer
in place and Swift cannot easily prove that that call will not lead to a
re-entrant access of the `HTTPResponseAggregator` object. For this
reason, during the call to `didReceiveBodyPart` there will be two copies
of the body buffer alive, and so the write will CoW.

This quadratic behaviour is a nasty performance trap that can become
highly apparent even at quite small body sizes.

Modifications:

While Swift can't prove that the `self.state` variable is dead, we can!
To that end, we temporarily set it to a different value that does not
store the buffer in question. This will force Swift to drop the ref on
the buffer, making it uniquely owned and avoiding the CoW.

Sadly, it's extremely difficult to test for "does not CoW", so this
patch does not currently come with any tests. I have experimentally
verified the behaviour.

Result:

No copy-on-write in the HTTPResponseAggregator during body aggregation.
@Lukasa Lukasa added the 🔨 semver/patch No public API change. label Mar 8, 2021
@Lukasa Lukasa requested review from artemredkin, weissi and glbrntt March 8, 2021 10:17
@Lukasa Lukasa merged commit 0dda95c into swift-server:main Mar 8, 2021
@Lukasa Lukasa deleted the cb-fix-cow-in-response-aggregator branch March 8, 2021 10:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🔨 semver/patch No public API change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants