bpart: Don't invalidate for changes that inference can't see #57616
+187
−41
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
On master, when we invalidate a CodeInstance, we also invalidate the entire associated MethodInstance. However, this is highly problematic, because we have a lot of CodeInstances that are associated with `getproperty(::Module, ::Symbol)` through constant propagation. If one of these CodeInstances gets invalidated (e.g. because the resolution of const-propagated M.s binding changed), it would invalidate essentially the entire world. Prevent this by re-checking the forward edges list to make sure that the code instance we're invalidating is actually in there.
This addresses one of the last remaining TODOs of the binding partition work by performing invalidations when bindings transition from being undefined to being defined. This in particular finally addresses the performance issue that #54733 was intended to address (the issue was closed when we merged the mechanism, but it had so far been turned off). Turning on the invalidations themselves were always easy (a one line deletion). What is harder is making sure that the additional invalidations don't take extra time. To this end, we add two additional flags, one on Bindings, and one on methods. The flag on bindings tells us whether any method scan has so far found an implicit (not tracked in ->backedges) reference to this binding in any method body. The insight here is that most undefined bindings will not have been referenced previously (because they did not exist), so with a simple one bit saturating counter of the number of edges that would exist (if we did store them), we can fast-path the invalidation. However, this is not quite sufficient, as people often do things like: ``` foo() = bar() bar() = ... ... ``` which, without further improvements would incur an invalidation upon the definition of `bar`. The second insight (and what the flag on `Method` is for) is that we don't actually need to scan the method body until there is something to invalidate (i.e. until some `CodeInstance` has been created for the method). By defering the scanning until the first time that inference accesses the lowered code (with a flag to only do it once), we can easily avoid invalidation in the above scenario (while still invalidating if `foo()` was called before the definition of `bar`). As a further bonus, this also speeds up bootstrap by about 20% (putting us about back to where we used to be before the full bpart change) by skipping unnecessary invalidations even for non-guard transitions. Finally, this does not yet turn on inference's ability to infer guard partitions as `Union{}`. The reason for this is that such partitions can be replaced by backdated constants without invalidation. However, as soon as we remove the backdated const mechanism, this PR will allow us to turn on that change, further speeding up inference (by cutting off inference on branches known to error due to missing bindings).
This implements the optimizations I promised in #57602 by checking in invalidation whether or not the information that inference sees changes. The primary situation in which this is useful is avoiding an invalidation for `export` flag changes or changes of which module a binding is being imported from that do not change what the actual binding being imported is. Base itself uses these patterns sparingly, so the bootstrap impact over #57615 is minimal (though non-zero).
|
Updated version merged in #57625. |
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.
This implements the optimizations I promised in #57602 by checking in invalidation whether or not the information that inference sees changes. The primary situation in which this is useful is avoiding an invalidation for
exportflag changes or changes of which module a binding is being imported from that do not change what the actual binding being imported is. Base itself uses these patterns sparingly, so the bootstrap impact over #57615 is minimal (though non-zero).