Remove unnecessary state CoWs. #104
Conversation
|
The unfortunate irony here is that the net performance gain of this patch in a more real-world benchmark is pretty small: about 1.5%. The reality is that in that benchmark we just aren't copying too much state around, so the CoWs don't hurt too badly. |
| var settingsState = HTTP2SettingsState(localState: true) | ||
| settingsState.emitSettings(settings) | ||
| self.state = .prefaceSent(.init(fromIdle: state, localSettings: settingsState)) | ||
| self.avoidingStateMachineCoW { [] newState in |
There was a problem hiding this comment.
mind removing the left-over [] ?
There was a problem hiding this comment.
It's there on purpose to avoid me ever accidentally capturing in this block.
There was a problem hiding this comment.
@Lukasa that doesn't work. [...] has no effect on things that are not listed
There was a problem hiding this comment.
I complain for years that Swift needs a mode where I can specify an exhaustive capture list, but still not there :(
There was a problem hiding this comment.
the only thing [ does is to create a by-value copy of all the listed things, optionally with renaming
Motivation: When benchmarking the HTTP/2 implementation I noticed that we were performing a large number of CoW operations on the Dictionary that backs the ConnectionStreamState structure: specifically, once per frame sent or received. This is substantial and unnecessary allocation overhead that drags down the performance of the program, sprays the heap with small allocations, and generally just leads to nasty performance costs with no gain. Sadly, this turns out not to be the easiest CoW to get rid of, because the ConnectionStreamState structure is stored in associated data on the connection state machine state enumeration. This means that our usual tricks for avoiding the CoW (such as swap()) no longer work. The only solution is to add a new state that corresponds to no state at all so that we can make the connection state go away while we mutate it. This is all a bit gross, frankly. Unfortunately, the act of making this simple change leads to a huge diff as we have to tweak basically every state machine function in order to ensure that it does the right thing. Modifications: - Added a function to control CoWing the state machine. - Used that function basically everywhere. - Made sure that function had assertions. - Annoyingly had to increase the visibility of a bunch of structs. Result: Fewer CoWs, better performance, sadder code reviewers.
Lukasa
force-pushed
the
cb-fix-cow-in-state-machine
branch
from
36a9df9
to
2af3eb8
Compare
|
Oh god, I hope I don't get retained, I don't need a cow. |
It's fine, you just killed a lot of cows. Enjoy 🥩 |
I think in the real world under allocator pressure this could have a much bigger impact. but who knows... |
|
Indeed, I'm still strongly in favour of removing it: it's just not a really heavy hitter. |
Motivation: This is basically a similar issue to apple#104: we have a CoW data structure (ByteBuffer) in some of these states, and when calling append on them they force an unnecessary CoW. We can reproduce the fix from that patch, which is to define an empty state and swap into it before we run the append. We also define a helper that ensures that we don't screw this up. Modifications: - Added new appending state to HTTP2FrameDecoder - Added helpers for avoiding a CoW - Avoided CoW in HTTP2FrameDecoder. Result: Fewer unnecessary copies
Motivation: This is basically a similar issue to #104: we have a CoW data structure (ByteBuffer) in some of these states, and when calling append on them they force an unnecessary CoW. We can reproduce the fix from that patch, which is to define an empty state and swap into it before we run the append. We also define a helper that ensures that we don't screw this up. Modifications: - Added new appending state to HTTP2FrameDecoder - Added helpers for avoiding a CoW - Avoided CoW in HTTP2FrameDecoder. Result: Fewer unnecessary copies
Motivation:
When benchmarking the HTTP/2 implementation I noticed that we were
performing a large number of CoW operations on the Dictionary that backs
the ConnectionStreamState structure: specifically, once per frame sent or
received.
This is substantial and unnecessary allocation overhead that drags down
the performance of the program, sprays the heap with small allocations, and
generally just leads to nasty performance costs with no gain.
Sadly, this turns out not to be the easiest CoW to get rid of, because
the ConnectionStreamState structure is stored in associated data on the
connection state machine state enumeration. This means that our usual tricks
for avoiding the CoW (such as swap()) no longer work.
The only solution is to add a new state that corresponds to no state at all
so that we can make the connection state go away while we mutate it. This is
all a bit gross, frankly. Unfortunately, the act of making this simple change
leads to a huge diff as we have to tweak basically every state machine
function in order to ensure that it does the right thing.
Modifications:
Result:
Fewer CoWs, better performance, sadder code reviewers.