fsharp icon indicating copy to clipboard operation
fsharp copied to clipboard

Make ILTypeDef interface calculation lazy

Open DedSec256 opened this issue 1 year ago • 6 comments

As stated in https://github.com/dotnet/fsharp/pull/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

DedSec256 avatar Jul 08 '24 20:07 DedSec256

:heavy_exclamation_mark: Release notes required


:white_check_mark: 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

github-actions[bot] avatar Jul 08 '24 20:07 github-actions[bot]

It appears that important conflicting changes have occurred during this time

DedSec256 avatar Jul 18 '24 20:07 DedSec256

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

psfinaki avatar Jul 22 '24 12:07 psfinaki

@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.

T-Gro avatar Aug 15 '24 08:08 T-Gro

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

DedSec256 avatar Aug 20 '24 20:08 DedSec256

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).

DedSec256 avatar Aug 21 '24 16:08 DedSec256

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

vzarytovskii avatar Aug 30 '24 13:08 vzarytovskii

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

DedSec256 avatar Sep 05 '24 17:09 DedSec256

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

psfinaki avatar Sep 09 '24 15:09 psfinaki

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

T-Gro avatar Sep 17 '24 11:09 T-Gro

Thanks @DedSec256!

psfinaki avatar Sep 18 '24 10:09 psfinaki