Avoid CoW while processing the next state when HTTP2FrameDecoder decodes #438
Conversation
Motivation: CoWs appear when switching over the current state of the parser and holding it while also modifying it - inside `processNExtState()`. Following `append(bytes: ByteBuffer)`'s pattern, we should use a function that sets the state to an intermediary one with no associated data, before making the transformations. Modifications: - created the `avoidParserCoW()` helper function for the throwing functions - used it in the switch cases inside `processNextState()` Result: CoW will be avoided when changing the state of the HTTP2FrameDecoder when decoding, so we won't have unnecessary heap allocations.
| /// This is the implementation for a throwing function. If the state doesn't get updated because an error | ||
| /// was thrown, this function sets the parser state to the initial one. | ||
| /// | ||
| /// The `body` function must assign a value to the parser state, otherwise it will be the intermediary | ||
| /// `appending` state, which is not wanted and it results in the this function's assertion failure. | ||
| @inline(__always) | ||
| private mutating func avoidingParserCoW<ReturnType>(_ body: (inout ParserState) throws -> ReturnType) throws -> ReturnType { |
There was a problem hiding this comment.
Rather than adding a complete new function can we just modify the existing one to accept a throwing closing and use rethrows to propagate the throwing-ness of the closure?
| return ParseResult.needMoreData | ||
| } | ||
| newState = .accumulatingFrameHeader(processResult) | ||
| return ParseResult.continue |
There was a problem hiding this comment.
We should be able to use type inference here (and elsewhere in this PR)
| return ParseResult.needMoreData | |
| } | |
| newState = .accumulatingFrameHeader(processResult) | |
| return ParseResult.continue | |
| return .needMoreData | |
| } | |
| newState = .accumulatingFrameHeader(processResult) | |
| return .continue |
| case .initialized: | ||
| // no bytes, no frame | ||
| return .needMoreData | ||
|
|
||
There was a problem hiding this comment.
nit: trailing whitespace
|
@swift-server-bot add to allowlist |
| @inline(__always) | ||
| private mutating func avoidingParserCoW<ReturnType>(_ body: (inout ParserState) -> ReturnType) -> ReturnType { | ||
| private mutating func avoidingParserCoW<ReturnType>(_ body: (inout ParserState) throws -> ReturnType) rethrows -> ReturnType { | ||
| let initialState = self.state |
There was a problem hiding this comment.
Sorry I didn't catch this before, but I think by doing this we're undoing the effectiveness of this function. The reason is that the closure will be capturing the associated payload held by self.state, now we're grabbing it here again so we'll CoW if we mutate the one we captured in the closure.
I don't think restoring the state in case of an error is actually necessary either: the previous code didn't do it.
There was a problem hiding this comment.
You are right, I'm trying to think how to solve this. The problem is that in the case of an error that assertion that checks the final state fails - the previous implementation didn't receive a throwing function as a parameter so the state was always updated to a valid one (in the body of that function). In the append() function where avoidingParserCoW was already used there are no throwing function calls.
There was a problem hiding this comment.
So when we are not updating it to the initial value the tests in which the process function throws fail - as the assertion from avoidingParserCoW fails. For example:
func testContinuationFrameStreamZero() throws {
var buf = byteBuffer(withBytes: frameBytes, extraCapacity: 10)
let continuationFrameBytes: [UInt8] = [
0x00, 0x00, 0x07, // 3-byte payload length (7 bytes)
0x09, // 1-byte frame type (CONTINUATION)
0x04, // 1-byte flags (END_HEADERS)
0x00, 0x00, 0x00, 0x00, // 4-byte stream identifier, stream 0
]
buf.writeBytes(continuationFrameBytes)
buf.writeBuffer(&headers2)
// This should fail
decoder.append(bytes: buf)
XCTAssertThrowsError(try decoder.nextFrame()) { error in
XCTAssertEqual(error as? InternalError, .codecError(code: .protocolError))
}
}
There was a problem hiding this comment.
should we get rid of that assertion if an error is thrown?
There was a problem hiding this comment.
I think you have to enforce that the closure updates the state if it throws.
You can police this in code by making this function non-throwing. Each closure which wants to throw can then catch the error in the closure and return the error in a Result type.
There was a problem hiding this comment.
should we get rid of that assertion if an error is thrown?
No the assertion is there for a reason: we must never leave state in the appending state, otherwise the next call which switches over the state won't have any idea what it can do!
| return try self.avoidingParserCoW { newState in | ||
| let result = Result<ParseResult, Error> { | ||
| guard let processResult = try state.process() else { | ||
| newState = .awaitingClientMagic(state) | ||
| return .needMoreData | ||
| } | ||
| newState = .accumulatingFrameHeader(processResult) | ||
| return .continue | ||
| } | ||
|
|
||
| self.state = .accumulatingFrameHeader(newState) | ||
| return .continue | ||
|
|
||
| switch result { | ||
| case let .success(parseResult): | ||
| return parseResult | ||
| case let .failure(error): | ||
| newState = .awaitingClientMagic(state) | ||
| throw error | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you fix the indentation here?
Motivation:
CoWs appear when switching over the current state of the parser and holding it while also modifying it - inside
processNExtState(). Followingappend(bytes: ByteBuffer)'s pattern, we should use a function that sets the state to an intermediary one with no associated data, before making the transformations.Modifications:
avoidParserCoW()helper function to take into consideration throwing functionsprocessNextState()Result:
CoW will be avoided when changing the state of the HTTP2FrameDecoder while decoding, so we won't have unnecessary heap allocations.