Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants