Conversation
robbertvanginkel
approved these changes
Dec 7, 2022
Contributor
robbertvanginkel
left a comment
There was a problem hiding this comment.
Very nice, those perf numbers look great! I can definitely see how this will be easier to extend, reads better now!
Small comment on the TestMain, but other than that looks good.
| fileName := file.Path() | ||
| fileDescriptorProto := file.Proto() | ||
| index.Files[fileName] = fileDescriptorProto | ||
| err := walk.DescriptorProtos(fileDescriptorProto, func(name protoreflect.FullName, msg proto.Message) error { |
Monirul1
pushed a commit
to Monirul1/buf
that referenced
this pull request
Apr 30, 2023
This adds a performance benchmark for the image filtering logic, to make sure the changes actually improve performance. Performance is improved in two key ways: 1. Shedding the `protosource` model wrappers and instead directly using descriptor protos. 2. Accumulating results in one place, instead of repeated allocation and then concatenation of slices. (This is also useful pre-factoring for adding more improvements in the future.) The net result is that it's now 1.7x as fast, uses 60% as much memory, and performs less than 1/10th as many allocations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This addresses a TODO about using the
protosourcemodel. The assumption in that TODO was that it was heavy-weight.So this PR first adds a benchmark (see first commit), to evaluate if the change actually improves performance.
The second commit is the change that addresses the TODO (which did have an impact on performance). It adds more information to the
imageIndexso that the descriptor protos can be used, instead of having to first wrap the whole hierarchy inprotosourcetypes.The third commit is another change that IMO makes the code easier to read, but also improves performance a little further by keeping accumulator variables in a new
transitiveClosurestruct, instead of each recursive call having to repeatedly concatenate slices. This is also a pre-factor for addressing other TODOs, which will require tracking new state, which is much easier to add with this new approach, instead of having to change the return values and then update the accumulation logic at many recursive call sites. It also removes some redundant tracking: we were tracking the visited elements in both the returneddescriptorAndDependentstype and in theseenmap. Now it's just a map in thetransitiveClosurestruct, and imports are tracked differently.Benchmark results at the start (i.e. for the current implementation on
main):After the first commit (to just use descriptor protos instead of
protosource):And after introducing the
transitiveClosureaccumulator type:So in the end, it's 1.7x as fast, uses 60% as much memory, with less than 1/10th as many allocations.