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

Make ILTypeDef super type calculation lazy #18005

Merged
merged 11 commits into from
Dec 13, 2024
Merged

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Nov 14, 2024

By analogy with issues from previous PRs (#17392, #16168) which added ways to pass lazy calculation of custom attributes and interfaces, ILTypeDef constructor (public API) lacks the ability to pass lazy computation of a base type. Right now, it is necessary to read the base type when creating ILTypeDef only to calculate ILTypeDefKind using the name of the base type, which seems redundant.

Therefore, this PR proposes:

  • Ability to pass lazy computation of a base type to ILTypeDef constructor
  • Logic to compute ILTypeDefKind using lightweight base type name lookup without fully reading base type
  • Additional constructor for ILTypeDef that does not require lazy evaluation and automatically evaluates ILTypeDefKind
  • Combining ILTypeDefKind with ILTypeDefAdditionalFlags into a single enum (in future prs, I will try to combine it with TypeAttributes and get rid of another redundant field in ILTypeDef)

Changes from this PR also show a slight performance increase in benchmarks (similar analysis of the ReSharper.FSharp.Psi.Services project as in previous PRs), although this is not the main reason for creating this PR

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFile - Before 6.946 s 0.0854 s 0.0799 s 32000.0000 14000.0000 5000.0000 6.88 GB
TypeCheckFile - After 6.747 s 0.0693 s 0.0648 s 31000.0000 13000.0000 5000.0000 6.86 GB

Copy link
Contributor

github-actions bot commented Nov 14, 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

@DedSec256 DedSec256 force-pushed the ber.a/lazyExtends branch 3 times, most recently from 05e897a to d08555d Compare November 19, 2024 23:03
@DedSec256 DedSec256 marked this pull request as ready for review November 20, 2024 19:22
@DedSec256 DedSec256 requested a review from a team as a code owner November 20, 2024 19:22
@T-Gro T-Gro self-assigned this Dec 3, 2024
src/Compiler/AbstractIL/il.fs Outdated Show resolved Hide resolved
Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

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

The deletion of ILTypeDefKind caused quite change to the compilerservice surface area. I presume compilerservice customers won't feel the pain to badly. I wonder if we should consider locking it down to no breaking changes. However, that is probably a discussion for another time.

@auduchinok
Copy link
Member

The deletion of ILTypeDefKind caused quite change to the compilerservice surface area. I presume compilerservice customers won't feel the pain to badly.

@KevinRansom Thanks for bringing this up, it's indeed important that we make things easier to FCS users. As it has been over the last few years, usually the changes improve things enough to justify the costs of updating things 🙂

AFAIK, the only FCS client that currently works with these IL types directly is Rider, and this change improves the things for us, so we're happy to update the changed types usages.

@T-Gro T-Gro enabled auto-merge (squash) December 13, 2024 13:51
@T-Gro T-Gro merged commit 3750b44 into dotnet:main Dec 13, 2024
33 checks passed
@DedSec256 DedSec256 deleted the ber.a/lazyExtends branch December 13, 2024 15:00
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.

4 participants