apple/swift-protobuf

Revisit `_NameMap` in light of Swift 5 changing the default `String` encoding

thomasvl opened this issue · 1 comments

For a while now I've been wondering if _NameMap should get a revisit since Swift 5 went UTF-8. We likely could drop all the allocations related to the InternPool, and we might actually end up at the point the compiler can just realized everything is concurrency safe again.

But given the Swift 5.10 new concurrency warnings around it (#1560), it seems like that might be even more reason to consider revisiting/simplifying it.

Adding a little more to what I was wondering:

  • Drop the InternPool
  • Maybe let Name continue with the enum storing StaticString vs. String; but this might not be needed (see below).
  • Stop Name from exposing the utf8Buffer, and instead put an function on it to have it append to a [UInt8].
    • That can do the fast/slow paths for accessing the data.
    • Update the encoders to use that api to write things out.
    • Maybe even mark this one as @inlinable, since it would still be internal, we might not have too.

Since all the uses of _NameMap should be from generated code, we'll get StaticStrings for everything but the case we have to compute the json name from the proto name; and in that case, we likely could even assert that what we generated still passes isContiguousUTF8 and/or just call makeContiguousUTF8() after making our String. So Name might not be needed at all, and _NameMap might be able to just store String directly.