• Created a separate PR that adds the existential any to Message here: Explicitly annotate existential usage of Message with the existential any keyword. #1509. This PR will not build independently since it requires the changes from Explicitly annotate existential usage of Message with the existential any keyword. #1509 to build. I don't see a way to mark this dependency in the github ui.

• Separated out the core API changes and message re-generation into separate commits. The core changes are here: 5de597d

Re the size changes - It's been a while since I looked into this, but at one point, I believe for readability and maybe for (debug?) code sizes, we were special casing generation for when a switch would only get one case, and generating an if instead. No idea if that would still make sense for some of your additions, but it might be something to look at again.

Do you see any impact to runtime memory use?

I'm debating whether your approach is preferable to changing the existing traverse methods to visit every field unconditionally. For example, I wonder if duration.pb.swift should change from

  public func traverse<V: Visitor>(visitor: inout V) throws {
    if self.seconds != 0 {
      try visitor.visitSingularInt64Field(value: self.seconds, fieldNumber: 1)
    }
    if self.nanos != 0 {
      try visitor.visitSingularInt32Field(value: self.nanos, fieldNumber: 2)
    }
    try unknownFields.traverse(visitor: &visitor)
  }

to

  public func traverse<V: Visitor>(visitor: inout V) throws {
    try visitor.visitSingularInt64Field(value: self.seconds, fieldNumber: 1, isNonDefault: self.seconds != 0)
    try visitor.visitSingularInt32Field(value: self.nanos, fieldNumber: 2, isNonDefault: self.nanos != 0)
    try unknownFields.traverse(visitor: &visitor)
  }

The latter form would likely be similar performance to yours, with similar code size and memory usage to the existing code.

Specifically, your code introduces several new generic types, which might need to be instantiated at runtime, and it adds some new large data structures that might get lazily allocated at runtime. For large projects with lots of protos, those could be problems. (I'm not certain how these different impacts will play out, but it's something I'd like to measure before committing to a particular approach.)

I haven't done any memory performance benchmarking yet. I can work on that tonight and tomorrow. I expect the addition of these static fields will cause an increase in memory usage, although I don't expect the impact to be too high since Field is a pretty small struct.

I think your outlined func traverse example is the optimal solution in terms of performance and I'm fine pivoting to that approach if you'd like. I think it would be a much simpler change.

The main downside of that approach is that it makes it impossible to make changes to the implementation of traverse without introducing a major version bump. This is the reason #861 has remained open for so long. By making the implementation of traverse internal to SwiftProtobuf we can add new visitor options without making braking changes. Like adding the ability to visit default fields, or the ability to visit unset fields, or maybe even adding some multithreaded/parallelized version of traverse.

... I'm fine pivoting to that approach if you'd like. ...

I'm not sure yet, that's why I'd like to get more information about the overhead of different approaches. I recall experimenting with a lot of different variations before settling on the current traverse implementation. Your approach intrigues me because it is one that I did not consider. 🤔 And to be honest, runtime memory usage is something I did not test as carefully as I should have, so it's quite possible that the existing code is not as efficient in that respect as I'd like.

The future possibilities you list don't really sound very compelling to me: visiting default/unset fields is something we can implement for 2.0 as a breaking change in a couple of different ways. (And note that your change does break the API between the generated code and the library, so we can only adopt it for 2.0.) No other new options have been suggested in a few years now, so I'm not too worried about trying to allow for future expansion there. Parallelizing also doesn't seem very appealing -- I doubt the overhead would be worth it.

The most interesting aspect of your proposal to me is that it could let us re-implement == as a pure library implementation, without any generated code. That's something that we cannot easily do with the current design, and could be a significant code size win. But that assumes that your proposal doesn't have other costs that we don't yet understand.

[...]

  public func traverse<V: Visitor>(visitor: inout V) throws {
    try visitor.visitSingularInt64Field(value: self.seconds, fieldNumber: 1, isNonDefault: self.seconds != 0)
    try visitor.visitSingularInt32Field(value: self.nanos, fieldNumber: 2, isNonDefault: self.nanos != 0)
    try unknownFields.traverse(visitor: &visitor)
  }

The latter form would likely be similar performance to yours, with similar code size and memory usage to the existing code.

I never really thought about it until looking at this snippet, we fetch the value twice in these calls, what does the compiler do in these cases? Does it optimize it down to a single fetch, are they really two property accesses? Is it worth generating different code to use a local to only fetch the value once? Is that smaller or larger? Faster?

The most interesting aspect of your proposal to me is that it could let us re-implement == as a pure library implementation, without any generated code. That's something that we cannot easily do with the current design, and could be a significant code size win. But that assumes that your proposal doesn't have other costs that we don't yet understand.

I do like this idea, not having to generate equality would be nice.

Having fields is a list also makes it that much more likely to be able to revisit the binary serialization and do it in reverse to remove the sizing pass.

I never really thought about it until looking at this snippet, we fetch the value twice in these calls, what does the compiler do in these cases? Does it optimize it down to a single fetch ... ?

Since all the code here is in the same source file and the getters are pretty trivial, the compiler should routinely inline the getter and the usual compiler optimizations should do the right thing from that point on. If anyone sees otherwise (in Release builds), the compiler team would appreciate a bug report.

Having fields is a list also makes it that much more likely to be able to revisit the binary serialization and do it in reverse to remove the sizing pass.

I recently managed to convince myself that reversing the order of fields is not a problem. So I'm just waiting for some spare time to sit down and actually implement this.

I did some memory testing and the proposed solution appears to use ~1.1x more memory.

Testing strategy:

1. Generated a test.proto file which has 100 messages with 50 random fields each test.proto.zip

2. Create a swift CLI built in release mode that assigns all fields on each message and then encodes them using both binary and json encoding 10000 times

3. Observe the memory usage displayed in xcode

Existing implementation used ~15.3 mb, the proposed implementation used ~16.7 mb

I want to go forward with this, since it gives us a lot of nice things:

• This will let us re-implement == without code generation, significantly reducing code size
• It simplifies Add FieldMask utilities to Message types #1505
• It enables a nice performance improvement in Binary encode from back-to-front #1518

The tricky work will be experimenting with different ways to implement the list of fields to properly balance code size, performance, and memory usage. A 10% growth in memory usage is probably more of a concern than the minor performance loss described above, so I think that's what we should focus the most attention on.

The most interesting aspect of your proposal to me is that it could let us re-implement == as a pure library implementation, without any generated code

I gave this a try last night and it was pretty straightforward to build. The end result was a ~10% binary size reduction in my test app that contained 100 messages with 50 fields each.

Just pushed 2 updates:

1. Figured out how to avoid the any Message syntax by putting static var _fields into _MessageImplementationBase
2. Remove isUnset from message fields

@tbkka Do you have any outstanding feedback for me? Anything I can do to help push this through?

Compared your branch to main using the Perf Harness that's in the tree (after I fixed it up in #1521). That shows a small increase in overall code size along with 50% reduction in binary encoding performance. I'm also surprised to see that JSON and Text encoding show a much smaller absolute slowdown than binary: Binary added about 60µs (42->105) but JSON only increased by 22µs (78->100) and Text 19µs (152->171). I would expect all three to slow down by about the same amount. Hmmm....

What would we need to do to also support decoding with this approach? Minimum, we'd need a way to quickly look up field info from field number and/or name. And we'd need to support setting a field, not just reading it.

Encoding performance is still pretty proportional to the number of fields, so we haven't significantly increased our fixed costs.

@mrabiciu Could you merge main into your branch? (Or rebase, whatever you're happier with right now.). That will make some of my experiments a little simpler.

Hmmm... TestJSONPerformance/testEncoding_manyEnumMapsEncoding_shouldBeUnder1s is almost 4s on my machine right now.

I'm also surprised to see that JSON and Text encoding show a much smaller absolute slowdown than binary ...

Ah. Binary encoding does two traversals -- one to measure the total size and another to actually serialize things -- which explains this. Eliminating this would require a resizable buffer that we could expand as necessary during the regular serialization. I've long thought such expansion would actually be slower than the second traversal, but I'm starting to wonder if that's actually true or not. Hmmm.... That may require some future experimentation.

I'm seeing some retain/release traffic during the traversals. I'm not sure where that's coming from. I'm also see copy and destroy of the Field objects, which might be due to some local changes I'm experimenting with.

That shows a small increase in overall code size

Yes I saw the increase too, however after experimenting with implementing == using Field I saw the bundle size reduced to smaller than whats currently on main

What would we need to do to also support decoding with this approach? Minimum, we'd need a way to quickly look up field info from field number and/or name. And we'd need to support setting a field, not just reading it.

I haven't familiarized myself with decoding yet but I think having _fields be [Int: Field<Self>] along with storing a NameDescription in on Field would be everything needed to make decoding work. This allows us to eliminate _NameMap, further decreasing the code size.

Could you merge main into your branch

Yep I can do that!

TestJSONPerformance/testEncoding_manyEnumMapsEncoding_shouldBeUnder1s is almost 4s on my machine right now.

Do you have any instructions for how I can run and profile this?

What would we need to do to also support decoding with this approach? Minimum, we'd need a way to quickly look up field info from field number and/or name. And we'd need to support setting a field, not just reading it.

I haven't familiarized myself with decoding yet but I think having _fields be [Int: Field<Self>] along with storing a NameDescription in on Field would be everything needed to make decoding work. This allows us to eliminate _NameMap, further decreasing the code size.

I'm still not sure we need to take this all the way to a Dictionary. I'd think most messages have few enough fields, that a linear search (or maybe a binary if the list if over a certain size) wouldn't be that bad (and might be better in performance and memory tradeoffs compared to having to make the Dictionary.

We might be able to provide a option to indicate if the input is expected to be in order. When in order, you could keep a current index, and after each filed, we just walk in order from that index to find the next field. For Binary and TextFormat where they should be well ordered, simply walking in order to the next field is likely going to be much better. It would take a really sparse message with lots of fields for that to be bad.

We also might be able to store some extra flags about the table, say if there are few gaps in the field numbers, we could null pad things, so that we could just index the field list by the field number and make all those lookups O(1).

TestJSONPerformance/testEncoding_manyEnumMapsEncoding_shouldBeUnder1s is almost 4s on my machine right now.

Do you have any instructions for how I can run and profile this?

You can use swift test to run the tests. That has various options to select specific tests, etc. (If that has any problems, try make test first -- the Makefile has some logic to update various pieces before it runs swift test.)

I've been profiling testEncoding_manyEnumMapsEncoding_shouldBeUnder1s and it seems isDefault is behaving slower than I expected. I think this and the retain/release traffic are related. I see the message being tested is backed by a storage class so I think the retain/release are happening when the getters are executed { $0._storage._repeatedMessage }.

I noticed we do withExtendedLifetime in a bunch of places. Does that avoid doing retain/release on the storage class?

... it seems isDefault is behaving slower than I expected. I think this and the retain/release traffic are related.

That seems like a reasonable guess. Accessing data on the storage class does require retaining that class object. withExtendedLifetime might help -- that tells the compiler to consider the object "in use" for at least a particular range of code; the compiler can then retain once at the beginning and release once at the end, avoiding intermediate retains/releases. But that doesn't necessarily avoid retain/release in intermediate functions.

Another approach might be to combine the fetch and default handling so that you make one function call and get both pieces of information at once with one round-trip down to the storage class instead of two.

The functional tests are passing now, which is promising. Failures above seem to only be because this branch has not re-generated the reference files, so the plugin tests are not generating the expected output. Please don't re-generate until we're ready to land this; that creates a huge number of differences that make it a little hard to see the actual code differences.

I'm still not sure we need to take this all the way to a Dictionary.

Good point. Dictionary has the drawback that it's hard to compile it down to static text. Where possible, it would be nice to avoid Dictionary and stick with simple Arrays. A few options:

• If there are few fields, binary search on an array makes a lot of sense.
• If the field numbers form a compact set, we can order the array to provides field number -> field info lookup directly. (This obviously won't work for an item with two fields numbered 1 and 50,000.)
• We could also emit a small dictionary that just mapped field number and/or name to the index into the field array. That would leave the bulk of the data in something that could conceivably be optimized into static text.

I'm still not sure we need to take this all the way to a Dictionary.

Good point. Dictionary has the drawback that it's hard to compile it down to static text. Where possible, it would be nice to avoid Dictionary and stick with simple Arrays. A few options:

• If there are few fields, binary search on an array makes a lot of sense.
• If the field numbers form a compact set, we can order the array to provides field number -> field info lookup directly. (This obviously won't work for an item with two fields numbered 1 and 50,000.)
• We could also emit a small dictionary that just mapped field number and/or name to the index into the field array. That would leave the bulk of the data in something that could conceivably be optimized into static text.

The C++ generation these days use some tables for a bunch of things also, so there might be things to draw them.

One thing I believe they do is have a header on the table, we could model that as a second static, or maybe make the static an enum with two cases: (a) packed array/table (b) sparse array/table. If for (a) any unused field number gets a marker that is is unused, and we can do direct field number to array indexing (minus one since there is no zero field number). For (b), we do the binary search over the list. And if we decided the do the map for field to offset, we could always compute that at runtime the first time we need it.

Haven't thought this through, but I wonder if it would help at all if the visitor could visit the indirect storage class directly when there was one, instead of always going through the façade struct. I'm not sure what that would entail... Hmmm...

(I've experimentally combined my reverse binary encoder with this and it seems to be working pretty well.)

Haven't thought this through, but I wonder if it would help at all if the visitor could visit the indirect storage class directly when there was one, instead of always going through the façade struct. I'm not sure what that would entail... Hmmm...

I've long wondered about making out generated traverse/decode move into the Storage class (when it exists), it means moving the unknowns/extension there also, and then we could skip the need for the extended lifetime things. But if we move to table driven, I'm not sure how that would work out. I guess we could have a default impl for message without Storage that directly uses the table there, but we'd generate a stub for storage that forwards to the Storage class, and provide a public function that that is generate to some Storage protocol and gets the table passed to it? (i think the C++ currently always generates their calls, but they calls are now really short and just call the table based helper passing in the class specific args, so we could do something like that)

Another approach might be to combine the fetch and default handling so that you make one function call and get both pieces of information at once with one round-trip down to the storage class instead of two.

Yeah I had the same idea, I'm in the middle of prototyping that now.

We could also emit a small dictionary that just mapped field number and/or name to the index into the field array. That would leave the bulk of the data in something that could conceivably be optimized into static text.

I was thinking the same thing. The dictionary could also be generated at runtime on first access of _fields rather than being pre-compiled.

Haven't thought this through, but I wonder if it would help at all if the visitor could visit the indirect storage class directly when there was one, instead of always going through the façade struct

I think this could be possible. The Storage class may need to become public in order for us to have public Field<_StorageClass> properties for this to work but we could keep all the fields private which is probably fine?

I've tried all of the above but it did not appear to improve the performance of the tests. I'm still seeing a lot of retain/release traffic despite doing something like this:

struct SomeMessage {
    class _StorageClass {
        var _singularInt: Int = 0
        // ...
        
        static let _fields: [Field<Unmanaged<_StorageClass>>] = [
            .singularInt32({ $0.takeUnretainedValue()._singularInt32 }, fieldNumber: 1), 
            // ...
        ]
    }
    
    func traverse<V: Visitor>(visitor: inout V) throws {
        let unmanagedStorage = Unmanaged.passUnretained(_storage)
        for field in _StorageClass._fields {
            try field.traverse(unmanagedStorage, visitor: visitor)
        }
    }
}

I'm not really sure why since I thought Unmanaged is supposed to stop adding retain/release calls. Unless the retain/release calls are on something else? Is there any good way to debug this and know what exactly is being retained and released?

As a side-note my original goal with this PR was to enable traversal of default and unset fields for the v2 release. Would you be opposed to me opening a second PR where I add a more naive version of default and unset field traversal that uses the existing traverse implementation? I'm happy to continue experimenting with this refactor but I would also like to unblock my project that depends on needing default fields in json.

Would you be opposed to me opening a second PR where I add a more naive version of default and unset field traversal that uses the existing traverse implementation?

Not at all. It's possible we would have objections to the specific implementation, but I'd have to see it first. Regardless, it's always worth discussing incremental improvements -- we don't have to get it all perfect in a single shot. 😁

As a side-note my original goal with this PR was to enable traversal of default and unset fields for the v2 release. Would you be opposed to me opening a second PR where I add a more naive version of default and unset field traversal that uses the existing traverse implementation? I'm happy to continue experimenting with this refactor but I would also like to unblock my project that depends on needing default fields in json.

Do you all use main as is vs. using tagged releases? I'm just making sure I understand how that would "unblock" your project. Since I believe this was only going to be a 2.x feature, and at the moment, we don't have a timetable for making the first 2.x release.

Once my PR is merged my intention is to either pin to the specific commit hash that includes my PR, or if main happens to be unstable at that commit I can just apply my PR as a Bazel patch on top of whatever the latest stable 1.x version is until 2.x is released.

I hope there's still activity on this, I'm working with a server that requires a request to have all of its options filled-out, even if they're values are false or an empty string.

I hope there's still activity on this, I'm working with a server that requires a request to have all of its options filled-out, even if they're values are false or an empty string.

This change is some what larger, and got paused for some issues around it. But it sounds like what you are interested in is in #1526. However, due to how the library has to change, it won't be in a 1.x release when done, you'll likely have to wait for a 2.0 release.