Avoid unnecessary heap allocation when parsing frame headers. #107
Conversation
|
This has a fraction of one percent of a performance improvement in the real-world benchmark I'm running: just not really noticeable. Still worth avoiding the allocation, but just worth keeping it in perspective. |
| @@ -1163,13 +1163,14 @@ fileprivate extension ByteBuffer { | |||
| return nil | |||
| } | |||
|
|
|||
| let lenBytes: [UInt8] = self.readBytes(length: 3)! | |||
There was a problem hiding this comment.
I think you should keep the old code but replace
let lenBytes: [UInt8] = self.readBytes(length: 3)!
with
let lenHigh = self.readInteger(as: UInt16.self)
let lenLow = self.readInteger(as: UInt8.self)
let len = UInt32(lenHigh) << 16 | UInt32(lenLow)
| @@ -1163,13 +1163,14 @@ fileprivate extension ByteBuffer { | |||
| return nil | |||
There was a problem hiding this comment.
I don't really like the pre-calculation up here ^^^ and the !s down there. I think this should be replaced with
let saveSelf = self
guard let lenHigh = self.readInteger(as: UInt16.self),
let lenLow = self.readInteger(as: UInt8.self),
let lenLow = self.readInteger(as: UInt8.self),
let type = self.readInteger(as: UInt8.self),
let flags = self.readInteger(as: UInt8.self),
let streamID = self.readInteger(as: UInt32.self) else {
self = saveSelf
return nil
}
return FrameHeader(...)There was a problem hiding this comment.
I disagree.
Specifically, the form you proposed does repeated and unnecessary bounds checking, incurring a bunch of potentially unnecessary pointer dereferences. Done maliciously it substantially increases the work needed to parse a frame header by forcing a quadratic behaviour when parsing frame headers.
There was a problem hiding this comment.
There are no pointer dereferences and no quadratic behaviour.
There was a problem hiding this comment.
Sure there are, reading data from the ByteBuffer is a pointer dereference.
There was a problem hiding this comment.
the bounds checks are integer comparisons. I agree that we should add a way that can hoist the bounds checks but we don't have one today. Going into unsafe-land is not a good excuse to reduce a constant factor
There was a problem hiding this comment.
For the future, I want something like
self.read( integer(UInt16.self) <*> integer(UInt8.self) <*> integer(UInt8.self) <*> integer(UInt8.self) )
that does the bounds check only once. But we're not there yet.
Motivation: As noted in apple#105, I spotted that we were heap-allocating an array when parsing the frame header. That heap allocation isn't necessary, we can just write directly into an Int, which should save us some heap spray. Modifications: - Rewrote code to avoid heap allocating array and instead to use Int. - Elided some bounds checks by using an unsafe pointer, since I was already here and it saved about 1% performance in a hot loop. Result: Fewer heap allocations on the hot code path.
Lukasa
force-pushed
the
cb-burn-frame-allocations
branch
from
bb71ad6 to
d438c23
Compare
| let rawStreamID: UInt32 = self.readInteger()! | ||
|
|
||
| return FrameHeader(length: len, type: type, flags: FrameFlags(rawValue: flags), rawStreamID: rawStreamID) | ||
| return FrameHeader(length: Int(lenHigh) << 8 | Int(lenLow), type: type, flags: FrameFlags(rawValue: flags), rawStreamID: rawStreamID) |
There was a problem hiding this comment.
aren't we overwriting 8 bits here?
There was a problem hiding this comment.
No: the UInt16 is converted to an Int and shifted left 8 bits. Then the UInt8 is converted to an Int and or'ed with it, fitting into the empty space from the previous left shift.
There was a problem hiding this comment.
@Lukasa oops, thanks, brain fart :D
Motivation:
As noted in #105, I spotted that we were heap-allocating an array
when parsing the frame header. That heap allocation isn't necessary, we
can just write directly into an Int, which should save us some heap spray.
Modifications:
already here and it saved about 1% performance in a hot loop.
Result:
Fewer heap allocations on the hot code path.
Resolves #105