Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPointer #883

glbrntt · 2019-06-06T15:24:00Z

This change allows users who already have a buffer allocated to
serialize a message straight into that buffer, avoiding an additional
copy via Data.

The motivation behind this is that in Swift gRPC the currency for
storing bytes is NIOs ByteBuffer, and we'd like to avoid having
an additional copy from going via Data. As such this change

makes serializedDataSize public and renames it serializedBinarySize
adds a serializeBinary(into:partial:) method, and
adds an init to Message to read from an UnsafeRawBufferPointer.

A couple of errors in the documentation were also fixed.

I also saw issue #816 after implementing this so more than happy to
hear what everyone has to think about this!

…nter. This change allows users who already have a buffer allocated to serialize a message straight into that buffer, avoiding an additional copy via Data.

glbrntt · 2019-06-06T15:25:44Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

+  /// This should be used with `serializeBytes(into:partial:)` so that enough space may
+  /// be allocated for the buffer so that encoding can proceed without bounds checks
+  /// or reallocation.
+  public func serializedBinarySize() throws -> Int {
    // Note: since this api is internal, it doesn't currently worry about


I wasn't sure how to handle this comment: it seems like we shouldn't need to check for required fields as that's left to the serialize methods?

Probably just promote it to a public comment.

thomasvl · 2019-06-06T15:28:35Z

Quick comment:

makes serializedDataSize public and renames it serializedBinarySize

We'd sorta been avoiding this because it lets folks fall into the trap of calling it and then fetching the Data anyways meaning the do extra passes over the proto graph.

glbrntt · 2019-06-06T15:41:52Z

Ah, okay, that makes sense.

I suppose another option would be keeping it internal and changing serializeBinary(into:partial:) to something like serializeBinary(partial: Bool = true, provider: (Int) -> UnsafeMutableRawBufferPointer) and passing the size into the closure?

thomasvl · 2019-06-06T15:33:37Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

@@ -31,26 +31,24 @@ extension Message {
    if !partial && !isInitialized {
      throw BinaryEncodingError.missingRequiredFields
    }


This should get removed, otherwise the validation is done twice because of the chaining of calls.

thomasvl · 2019-06-06T15:34:52Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

+  /// - Returns: The number of bytes written into the buffer.
+  /// - Throws: `BinaryEncodingError` if encoding fails.
+  @discardableResult
+  public func serializeBinary(into buffer: UnsafeMutableRawBufferPointer, partial: Bool = false) throws -> Int {


can you wrap this like the others are, we mostly try to keep to 80c

thomasvl · 2019-06-06T15:37:08Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

-                                      extensions: extensions)
-          try decoder.decodeFullMessage(message: &self)
-        }
+        try self.merge(body: body, extensions: extensions, options: options)
      }
    }
    if !partial && !isInitialized {


Aren't we checking twice here also? I guess here you need it just in an else clause for when data was empty.

thomasvl · 2019-06-06T15:42:03Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

+  ///     `BinaryDecodingError.missingRequiredFields`.
+  ///   - options: The BinaryDecodingOptions to use.
+  /// - Throws: `BinaryDecodingError` if decoding fails.
+  public init(


Maybe group the two init methods together?

thomasvl · 2019-06-06T15:43:42Z

Sources/SwiftProtobuf/Message+BinaryAdditions.swift

+  ///     `BinaryEncodingError.missingRequiredFields`.
+  /// - Returns: The number of bytes written into the buffer.
+  /// - Throws: `BinaryEncodingError` if encoding fails.
+  @discardableResult


Seems like we shouldn't add this annotation as callers should always use this value, no?

thomasvl · 2019-06-06T15:50:26Z

Tests/SwiftProtobufTests/TestHelpers.swift

@@ -42,6 +42,22 @@ extension PBTestHelpers where MessageTestType: SwiftProtobuf.Message & Equatable
            } catch {
                XCTFail("Failed to decode protobuf: \(string(from: encoded))", file: file, line: line)
            }
+
+            let expectedSize = try configured.serializedBinarySize()


Please add tests that use serializeBinary(into:) with a few more cases:

a buffer a little too short and a message with fields set to ensure that a proper error is thrown (looks like BinaryEncodingError doesn't have something for "too short", so the current code might not support this case (just assert), so it might take a lot of plumbing to get this right.

a zero length buffer and a message with no fields set (since the binary size will be zero) and ensure it passes

a zero length buffer and a message with fields set to ensure that does error.

a buffer larger than needed and a message with fields set to ensure it works and reports the correct size.

a buffer larger than needed and a message with no fields set to ensure it works and reports zero.

thomasvl · 2019-06-06T15:51:58Z

I suppose another option would be keeping it internal and changing serializeBinary(into:partial:) to something like serializeBinary(partial: Bool = true, provider: (Int) -> UnsafeMutableRawBufferPointer) and passing the size into the closure?

That was about the only thing I could think of, but I wasn't sure how well that would work for consumers. Would that work for grpc/swiftnio based things?

tbkka · 2019-06-06T16:04:17Z

serializeBinary(partial: Bool = true, provider: (Int) -> UnsafeMutableRawBufferPointer)

I'm not sure that works, actually. MutableRawBufferPointers are vended only as closure arguments in Swift; they're never returned as results. I think you'd have to do something like this:

   serializeBinary(
        partial: Bool,
        provider: (Int, (UnsafeMutableRawBufferPointer) throws -> Int) throws
   ) throws -> Int

That is, the provider is given the size of the message. It then calls another provided function with the requested buffer pointer.

allevato · 2019-06-06T17:21:53Z

Rather than providing an API that works with unsafe pointers, we should consider using the new data protocols in Foundation that come as part of 5.1. My comment in the related issue:

From a Foundation session at WWDC: Data is always only going to be contiguous. They've introduced the new DataProtocol and MutableDataProtocol and provide many of the same Data operations but don't promise contiguity.

It looks like NIOFoundationCompat has a ByteBufferView that conforms to DataProtocol & ContiguousBytes.

So, it may be worth figuring out how to make our serialization methods generic over DataProtocol and also provide fast-paths over DataProtocol & ContiguousBytes.

glbrntt · 2019-06-07T11:38:28Z

serializeBinary(partial: Bool = true, provider: (Int) -> UnsafeMutableRawBufferPointer)

I'm not sure that works, actually. MutableRawBufferPointers are vended only as closure arguments in Swift; they're never returned as results. I think you'd have to do something like this:
   serializeBinary(
        partial: Bool,
        provider: (Int, (UnsafeMutableRawBufferPointer) throws -> Int) throws
   ) throws -> Int
That is, the provider is given the size of the message. It then calls another provided function with the requested buffer pointer.

You're totally right, thanks. It feels a little clunky but I don't think that's necessarily a bad thing considering it's "unsafe".

glbrntt · 2019-06-07T11:42:27Z

Rather than providing an API that works with unsafe pointers, we should consider using the new data protocols in Foundation that come as part of 5.1. My comment in the related issue:

Are you opposed to having an unsafe pointer API entirely? Using the new protocols with NIOs ByteBuffer is fine for deserialization, but I'm not sure how feasible it is for serialization. The idea of deserializing from non-contiguous DataProtocol is interesting though.

glbrntt · 2019-06-14T12:24:01Z

Closing this in favour of using DataProtocol.

Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPoi…

102ef3e

…nter. This change allows users who already have a buffer allocated to serialize a message straight into that buffer, avoiding an additional copy via Data.

glbrntt commented Jun 6, 2019

View reviewed changes

thomasvl requested changes Jun 6, 2019

View reviewed changes

glbrntt closed this Jun 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPointer #883

Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPointer #883

glbrntt commented Jun 6, 2019

glbrntt Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl commented Jun 6, 2019

glbrntt commented Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl Jun 6, 2019

thomasvl commented Jun 6, 2019

tbkka commented Jun 6, 2019

allevato commented Jun 6, 2019

glbrntt commented Jun 7, 2019

glbrntt commented Jun 7, 2019

glbrntt commented Jun 14, 2019

Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPointer #883

Allow messages to be (de/)serialized from Unsafe[Mutable]RawBufferPointer #883

Conversation

glbrntt commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasvl commented Jun 6, 2019

glbrntt commented Jun 6, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasvl commented Jun 6, 2019

tbkka commented Jun 6, 2019

allevato commented Jun 6, 2019

glbrntt commented Jun 7, 2019

glbrntt commented Jun 7, 2019

glbrntt commented Jun 14, 2019