Use heap-based storage only when a Protobuf has a singular transitive recursion #900

ydnar · 2019-09-17T06:25:33Z

Hi there—long time reader, first-time PR.

I think this addresses the issues raised in #733 and broken out into #735 by analyzing if a proto has a message field with a transitive recursion.

I broke this out into two major commits: one that introduces the change to protoc-gen-swift, and a second commit with the updated generated Swift files. There’s a third commit that undoes a change I needed to build + run the tests on my machine using XCode 11 / Swift 5.1.

Following the instructions in the Makefile, all tests including conformance tests, passed.

Is this the right idea?

ydnar · 2019-09-17T17:41:52Z

Subsequent commits (e0c482f and db85e48) restrict the recursive check to singular fields, to mirror the existing behavior which filters out repeated fields and maps.

thomasvl · 2019-09-19T12:41:52Z

Sources/protoc-gen-swift/MessageGenerator.swift

@@ -506,6 +506,23 @@ fileprivate func hasSingleMessageField(descriptor: Descriptor) -> Bool {
  return result
 }

+fileprivate func hasRecursiveSingularField(descriptor: Descriptor, visited: [Descriptor] = []) -> Bool {
+  var visited = visited


If you don't default visited, and make it inout, can you ensure every node is only checked once? i.e. - since it is a copy, the deeper nodes can get visited multiple times because nothing is shared between those sub visits?

@thomasvl I guess it could be a Set<Descriptor>. As it stands, it short-circuits on any recursion, but doesn’t share state between branches. Are you thinking about this as an optimization?

Is there a straightforward way to benchmark protoc for the test protos and compare against master? I guess that would answer the question.

@thomasvl tried inout visited and got 19 additional heap-allocated messages:

https://github.com/ydnar/swift-protobuf/commit/11698e3cf60f434646e0e3a808e689fcac26fd8a

Edit: I believe the additional heap allocations are because of non-intersecting paths:

A > B > C + A > C would match as visited, even though there wasn’t recursion.

I'll have to digest this one a little more, I didn't expect there to be any change in what it decided should use the heap.

@thomasvl thanks for discovering the logic issue. I think you’re right in your example, A won’t need heap storage, but B will.

I had originally coded this without a visited array, just checking if the root message (in this case A) was referenced by any sub-messages. I’ll try that again.

Unrelated: on my machine (2018 MB Air), using Swift 5.1, the package fails to compile because of a missing default statement in a generated switch (generated_swift_names_enum_cases.pb.swift:2605). I have to manually insert it (and accept the inevitable warnings) in order to get the package to build and tests to run.

@thomasvl I revised the logic based on your findings, and the result is several fewer heap allocations: d879e59

Since it is hard to tell from the diffs, what is left using storage, and does it make sense?

The remaining heap allocations look like they makes sense. I’d love to see it stress-tested by someone hitting performance limits with heap allocs like #733.

Solving this optimally is tricky: In @thomasvl's example above, we technically only need storage for one of B, C, and D. You don't need all three.

Some quick digging suggests that the optimal answer requires solving something called the "maximal acyclic subgraph" problem. That sounds like more work than I really want to tackle in this PR.

thomasvl · 2019-09-23T18:15:16Z

Re the compile failure in the generated file, can you open a sep issue for that (we already saw it on linux, but it hadn't hit macOS hosted builds yet). We can tweak the generated files to dodge it, but the underlying problem can happen in a real proto file if it has enough enum cases, so a complete solution for that will take more thought/work.

ydnar · 2019-09-23T18:30:59Z

Thanks. The exact error I’m seeing:

swift-protobuf/Tests/SwiftProtobufTests/generated_swift_names_enum_cases.pb.swift:1749:5: error: the compiler is unable to check that this switch is exhaustive in reasonable time

Does swift have any arguments to extend the timeout for type checking?

Sources/protoc-gen-swift/MessageGenerator.swift

thomasvl · 2019-09-26T21:06:20Z

fyi - #904 opened for the general issue with large enum. #906 landed to avoid the problem with our testing file generated in the tree. So you likely want to sync and rebase these changes off that.

- Attempted fix for #735 - Inspired by #733

- `make regenerate` to rebuild the protos used by the runtime and plugin - `make test-runtime` to verify that the runtime works correctly with the new changes - `make reference` to update the Reference directory - MANUALLY go through `git diff Reference` to verify that the generated Swift changed in the way you expect - `make clean build test` to do a final check Test results: CONFORMANCE SUITE PASSED: 1940 successes, 0 skipped, 0 expected failures, 0 unexpected failures. CONFORMANCE SUITE PASSED: 63 successes, 0 skipped, 0 expected failures, 0 unexpected failures. Fixes #735 and #733.

…age for additional protos

… root message

ydnar · 2019-09-27T16:04:44Z

@thomasvl I rebased off current master (post #904), re-ran make build regenerate test-runtime reference build test. Regenerated code produced no diffs, and all tests passed.

git diff apple/master | grep _StorageClass.defaultInstance | wc
     139     834    8400

thomasvl · 2019-09-27T17:15:47Z

Sources/protoc-gen-swift/MessageGenerator.swift

+    }
+
+    // Skip other visited fields.
+    if (visited[1...].contains { $0 === messageType }) {


Actually, from a performance pov, you don't need this slice. visted[0] was checked in the conditional above, so just check all of visitited and avoid making the temp object, etc.

Ah, got it. I’m used to @golang where slicing is cheap and on the stack (shared storage with original slice).

visited[1...] returns an ArraySlice that shares storage (and indices) with its original array, so it's the same in that regard.

@allevato so is it correct to assume that sub slice doesn’t result in any additional heap allocations?

(I’m trying to understand if I should change it per @thomasvl’s suggestion).

My thought was there is a call to make the slice (which may or may not have an alloc to its internal layout). So that extra call up front (with potential allocs) is all to avoid one extra === check within the actually contains(where:) loop, so it didn't seem like trying to skip the first one was worth it compare to the cost of that one extra compare.

Roger that. Updated in 1dafa90.

thomasvl · 2019-09-27T18:09:07Z

.vscode/settings.json

+{
+  "swiftformat.enable": false,
+  "files.trimTrailingWhitespace": true
+}


On that note, why doesn’t this repo use swiftformat?

Which swiftformat, the new one in the Apple repo? There isn't yet an general style guide the community has adopted, so we haven't bothered to chase one yet (just like different parts of Swift have different formats at the moment).

Great question. We’ve been using the non-Apple swiftformat, not the new swift-format. A blessed standard format is one of the best aspects of working with Go.

ydnar · 2019-10-07T03:01:42Z

Any thoughts?

tbkka

I'll look into this more deeply this week.

Sources/protoc-gen-swift/MessageGenerator.swift

tbkka

I might well have missed something, but I think this is "correct" in the sense that it will never choose non-heap storage when heap storage is required. (Performance and finding a more optimal set of storage choices can be iterated later.)

My only real concern with this is whether the larger structs we're generating will cause problems for some people. The best way to find out is to merge this and solicit feedback.

tbkka · 2019-10-08T19:53:06Z

Sources/protoc-gen-swift/MessageGenerator.swift

@@ -506,6 +506,23 @@ fileprivate func hasSingleMessageField(descriptor: Descriptor) -> Bool {
  return result
 }

+fileprivate func hasRecursiveSingularField(descriptor: Descriptor, visited: [Descriptor] = []) -> Bool {
+  var visited = visited


Solving this optimally is tricky: In @thomasvl's example above, we technically only need storage for one of B, C, and D. You don't need all three.

Some quick digging suggests that the optimal answer requires solving something called the "maximal acyclic subgraph" problem. That sounds like more work than I really want to tackle in this PR.

thomasvl · 2019-10-11T14:32:24Z

My only real concern with this is whether the larger structs we're generating will cause problems for some people. The best way to find out is to merge this and solicit feedback.

I'll go head and merge this now so we can try to get the feedback.

thomasvl · 2019-10-11T14:36:24Z

@ydnar thanks again for the work and patch!

ydnar · 2019-10-11T16:18:51Z

Oh, cool! Thanks @thomasvl @tbkka for the review + feedback!

ydnar changed the title ~~Use heap-based storage when a Protobuf has a transitive recursion~~ Use heap-based storage only when a Protobuf has a transitive recursion Sep 17, 2019

ydnar changed the title ~~Use heap-based storage only when a Protobuf has a transitive recursion~~ Use heap-based storage only when a Protobuf has a singular transitive recursion Sep 17, 2019

tbkka mentioned this pull request Sep 17, 2019

_StorageClass makes large arrays of non-trivial messages impractical. #733

Open

ydnar marked this pull request as ready for review September 17, 2019 18:04

thomasvl reviewed Sep 19, 2019

View reviewed changes

thomasvl reviewed Sep 23, 2019

View reviewed changes

Sources/protoc-gen-swift/MessageGenerator.swift Show resolved Hide resolved

ydnar added 7 commits September 27, 2019 08:22

Sources/protoc-gen-swift: test for transitive recursion

8e22c28

- Attempted fix for #735 - Inspired by #733

protoc-gen-swift: hasRecursiveField -> hasRecursiveSingularField

e6dfe59

Recompile protos with hasRecursiveSingularField; eliminates heap stor…

6757782

…age for additional protos

protoc-gen-swift: fix logic error in recursion check by only checking…

296b55a

… root message

Recompile test and reference protos with fix from deaa284

e2d4cf5

protoc-gen-swift: remove unused hasSingleMessageField function

3439922

thomasvl reviewed Sep 27, 2019

View reviewed changes

protoc-gen-swift: update visited.contains from PR feedback

1dafa90

thomasvl reviewed Sep 27, 2019

View reviewed changes

Remove .vscode directory

6c77dc9

thomasvl requested a review from tbkka September 27, 2019 18:41

thomasvl approved these changes Sep 27, 2019

View reviewed changes

tbkka reviewed Oct 7, 2019

View reviewed changes

Sources/protoc-gen-swift/MessageGenerator.swift Outdated Show resolved Hide resolved

protoc-gen-swift: address PR feeback from @tbkka

5edbfff

tbkka approved these changes Oct 8, 2019

View reviewed changes

thomasvl merged commit b2baf41 into apple:master Oct 11, 2019

ydnar mentioned this pull request Nov 5, 2019

Revisit when to use heap based storage for messages. #735

Open

n8gray mentioned this pull request Aug 7, 2020

EXC_BAD_ACCESS in decodeMessage(decoder:) due to stack overflow #1034

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use heap-based storage only when a Protobuf has a singular transitive recursion #900

Use heap-based storage only when a Protobuf has a singular transitive recursion #900

ydnar commented Sep 17, 2019 •

edited

Loading

ydnar commented Sep 17, 2019

thomasvl Sep 19, 2019

ydnar Sep 21, 2019

ydnar Sep 21, 2019

ydnar Sep 21, 2019 •

edited

Loading

thomasvl Sep 23, 2019

ydnar Sep 23, 2019

ydnar Sep 23, 2019 •

edited

Loading

thomasvl Sep 23, 2019

ydnar Sep 26, 2019

tbkka Oct 8, 2019

thomasvl commented Sep 23, 2019

ydnar commented Sep 23, 2019

thomasvl commented Sep 26, 2019

ydnar commented Sep 27, 2019 •

edited

Loading

thomasvl Sep 27, 2019

ydnar Sep 27, 2019

allevato Sep 27, 2019

ydnar Sep 27, 2019

thomasvl Sep 27, 2019

ydnar Sep 27, 2019

thomasvl Sep 27, 2019

ydnar Sep 27, 2019

ydnar Sep 27, 2019

thomasvl Sep 27, 2019

ydnar Sep 27, 2019

ydnar commented Oct 7, 2019

tbkka left a comment

tbkka left a comment

tbkka Oct 8, 2019

thomasvl commented Oct 11, 2019

thomasvl commented Oct 11, 2019

ydnar commented Oct 11, 2019

Use heap-based storage only when a Protobuf has a singular transitive recursion #900

Use heap-based storage only when a Protobuf has a singular transitive recursion #900

Conversation

ydnar commented Sep 17, 2019 • edited Loading

ydnar commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydnar Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ydnar Sep 23, 2019 • edited Loading

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 Sep 23, 2019

ydnar commented Sep 23, 2019

thomasvl commented Sep 26, 2019

ydnar commented Sep 27, 2019 • edited Loading

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

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

ydnar commented Oct 7, 2019

tbkka left a comment

Choose a reason for hiding this comment

tbkka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasvl commented Oct 11, 2019

thomasvl commented Oct 11, 2019

ydnar commented Oct 11, 2019

ydnar commented Sep 17, 2019 •

edited

Loading

ydnar Sep 21, 2019 •

edited

Loading

ydnar Sep 23, 2019 •

edited

Loading

ydnar commented Sep 27, 2019 •

edited

Loading