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 members and custom attributes reading #17364

Merged
merged 9 commits into from
Jul 27, 2024

Conversation

DedSec256
Copy link
Contributor

@DedSec256 DedSec256 commented Jun 28, 2024

Now the search for custom attributes and type members in metadata tables is performed as follows:

  • a binary or sequential search is used to find a pivot element in the table, whose key matches the required one (for example, enclosing type/member index)
  • Then, the algorithm searches for a range of suitable elements (having the required key) by scanning both forwards and backwards from the pivot element

Currently,

  • the entire search candidate from the table is being read, even if only the key field needs to be read
  • data is added to the cache even if the key doesn't match the required one

So, during the search, a lot of unnecessary data is read and stored in cache.

This PR suggests an approach used in #16168:

  • if the key is the first field of the candidate being checked, then only the key is read; the remaining fields are not read
  • the whole fields data are read and stored in the cache only for suitable elements while rowConverter function called

Thus, for optimization in this PR, the data for seekReadEvents, seekReadMethodImpls, seekReadProperties, and customAttrsReader are suitable.

In order to verify the optimization, 450 assemblies from the ReSharper.FSharp/FSharp.Psi.Services project were taken and the following benchmark was written:

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

            let ilModuleDef = reader.ILModuleDef

            let ilAssemblyManifest = ilModuleDef.Manifest.Value

            ilAssemblyManifest.CustomAttrs |> ignore
            for x in ilAssemblyManifest.ExportedTypes.AsList() do
                x.CustomAttrs |> ignore

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

                for ilMethodDef in ilTypeDef.Methods.AsArray() do
                    ilMethodDef.CustomAttrs |> ignore

                for ilFieldDef in ilTypeDef.Fields.AsList() do
                    ilFieldDef.CustomAttrs |> ignore

                for ilPropertyDef in ilTypeDef.Properties.AsList() do
                    ilPropertyDef.CustomAttrs |> ignore

                for ilEventDef in ilTypeDef.Events.AsList() do
                    ilEventDef.CustomAttrs |> ignore

                for _ in ilTypeDef.MethodImpls.AsList() do ()

    [<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 5.752 s 0.0843 s 0.0788 s 5000.0000 4000.0000 3000.0000 9.29 GB
ILReading - New 4.331 s 0.0617 s 0.0577 s 4000.0000 3000.0000 3000.0000 3.79 GB

Copy link
Contributor

github-actions bot commented Jun 28, 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 Optimize custom attributes reading [WIP] Optimize metadata members and custom attributes reading [WIP] Jun 28, 2024
@DedSec256 DedSec256 changed the title Optimize metadata members and custom attributes reading [WIP] Optimize metadata members and custom attributes reading Jul 1, 2024
@DedSec256 DedSec256 marked this pull request as ready for review July 1, 2024 20:19
@DedSec256 DedSec256 requested a review from a team as a code owner July 1, 2024 20:19
Copy link
Member

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The benchmarks seem almost too good to be true :)

@KevinRansom, just in case, maybe you have some ideas for extra testing here?

@DedSec256
Copy link
Contributor Author

DedSec256 commented Jul 4, 2024

@psfinaki, some additional explanations for the benchmark:

It forces members/attributes reading to check the total profit that we can achieve in reading. In a full analysis, these same results will be distributed over time due to lazy evaluation.

During the phase of searching for a range of rows in the table, a complete set of fields for each checked row was read and cached, even if only the key was needed.
If the search was performed by binary search, then, for example, in a table of 1000 rows there were approximately 10 searches for the pivot element and, let's say, 2-3 passes back and forth to find the start and end of the range. That is, instead of fully reading and caching 2-3 rows, this was happening for 12-13 rows.
Some tables, for example, PropertyMap, do not support binary search, which means almost complete reading and caching of the table, even if only 2-3 properties are currently needed from there.

From here, there is a reduction in search time, since only the key is read; in memory, since only the rows from the found range are cached, the intermediate ones needed only for search are not cached.

@DedSec256
Copy link
Contributor Author

DedSec256 commented Jul 5, 2024

Similar to benchmark from #16168 (comment), sequential analysis of all files in ReSharper.FSharp/FSharp.Psi.Services with additional unused opens in some files

Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFile - OLD 5.778 s 0.0419 s 0.0392 s 9000.0000 7000.0000 6000.0000 7.4 GB
TypeCheckFile - OLD 5.784 s 0.0514 s 0.0481 s 10000.0000 8000.0000 7000.0000 7.45 GB
Method Mean Error StdDev Gen0 Gen1 Gen2 Allocated
TypeCheckFile - NEW 5.591 s 0.0395 s 0.0369 s 9000.0000 7000.0000 6000.0000 7.36 GB
TypeCheckFile - NEW 5.596 s 0.0393 s 0.0349 s 10000.0000 8000.0000 7000.0000 7.41 GB

Sure, it is possible to find a more illustrative example.

@psfinaki
Copy link
Member

psfinaki commented Jul 8, 2024

@DedSec256 thanks for sharing a full(er) picture. The original benchmarks are still useful information since they show how much space for optimization we have here. Thanks for tackling this bit by bit :)

@vzarytovskii
Copy link
Member

@KevinRansom happy to merge after your approval.

@edgarfgp
Copy link
Contributor

I thought this already merged and I was checking for this in the latest net9 preview.

I know we are all busy but, Please let’s value people’s time.

@vzarytovskii
Copy link
Member

vzarytovskii commented Jul 27, 2024

I thought this already merged and I was checking for this in the latest net9 preview.

I know we are all busy but, Please let’s value people’s time.

We will merge it once we have 3 or more approvals from the team, we literally had everyone on the team on a leave except 2 people for a few weeks.
Change impactful enough to require more thorough review.
IL reading is a tricky part, we often had problems there even after smaller changes.

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@DedSec256 DedSec256 deleted the ber.a/optimizeAttrsReading branch October 8, 2024 03:27
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