BinaryEncoder look at using withContiguousStorageIfAvailable for String.utf8

[thomasvl]

Currently the BinaryEncoder pulls over string data a byte at a time, but it looks like we could fastpath with withContiguousStorageIfAvailable and fall back to the loop only if need be.

[RLovelett]

I'm interested in doing this one. I've even come up with what I think is a reasonable first attempt at a patch.

The part that might be unexpected and not great is the modification to append(contentsOf:). I made that function return a value to silence the Constant 'isAvailable' inferred to have type '()?', which may be unexpected.

From 7e6da390148ea6a782c0c4040563069a89f4c84a Mon Sep 17 00:00:00 2001
From: Ryan Lovelett <RLovelett@users.noreply.github.com>
Date: Thu, 6 Feb 2020 11:01:22 -0500
Subject: [PATCH 1/1] Use withContiguousStorageIfAvailable for String encoding

---
 Sources/SwiftProtobuf/BinaryEncoder.swift | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/Sources/SwiftProtobuf/BinaryEncoder.swift b/Sources/SwiftProtobuf/BinaryEncoder.swift
index b3c73f18..6e3800b1 100644
--- a/Sources/SwiftProtobuf/BinaryEncoder.swift
+++ b/Sources/SwiftProtobuf/BinaryEncoder.swift
@@ -36,10 +36,12 @@ internal struct BinaryEncoder {
         pointer = pointer.advanced(by: count)
     }
 
-    private mutating func append(contentsOf bufferPointer: UnsafeBufferPointer<UInt8>) {
+    @discardableResult
+    private mutating func append(contentsOf bufferPointer: UnsafeBufferPointer<UInt8>) -> Int {
         let count = bufferPointer.count
         pointer.assign(from: bufferPointer.baseAddress!, count: count)
         pointer = pointer.advanced(by: count)
+        return count
     }
 
     func distance(pointer: UnsafeMutablePointer<UInt8>) -> Int {
@@ -120,12 +122,17 @@ internal struct BinaryEncoder {
 
     // Write a string field, including the leading index/tag value.
     mutating func putStringValue(value: String) {
+      // If the collection does not support an internal representation in a form of contiguous storage,
+      // body is not called and nil is returned.
+      let isAvailable = value.utf8.withContiguousStorageIfAvailable { self.append(contentsOf: $0) }
+      if isAvailable == nil {
         let count = value.utf8.count
         putVarInt(value: count)
         for b in value.utf8 {
             pointer.pointee = b
             pointer = pointer.successor()
         }
+      }
     }
 
     mutating func putBytesValue(value: Data) {
-- 
2.25.0

Should I just make a PR and discuss there?

[tbkka]

Yes, please send a PR. I would also be very interested if you could run the Performance test with/without your change and see what the impact is.

[RLovelett]

@tbkka I've made the pull request. It is not clear to me though how I run the performance tests.

[thomasvl]

Done via #949