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

Optimize metadata reading for extension methods #16168

Merged
merged 8 commits into from
Jun 26, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Oct 24, 2023

Currently, finding out whether a type in an IL assembly has Extension attribute requires reading all attributes from the metadata and constructing a representation of ILAttribute for each one. This process is time-consuming and involves unnecessary work. It would be beneficial to have a more efficient method for determining if a type can contain extension methods.

Furthermore, ILTypeDef constructor is a public API but lacks the ability to pass lazy computation of custom attributes. In contrast, the internal constructor in FCS supports this feature.

This PR proposes:

  • The ability to pass lazy computation of custom attributes to ILTypeDef public constructor
  • The ability to specify the presence of Extension attribute in ILTypeDef (and some refactoring around isKnownToBeAttribute flag)
  • Default implementation for lightweight detection of Extension attribute presence

This solution will allow for more efficient filtering of ILTypeDef that definitely does not contain extension methods, both for users of the public API and within the FCS implementation.

@DedSec256 DedSec256 requested a review from a team as a code owner October 24, 2023 15:58
@DedSec256 DedSec256 marked this pull request as draft October 24, 2023 16:11
@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch from ca3f670 to 8e3c947 Compare December 11, 2023 18:00
@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch from 8e3c947 to fc68762 Compare March 5, 2024 21:16
Copy link
Contributor

github-actions bot commented Mar 5, 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/8.0.400.md

@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch from 41d34e1 to d7c77b3 Compare March 6, 2024 21:51
@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch 2 times, most recently from 3d691e3 to 029fd16 Compare June 13, 2024 19:03
@T-Gro
Copy link
Member

T-Gro commented Jun 17, 2024

Still a WIP, or ready for review? I can see CI is passing all the gates now.

@DedSec256
Copy link
Contributor Author

I'm still doing some experiments, and it will be ready for review soon

@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch from 51cbd44 to 9255b94 Compare June 18, 2024 16:41
@DedSec256 DedSec256 changed the title [WIP] Optimize metadata reading for extension methods Optimize metadata reading for extension methods Jun 19, 2024
@DedSec256
Copy link
Contributor Author

@T-Gro, @psfinaki, I think it's ready for review.

Lazy evaluation of attributes (our own implementation inside ReSharper.FSharp) and the presence of extension methods flag allowed us to eliminate unnecessary attribute calculations (from 30 seconds to 1 second), reduce the ILTypeDef creation time by 30 seconds, and the overall analysis time of the F# file inside the Rider monorepository from 70 seconds to 40 seconds.

@DedSec256 DedSec256 marked this pull request as ready for review June 19, 2024 12:21
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.

This needs release notes and a rough estimation how does it affect startup time/runtime and memory consumtion.

@DedSec256 DedSec256 force-pushed the ber.a-metadata-extensions branch from b1bb412 to 860b97a Compare June 21, 2024 01:14
@DedSec256 DedSec256 changed the title Optimize metadata reading for extension methods Optimize metadata reading for extension methods [WIP] Jun 21, 2024
@DedSec256 DedSec256 changed the title Optimize metadata reading for extension methods [WIP] Optimize metadata reading for extension methods Jun 21, 2024
@DedSec256
Copy link
Contributor Author

DedSec256 commented Jun 26, 2024

The main idea of PR is to spend a little more time searching for a possible ExtensionAttribute in typeDefReader, while reducing the number of searches with a full read of all type attributes later.
Theoretically, this does not affect the startup time, increases the time and memory of typeDefReader calls, and reduces the time and memory of IsTyconRefUsedForCSharpStyleExtensionMembers calls.

Benchmarks were conducted for some scenarios

  • For the FCS benchmark, the following code was used to analyze files
[<MemoryDiagnoser>]
type Bench() =
    let mutable files = [||]
    let mutable sources = [||]
    let mutable options = Unchecked.defaultof<_>
    let mutable checker = Unchecked.defaultof<_>

    [<GlobalSetup>]
    member this.Setup() =
        let otherOptions = [| (* options with referenced dlls *) |]
        files <- [| (* files *) |]
        sources <- files |> Array.map (File.ReadAllText >> SourceText.ofString)
        options <- {
            ProjectFileName = // some  .fsproj
            ProjectId = None
            SourceFiles = f
            OtherOptions = otherOptions
            ReferencedProjects = [||]
            IsIncompleteTypeCheckEnvironment = false
            UseScriptResolutionRules = false
            LoadTime = DateTime.UtcNow
            UnresolvedReferences = None
            OriginalLoadReferences = []
            Stamp = None
        }
        checker <- FSharpChecker.Create(projectCacheSize = 200)

    [<IterationSetup>]
    member this.Invalidate() =
        checker.InvalidateAll()
        checker.ClearLanguageServiceRootCachesAndCollectAndFinalizeAllTransients()

    [<Benchmark>]
    member _.TypeCheckFile() =
        for file, source in Seq.zip files sources do
            let _, checkResult =
                checker.ParseAndCheckFileInProject(file, 0, source, options)
                |> Async.RunSynchronously
            match checkResult with
            | FSharpCheckFileAnswer.Succeeded(checkResult) when checkResult.Diagnostics.Length <> 0 -> failwithf $"Errors: %A{checkResult.Diagnostics}"
            | FSharpCheckFileAnswer.Aborted -> failwithf "Aborted"
            | _ -> ()
  • System specification

BenchmarkDotNet v0.13.10, Windows 11 (10.0.22631.3737/23H2/2023Update/SunValley3)
12th Gen Intel Core i9-12900H, 1 CPU, 20 logical and 14 physical cores
.NET SDK 9.0.100-preview.5.24307.3
[Host] : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2 DEBUG
Job-WKOUUV : .NET 8.0.1 (8.0.123.58001), X64 RyuJIT AVX2

InvocationCount=1 UnrollFactor=1

Analyzing a single file in the Giraffe framework

300 dll references required for project analysis were taken

The averaged results of launches look as follows.

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFile - old 587.2 ms 8.64 ms 8.08 ms 2000.0000 1000.0000 1000.0000 851.73 MB
TypeCheckFile - new 581.8 ms 6.09 ms 5.70 ms 2000.0000 1000.0000 1000.0000 819.13 MB

Sequential analysis of all files in src/Giraffe

22 files were taken

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFiles - Giraffe - old 1.185 s 0.0136 s 0.0121 s 3000.0000 2000.0000 1000.0000 1.69 GB
TypeCheckFile - Giraffe - new 1.167 s 0.0078 s 0.0069 s 3000.0000 2000.0000 1000.0000 1.63 GB

Sequential analysis of all files in ReSharper.FSharp/FSharp.Psi.Services

48 files & 471 dll references required for project analysis were taken

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFile - ReSharper - old 5.689 s 0.0472 s 0.0419 s 8000.0000 6000.0000 5000.0000 7.35 GB
TypeCheckFiles - ReSharper - new 5.635 s 0.0553 s 0.0517 s 7000.0000 5000.0000 4000.0000 7.29 GB

Unit-test for ILReading

The benchmark checks for additional expenses when called typeDefReader.
450 assemblies have been taken from the benchmark for ReSharper.FSharp/FSharp.Psi.Services, which contain approximately 1,000,000 types, 287,000 of which have extensions

The code of an existing test from CompilerServiceBenchmarks was rewritten to the following

        for fileName in assemblies do
            let reader = OpenILModuleReader fileName readerOptions

            let ilModuleDef = reader.ILModuleDef

            for typedef in ilModuleDef.TypeDefs.AsArray() do
                typedef.IsKnownToBeAttribute |> ignore
Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ILReading - old 377.4 ms 7.55 ms 11.06 ms 3000.0000 2000.0000 2000.0000 731.9 MB
ILReading - new 485.8 ms 9.57 ms 12.77 ms 3000.0000 2000.0000 2000.0000 749.5 MB

Profiling the ReSharper.FSharp project inside the Rider soluition

In this scenario F# projects refer directly to C# projects in the Rider solution instead of DLLs from its SDK. Based on the C# sources, the metadata required for FCS analysis is built. Lazy evaluation of custom attributes (our own implementation inside ReSharper.FSharp) for ILTypeDef and the presence of the extension methods flag allowed us to eliminate unnecessary forced attribute calculations (from 30 seconds to 1 second), reduce the ILTypeDef creation time by 30 seconds, and the overall analysis time of the F# file inside the Rider solution from 70 seconds to 40 seconds.

Notes and further work

1

On all profiles captured during the investigation, it is clear that a significant amount of time is reduced from IsTyconRefUsedForCSharpStyleExtensionMembers calculation because of CanContainExtensionMethods flag

Before:
image

After:
image

2

Full line-by-line profile for merged typeDefReader calls from sequential analysis of all files in src/Giraffe benchmark looks like
image
The search for the range of attributes for the type adds here 150 ms.

2.1

On all profiles captured during the investigation, it is clear that a significant amount of time is consumed by non-lazy calls to the following two functions in typeDefReader:

image

In a separate PR, we can think about making their computation more lazy. For example, the list of generic parameters was needed inside IsTyconRefUsedForCSharpStyleExtensionMembers, which after adding the CanContainExtensionMethods flag, probably, no longer needs to be computed eagerly

image

Conclusions

  • The benchmarks showed that the analysis time, at the very least, did not increase. Memory consumption has slightly decreased.
  • Profiling showed that the IsTyconRefUsedForCSharpStyleExtensionMembers call time has significantly decreased due to the absence of the need to read custom attributes.
  • The flag CanContainExtensionMembers and the ability to lazily compute attributes in the public API of ILTypeDef can significantly reduce analysis time.
  • The typeDefReader call time has naturally increased due to the search for indexes of custom attributes for each readable type.
  • There is room for optimizations in the typeDefReader for reading interfaces and generic parameters.

With a larger number of types being checked for the presence of ExtensionAttribute, it is likely analysis in NameResolution should result in an overall greater gain in speed.

@DedSec256
Copy link
Contributor Author

Note that statistical optimization for searching MethodDefParent has been added: if the index of the requested method does not fall within the range of methods of the checked type, it is not necessary to call seekReadTypeDefRowExtents (and thus read the indexes of methods of the next type)

image

The same optimization can be applied to fields in a separate PR

seekReadIndexedRow (
ctxt.getNumRows TableNames.TypeDef,
(fun i -> i, seekReadTypeDefRowWithExtents ctxt i),
id,
(fun (_, ((_, _, _, _, fieldsIdx, _), (endFieldsIdx, _))) ->
if endFieldsIdx <= idx then 1
elif fieldsIdx <= idx && idx < endFieldsIdx then 0
else -1),
true,
fst
)

@DedSec256 DedSec256 requested a review from vzarytovskii June 26, 2024 03:09
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.

6 participants