Conversation
Member
Author
|
I also re-ran the benchmarks, to make sure the new code movement isn't reducing performance. It turns out, it is not slower -- in fact it's a little faster (see original benchmarks here). However, all of the benchmarks are faster, and protocompile's has improved the least :/ Looking at the details of the protoparse executions, I have to guess that these are due to improvements in the Go compiler/runtime (these benchmarks were done with Go 1.20; the original's used Go 1.18). Even |
befc07f to
7f2f67d
Compare
descriptor.proto
this allows us to remove several now-redundant methods on linker.Result
this also cleans up the way override descriptor.proto files were handled:
- Instead of it being modeled as an implicit dependency and given to
linker (which could then use it to lookup options type), it is now
handled (more appopriately IMO) as an option for the interpreter.
this features numerous interface changes so is not technically backwards
compatible; but all interfaces changed were items that were not intended
to be implemented outside of the repo (and this is still pre-v1.0)
7f2f67d to
47cbde1
Compare
pkwarren
reviewed
Mar 7, 2023
pkwarren
approved these changes
Mar 7, 2023
jhump
added a commit
that referenced
this pull request
Mar 7, 2023
…115) In #109, trying to be clever, I moved the place where we block for an override descriptor.proto to "just in time", before options are interpreted. That seemed like a potential latency improvement to allow compilation of the override descriptor.proto to overlap with the linking of the file in question, if the file in question didn't actually directly depend on descriptor.proto. However, it blocks for the other result while still holding the semaphore! So this opens up the possibility of deadlock 🤦. A possible remedy is to release the semaphore before blocking, and re-acquiring when done. But that seems too complicated/brittle: we already have code that must release/re-acquire the semaphore, so it seems best to just keep that in one place. Which means reverting the "clever" change and just waiting for an override descriptor.proto while waiting for all other imports.
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.
An issue was introduced in #97 -- if a custom
descriptor.protois used, then custom options may come across in the final compilation result as unrecognized fields. Here's a test failure inbuf: link. The first commit in this PR is a repro case.The issue was two-fold: the options in question come from public imports, and this only happens when an override set of options are used. This happens because we have to use a dynamic message to represent the override definition of the options. But the result is ultimately a
descriptorpb.FileDescriptorProto, which needs a reference to the generated type (not a dynamic message). So in this case, we serialize the interpreted options message to bytes and then de-serialize that into the generated type.This one's a little complicated. But the root issue is that we used
linker.ResolverFromFileas if it correctly implemented visibility rules, but it didn't. So when de-serializing the bytes in the above conversion, it failed to recognize the custom option, that was visible to the file via a public import.So now, it does implement visibility rules exactly: so it can resolve any symbol that is visible to the given file. That means any symbol defined in that file or in an import OR in a transitive public import.
We already had logic in here for the visibility rules, which were used for other parts of linking and type lookup. So I've now updated it so that everything runs on the same rails -- e.g. the resolver implementation in
linker.ResolverFromFilecalls into the same code that already correctly implemented the rules.While consolidating, there were suddenly some unused/unnecessary methods. For example, the handful of
Resolve*methods on thelinker.Resultwere now redundant. So I've removed them. There was also an unexported method on thelinker.Filesinterface, which was now unused so I removed it, too. I know these aren't backwards-compatible changes, but this is still pre-v1.0 and these are interfaces which users are highly unlikely to actually implement.While going through this, I also took an opportunity to do some cleanup after #97. Instead of treating an override
descriptor.protoas if it were an implicit dependency used by the linker, it's now an option for the options interpreter (which is the only place that uses it, so seemed more natural).