Combine BindingGraphFactory.Resolver's caching logic into a single location.
#4387
Conversation
This file contains 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
![copybara-service[bot]](https://avatars.githubusercontent.com/in/44061?s=60&v=4){kind=small}
…location. Currently, there are two places where we try to cache a binding: 1. `Resolver#resolve(Key)` checks if there's a previously resolved binding for the given key in an ancestor resolver. 2. `Resolver#lookUpBindings(Key)` checks if a resolved binding should be owned by an ancestor component (e.g. because its scoped). Note that these two checks are slightly different. Case 1 only depends on the `Key` and is iteration-order dependent since it depends on whether the key has already been resolved in a ancestor binding. Case 2 depends on the binding rather than just the key (which is why it occurs at the end of `lookUpBindings`) and tries to determine if a binding could be resolved in a parent component (e.g. based on the scope or which component installs the module). Thus, case 2 may cache bindings that case 1 doesn't in cases where we know a binding is resolvable in an ancestor but hasn't yet been resolved (e.g. due to order or just because nothing actually requests it). In any case, this CL consolidates that logic into a single place. This should make it easier to extract the caching logic later from the intermediate graph creation. RELNOTES=N/A PiperOrigin-RevId: 661363402
copybara-service
bot
force-pushed
the
test_644086367
branch
from
efa3017 to
86fcef5
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Combine
BindingGraphFactory.Resolver's caching logic into a single location.Currently, there are two places where we try to cache a binding:
Resolver#resolve(Key)checks if there's a previously resolved binding for the given key in an ancestor resolver.Resolver#lookUpBindings(Key)checks if a resolved binding should be owned by an ancestor component (e.g. because its scoped).Note that these two checks are slightly different. Case 1 only depends on the
Keyand is iteration-order dependent since it depends on whether the key has already been resolved in a ancestor binding. Case 2 depends on the binding rather than just the key (which is why it occurs at the end oflookUpBindings) and tries to determine if a binding could be resolved in a parent component (e.g. based on the scope or which component installs the module). Thus, case 2 may cache bindings that case 1 doesn't in cases where we know a binding is resolvable in an ancestor but hasn't yet been resolved (e.g. due to order or just because nothing actually requests it).In any case, this CL consolidates that logic into a single place. This should make it easier to extract the caching logic later from the intermediate graph creation.
RELNOTES=N/A