Skip to content

Enable TypeSubsumptionCache for IDE use #18499

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

Merged
merged 35 commits into from
May 1, 2025
Merged

Conversation

majocha
Copy link
Contributor

@majocha majocha commented Apr 24, 2025

Description

This builds upon Vlad's work from #18190 and #17668, extending the fix for #17501 into incremental compilation / typecheck in IDE editor.

This is resubmitted and cleaned up #18468.

Currently the feature is enabled only for lang version preview.
As for testing, this is mostly tested by dogfooding. This repo builds fine and the tests that do use preview version pass.
I also tested manually the scenario from original issue. It is fixed now also in the IDE.

Added some basic cache tests as a starting point, too.

Performance considerations:
The goals here are:

  • do keep the performance gain from original fix
  • do not degrade performance in typical scenarios, including incremental and stand-alone compilation.

Copy link
Contributor

github-actions bot commented Apr 24, 2025

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@majocha majocha changed the title Enable TypeSubSumptionCache in IDE use Enable TypeSubsumptionCache for IDE use Apr 24, 2025
majocha added 2 commits April 25, 2025 14:08
deal with memory restrictions in CI, dispose caches
Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's great! Thanks a tonne for taking over it and making it actually work!

@majocha
Copy link
Contributor Author

majocha commented Apr 29, 2025

@T-Gro I went with your suggestion of MailboxProcessor, it made the code way better. I just need to test it a bit.

@majocha
Copy link
Contributor Author

majocha commented Apr 29, 2025

Quick smoke test to make sure everything works as before in the IDE:
image
Looks good, no failures and no capacity overruns.

@majocha majocha closed this Apr 29, 2025
@majocha majocha reopened this Apr 29, 2025
@majocha
Copy link
Contributor Author

majocha commented Apr 29, 2025

Alas with MailboxProcessor this seems less efficient. Some resource is getting exhausted in the already meager Linux CI (memory? threadpool? waithandles? I have no idea) and it finally gives up.I'll try to optimize this somehow because I really like the Mailbox approach.

@T-Gro
Copy link
Member

T-Gro commented Apr 29, 2025

Failing after 79m on the Linux CI is strange, might be queue or machine specific.
I would not expect MacOS to be that MUCH different and it finished just fine in 44 minutes.

=> can be environmental flakiness

@majocha
Copy link
Contributor Author

majocha commented Apr 30, 2025

I got it to pass with #18527

@T-Gro T-Gro merged commit 05500b8 into dotnet:main May 1, 2025
33 checks passed
@T-Gro
Copy link
Member

T-Gro commented May 1, 2025

Excellent job @majocha , I am really glad we are getting this contribution in and the compiler performance regression (since introduction of the many interfaces for numeric primitives) for especially numeric codebases can be fixed.

@majocha
Copy link
Contributor Author

majocha commented May 1, 2025

I spotted a bug in the eviction async. No big deal I'll post a fix later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants