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 type interfaces reading from metadata #17382

Merged
merged 10 commits into from
Aug 29, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Jul 4, 2024

Continuation of #17364

As stated in #16168
image

In this PR, reading of interfaces and generic type parameters (also necessary for reading interfaces) is optimized similarly to the above-mentioned #17364 PR.

To verify the optimizations, all readings except for the readings of interfaces and generic parameters have been removed from typeDefReader.
The same benchmark data from #17364 was taken, and the following benchmark was written:

    [<Benchmark>]
    member _.ILReading() =
        for fileName in assemblies do
            let reader = OpenILModuleReader fileName readerOptions

            let ilModuleDef = reader.ILModuleDef

            for ilTypeDef in ilModuleDef.TypeDefs.AsArray() do
                ilTypeDef.Implements |> ignore

    [<IterationCleanup(Target = "ILReading")>]
    member _.ILReadingSetup() =
        // With caching, performance increases an order of magnitude when re-reading an ILModuleReader.
        // Clear it for benchmarking.
        ClearAllILModuleReaderCache()

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

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ILReading - OLD 309.3 ms 6.17 ms 10.13 ms 2000.0000 2000.0000 2000.0000 543.19 MB
ILReading - NEW 288.2 ms 5.62 ms 9.08 ms 2000.0000 2000.0000 2000.0000 507.47 MB

As you can notice, the search phase for the interface range has become faster, as a smaller amount of data is being read. The full reading only occurs when converting the found rows:

image
Profile for the sequential analysis of all files in Giraffe - src/Giraffe

Plans

It is also planned to make the computation of type interfaces lazy in a separate PR, since it is independent optimization/API change.

@DedSec256 DedSec256 requested a review from a team as a code owner July 4, 2024 00:20
Copy link
Contributor

github-actions bot commented Jul 4, 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

@T-Gro T-Gro self-requested a review July 8, 2024 17:15
@edgarfgp
Copy link
Contributor

edgarfgp commented Aug 6, 2024

@DedSec256 Thanks for improving this. Would you be able to fix the conflicts?. I think these performance improvements will make a difference in large projects that relies on interfaces and extension attributes.

@T-Gro
Copy link
Member

T-Gro commented Aug 14, 2024

@DedSec256 : We would be ready the merge this contribution now. Can you please resolve the new conflict Alex?

@T-Gro T-Gro requested a review from KevinRansom August 14, 2024 17:21
@DedSec256 DedSec256 force-pushed the ber.a/optmizeMetadataReading3 branch from 88721e0 to f77e5df Compare August 20, 2024 17:14
@DedSec256
Copy link
Contributor Author

DedSec256 commented Aug 20, 2024

@T-Gro, There are some additional changes here related to reducing the copying of anonymous structures (and tuples creating) in private functions. Can I ask you to take a look yet another time?

@DedSec256
Copy link
Contributor Author

Looks flaky 🤔

image

@psfinaki
Copy link
Member

/azp run

@psfinaki psfinaki requested a review from T-Gro August 21, 2024 12:15
@psfinaki
Copy link
Member

@DedSec256 before merging this - could you please update the benchmark numbers, just to make sure that they still hold after all this changed merged meanwhile?

@DedSec256
Copy link
Contributor Author

@psfinaki, of course, I will try to do it soon.

@DedSec256
Copy link
Contributor Author

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
ILReading - Interfaces - Old (after nullnes merge) 329.5 ms 6.47 ms 13.51 ms 8000.0000 7000.0000 4000.0000 504.71 MB
ILReading - Interfaces - New 312.6 ms 6.23 ms 11.08 ms 8000.0000 7000.0000 4000.0000 501.87 MB

@psfinaki
Copy link
Member

Thanks @DedSec256 :)

@psfinaki
Copy link
Member

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki psfinaki enabled auto-merge (squash) August 28, 2024 17:06
@DedSec256
Copy link
Contributor Author

/azp run

Copy link

Commenter does not have sufficient privileges for PR 17382 in repo dotnet/fsharp

@DedSec256 DedSec256 closed this Aug 28, 2024
auto-merge was automatically disabled August 28, 2024 21:11

Pull request was closed

@DedSec256 DedSec256 reopened this Aug 28, 2024
@DedSec256 DedSec256 closed this Aug 29, 2024
@DedSec256 DedSec256 reopened this Aug 29, 2024
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.

5 participants