Swift: encode & decode size-delimited messages #2424
Conversation
| // Use the size of the struct as an initial estimate for the space needed. | ||
| let structSize = MemoryLayout.size(ofValue: T.self) | ||
|
|
||
| let fullBuffer = WriteBuffer(capacity: structSize * values.count) |
There was a problem hiding this comment.
Should this calculation attempt to reserve some capacity for the varints or just let the buffer expand naturally?
There was a problem hiding this comment.
Better to allocate slightly more than slightly less (expanding requires copying memory). I would change to (8 + structSize) * values.count where the 8 is 64 bits for the varint.
There's probably already plenty of buffer realistically, since I think the struct size is almost always larger than what is written, but I'd still go with what I mentioned and we can do some profiling and optimizing at a later date.
There was a problem hiding this comment.
Updated to reserve space 👍
| @@ -36,7 +36,7 @@ final class WriteBuffer { | |||
| init(capacity: Int = 0) { | |||
| self.capacity = 0 | |||
|
|
|||
| if capacity > 0 { | |||
| if capacity >= 0 { | |||
There was a problem hiding this comment.
This was required to ensure storage was allocated in all cases (see testAppendEmptyFirst)
There was a problem hiding this comment.
I'm a little wary of calling malloc(0) in expand(). In C at least, the behavior for this can vary from runtime to runtime.
Can we achieve the same thing by adding something like this into append:
guard !data.isEmpty else { return }
There was a problem hiding this comment.
Added guards where appropriate
| try data.withUnsafeBytes { buffer in | ||
| // Handle the empty-data case. | ||
| guard let baseAddress = buffer.baseAddress, buffer.count > 0 else { | ||
| values = [try T(from: .empty)] |
There was a problem hiding this comment.
Does the spec specify that "no bytes" should be one empty object vs. an empty array? (Or can we verify with protoc?)
There was a problem hiding this comment.
This should have been an empty array
| // Use the size of the struct as an initial estimate for the space needed. | ||
| let structSize = MemoryLayout.size(ofValue: T.self) | ||
|
|
||
| let fullBuffer = WriteBuffer(capacity: structSize * values.count) |
There was a problem hiding this comment.
Better to allocate slightly more than slightly less (expanding requires copying memory). I would change to (8 + structSize) * values.count where the 8 is 64 bits for the varint.
There's probably already plenty of buffer realistically, since I think the struct size is almost always larger than what is written, but I'd still go with what I mentioned and we can do some profiling and optimizing at a later date.
| } | ||
|
|
||
| // write this value's size + contents to the main buffer | ||
| fullBuffer.writeVarint(UInt64(writer.buffer.count), at: fullBuffer.count) |
There was a problem hiding this comment.
FYI: We have some tricks inside the writer to avoid this write-then-copy inside of the ProtoWriter (see beginLengthDelimitedEncode). They're probably not worth doing here, but for future archaeologists who might be trying to optimize this I'll mention.
Basically, most messages will be in the size range that requires (IIRC) two bytes of varint, so we reserve two bytes, then write out value into the full buffer. If it happens to not take two bytes for its size, only then do we go back and move it to add more or less room for the prefix size.
In the future, we could extract this into an extension on WriteBuffer, like:
// This will reserve the length bytes, then write the buffer, then adjust and fill in the length
// bytes based on what was written.
buffer.writeLengthEncoded { tempBuffer in
let writer = ProtoWriter(data: tempBuffer, ...)
try value.encode(to: writer)
}
Again, all a good future enhancement. Not required now.
| @@ -36,7 +36,7 @@ final class WriteBuffer { | |||
| init(capacity: Int = 0) { | |||
| self.capacity = 0 | |||
|
|
|||
| if capacity > 0 { | |||
| if capacity >= 0 { | |||
There was a problem hiding this comment.
I'm a little wary of calling malloc(0) in expand(). In C at least, the behavior for this can vary from runtime to runtime.
Can we achieve the same thing by adding something like this into append:
guard !data.isEmpty else { return }
jszumski
force-pushed
the
jszumski/swift-size-delimited-messages
branch
from
238c348
to
41fdf6a
Compare
jszumski
force-pushed
the
jszumski/swift-size-delimited-messages
branch
from
41fdf6a
to
7baa8cc
Compare
Adds the ability to encode and decode a size-delimited message collection in Swift.
The intended use case is to decode a stream of messages recorded to disk (e.g., Bazel's Build Event Protocol). Similar implementations exist in Java (writeDelimitedTo and parseDelimitedFrom) and other languages in protocolbuffers/protobuf#10229.