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 interface calculation lazy #17392

Merged
merged 9 commits into from
Sep 18, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Jul 8, 2024

As stated in #16168
image

By analogy with custom attributes, reading interfaces is really time-consuming and ILTypeDef constructor is a public API but lacks the ability to pass lazy computation of interfaces.

This PR proposes the ability to pass lazy computation of interfaces to ILTypeDef constructor.

As evidence of the need for lazy calculation even for FCS default interfaces reading implementation, a benchmark was conducted for analysis of all files in ReSharper.FSharp/FSharp.Psi.Services with some additional unused opens,
48 files & 471 dll references required for project analysis were taken

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFiles - OLD 5.758 s 0.0510 s 0.0477 s 10000.0000 8000.0000 7000.0000 7.45 GB
TypeCheckFiles - NEW 5.302 s 0.0335 s 0.0332 s 4000.0000 2000.0000 1000.0000 7.02 GB

Copy link
Contributor

github-actions bot commented Jul 8, 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.100.md

@DedSec256 DedSec256 changed the title Make ILTypeDef interface calculation lazy [WIP] Make ILTypeDef interface calculation lazy Jul 18, 2024
@DedSec256 DedSec256 marked this pull request as ready for review July 18, 2024 17:30
@DedSec256 DedSec256 requested a review from a team as a code owner July 18, 2024 17:30
@DedSec256
Copy link
Contributor Author

It appears that important conflicting changes have occurred during this time

@psfinaki
Copy link
Member

@DedSec256 yes, likely due to nullness finally merged, sorry. Let us know if you need any help with conflicts.

@T-Gro T-Gro self-assigned this Aug 14, 2024
@T-Gro
Copy link
Member

T-Gro commented Aug 15, 2024

@DedSec256 : Interface implementation can have attributes on it (not expressible in F#/C#, but in IL it is) and C# nullness metadata export makes use of it.

The nullness PR started to read it together with interface implementations, which will likely be the biggest conflict - let me know if you need any help with that.

Otherwise, once conflicts and CI are resolved, this should be good to be merged.

@DedSec256
Copy link
Contributor Author

This PR probably requires changes to the API, so some work is still in progress.

@DedSec256 DedSec256 force-pushed the ber.a/lazyInterfaces branch from 633bed8 to 161af55 Compare August 21, 2024 15:59
@DedSec256
Copy link
Contributor Author

In this PR, the interface implementations and their custom attributes have been combined into a single type InterfaceImpl, which should simplify both the usage of the API inside the compiler and its passing to the ILTypeDef constructor. Additionally, the type of the Implements property in ILTypeDef has been changed to InterruptibleLazy, as there are scenarios where it is necessary to check is metadata up to date, for which it is necessary to check whether the data has been computed or not (and there are already places in the AbstractIL API that expose lazy containers).

@vzarytovskii vzarytovskii marked this pull request as draft August 30, 2024 13:19
@vzarytovskii
Copy link
Member

Converted to draft for now, lemme know if you need any help resolving issues/testing.

@DedSec256 DedSec256 force-pushed the ber.a/lazyInterfaces branch from b08e2ca to 5214c00 Compare September 5, 2024 16:23
@DedSec256
Copy link
Contributor Author

Benchmarks update

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFiles - Old (latest main) 6.212 s 0.0654 s 0.0611 s 38000.0000 21000.0000 12000.0000 7.3 GB
TypeCheckFiles - New 5.577 s 0.0376 s 0.0333 s 31000.0000 14000.0000 5000.0000 6.74 GB

@psfinaki
Copy link
Member

psfinaki commented Sep 9, 2024

Is this ready now @DedSec256? Nevermind one failing test, it's failing everywhere now.

@T-Gro
Copy link
Member

T-Gro commented Sep 17, 2024

Alex @DedSec256 , can I move it out of draft now that all is passing?

@DedSec256 DedSec256 marked this pull request as ready for review September 17, 2024 16:03
@psfinaki
Copy link
Member

Thanks @DedSec256!

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