Dyno: improve tracking of POI candidates + caching #28244
Conversation
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
A lot of serialization packages relied on the internal detail that the IO module `private use`d `Reflection`. Specifically, they used `canResolveMethod` and `canResolveTypeMethod` without importing/using them, relying on POI w/the IO module. This is fine, but feels brittle. In this PR, bring the required methods in directly. After writing some compiler scripts to identify these caes, I've also observed similar issues with `LinearAlgebra` and `Syncvar`, where I fixed them as well. Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Signed-off-by: Danila Fedorin <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Depends on #28243 only for convenience.
In investigating the bug fixed by #28226, I noticed that some of our module code relied on POI where it probably didn't intend to do so (or at least, where it seemed brittle). I adjusted the compiler to identify such cases, and found a number of generic functions that claimed to use POI that weren't actually using POI.
It turned out that Dyno was considering methods found via forwarding to be "POI". Besides being inaccurate, this tainted the resulting
PoiInfoof the resolved function, thereby reducing its eligibility for being used in caching. E.g., any method that (transitively) relied on DSI methods (e.g., creating array literals, copying arrays, array indexing) was marked as "non-cache-eligible" because it "relied on POI" (even though it just used forwarding to the array instance).It's not clear to me how impactful this issue was, since many calls remain uncacheable after this PR because they contain recursion. However, I did confirm that some uncacheable calls became cacheable after this PR. Thus, we are bound to see an improvement, if a tiny one.
Testing
--dyno-resolve-only