-
Notifications
You must be signed in to change notification settings - Fork 783
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
(Eventually) fix #17501 #17668
base: main
Are you sure you want to change the base?
(Eventually) fix #17501 #17668
Conversation
❗ Release notes required
|
It helps compilation. By a lot (130s -> 5s). A bunch of blocking issues I'm not yet sure how to address:
For 1st, we can technically clean the cache on each FSI submission. For 2nd - I am not sure what to do, since we fill in the cache on-demand. Pre-filling it will be costly and will slowdown every single compilation and check. Checking interfaces on any cache hit will neglect this optimization. One of the solutions might be to only enable it for compilation. |
I agree this is not a large change. As for testing this, one idea is to enable this for all compilation scenarios, not just one off. More chance to spot regressions if the whole existing test suite uses this code, not just a subset. |
Can't enable for one reason - we don't have a good solution for the caching in compiler, which has eviction and statistics built in. In the service scenarios, dictionary will just grow forever, since (almost) any change to the type will introduce a new instance of TType and will cache it. I think we build product compiler and some of suites with preview version, so it has some coverage. I also tested it on the original OpenTK solution |
I added a few remarks for myself as I plan to (later) finish this impl. |
I addressed a couple of them. I still think that it's tested enough (as good as we can), plus is hidden under flags (language and tooling), and think that we can merged it as is, it's better to test it this early than try and plan "better" solution. But then again, if there's any pushback, happy to hear a better plan/solution to it. |
Co-authored-by: Tomas Grosup <[email protected]>
Co-authored-by: Tomas Grosup <[email protected]>
The approach I want to take is to cache the results of type subsuming for two given types (in
TypeFeasiblySubsumesType
& co).For now just make code a bit more readable, trying to figure out how should we approach the caching itself, since
TType
itself is not have and equality for it (by design), since we can have non-nominal generic types, where TType_var's solution can be (and is) mutable, and can change with time.Update, perf:
With cache (PR): 5.8s
Without cache (main): 130.9s
On a real project with issue (OpenTK, which uses NET 8 BCL and its types extensively), compiled successfully.
One concern is that there can be a hash collision eventually, which will trip this logic. But I will play with additional value comparisons if we hit the cache.