Skip to content
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

Fix #17501 #17668

Merged
merged 51 commits into from
Oct 31, 2024
Merged

Fix #17501 #17668

merged 51 commits into from
Oct 31, 2024

Conversation

vzarytovskii
Copy link
Member

@vzarytovskii vzarytovskii commented Sep 6, 2024

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.

Copy link
Contributor

github-actions bot commented Sep 6, 2024

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/9.0.200.md
LanguageFeatures.fsi docs/release-notes/.Language/preview.md

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Sep 19, 2024

It helps compilation. By a lot (130s -> 5s).

A bunch of blocking issues I'm not yet sure how to address:

  1. If in interactive session, we use shared amap, thus there will potentially be a lot of false-positive cache hits.
  2. In tooling scenario, we potentially have a lot of false-positive cache hits:

    type A implements interface IA, we cache it.
    interface IA is removed from the type A, it's still in the cache.
    Any check will pass for "is A an IA".

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.

@T-Gro
Copy link
Member

T-Gro commented Sep 30, 2024

I added a few remarks for myself as I plan to (later) finish this impl.

@vzarytovskii
Copy link
Member Author

vzarytovskii commented Sep 30, 2024

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.

@vzarytovskii vzarytovskii reopened this Sep 30, 2024
@dotnet dotnet deleted a comment from github-actions bot Oct 29, 2024
@vzarytovskii
Copy link
Member Author

This is ready @dotnet/fsharp-team-msft

@T-Gro T-Gro merged commit b34e992 into dotnet:main Oct 31, 2024
32 checks passed
@vzarytovskii vzarytovskii changed the title (Eventually) fix #17501 Fix #17501 Nov 2, 2024
@gusty
Copy link
Contributor

gusty commented Nov 7, 2024

It would be interesting to see if there's any perf gain when comparing to .net6/.net7 ?
Or does the optimized code not even exists there?

@vzarytovskii
Copy link
Member Author

It would be interesting to see if there's any perf gain when comparing to .net6/.net7 ?

Or does the optimized code not even exists there?

Will generally be minimal, because bcl there didn't have such large type hierarchies.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

7 participants