Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix for #21456 (Regressions in attribute allocations for non-generic attributes) #21462

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

NickCraver
Copy link
Member

This is a quick-fix for #21456 where regressions between netcoreapp2.1 and netcoreapp3.0 were discovered on most attribute pathways due to the allocation overhead in the generic-supporting pathways. This was introduced in #9189 (thanks @jkotas for the quick find). The workaround here is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize RuntimeModule.ResolveMethod further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.

Benchmarks are available in dotnet/performance - run via:

C:\git\dotnet\performance\src\benchmarks\micro

dotnet run -c Release -f netcoreapp3.0 -- --runtimes netcoreapp3.0 --filter System.Reflection.Attributes.GetCustom*
dotnet run -c Release -f netcoreapp3.0 -- --filter System.Reflection.Attributes.GetCustom* --coreRun "C:\git\dotnet\coreclr\bin\tests\Windows_NT.x64.Release\Tests\Core_Root\CoreRun.exe"

Before (netcoreapp3.0 preview 1 - same as master)

BenchmarkDotNet=v0.11.3.886-nightly, OS=Windows 10.0.17763.134 (1809/October2018Update/Redstone5)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview-009820
  [Host]     : .NET Core 3.0.0-preview-27205-02 (CoreCLR 4.6.27204.02, CoreFX 4.7.18.60501), 64bit RyuJIT
  Job-JKCWXL : .NET Core 3.0.0-preview-27205-02 (CoreCLR 4.6.27204.02, CoreFX 4.7.18.60501), 64bit RyuJIT

Runtime=Core  Toolchain=netcoreapp3.0  IterationTime=250.0000 ms
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1
Method Mean Error StdDev Median Min Max Gen 0/1k Op Allocated Memory/Op
'GetCustomAttributes - Class: Hit (inherit)' 1,884.2 ns 33.686 ns 31.510 ns 1,877.4 ns 1,832.9 ns 1,948.8 ns 0.1204 384 B
'GetCustomAttributes - Class: Miss (inherit)' 463.8 ns 8.109 ns 7.585 ns 464.2 ns 450.6 ns 479.0 ns - -
'GetCustomAttributes - Class: Hit (no inherit)' 1,713.1 ns 17.682 ns 16.540 ns 1,706.6 ns 1,689.5 ns 1,738.3 ns 0.1211 384 B
'GetCustomAttributes - Class: Miss (no inherit)' 268.4 ns 3.372 ns 2.989 ns 268.3 ns 263.0 ns 272.5 ns - -
'GetCustomAttributes - Method Override: Hit (inherit)' 2,909.5 ns 56.034 ns 62.282 ns 2,899.0 ns 2,808.9 ns 3,019.5 ns 0.2057 656 B
'GetCustomAttributes - Method Override: Miss (inherit)' 1,796.4 ns 22.963 ns 21.480 ns 1,796.0 ns 1,751.6 ns 1,828.6 ns 0.1199 384 B
'GetCustomAttributes - Method Override: Hit (no inherit)' 1,765.0 ns 32.926 ns 32.338 ns 1,766.1 ns 1,710.7 ns 1,816.7 ns 0.1270 400 B
'GetCustomAttributes - Method Override: Miss (no inherit)' 1,726.6 ns 29.423 ns 27.522 ns 1,726.3 ns 1,675.9 ns 1,764.2 ns 0.1208 384 B
'GetCustomAttributes - Method Base: Hit (inherit)' 1,796.3 ns 37.857 ns 42.078 ns 1,794.1 ns 1,728.2 ns 1,900.2 ns 0.1180 384 B
'GetCustomAttributes - Method Base: Miss (inherit)' 311.5 ns 7.982 ns 7.466 ns 311.6 ns 302.5 ns 331.0 ns - -
'GetCustomAttributes - Method Base: Hit (no inherit)' 1,738.7 ns 31.928 ns 35.488 ns 1,732.8 ns 1,671.6 ns 1,819.6 ns 0.1163 384 B
'GetCustomAttributes - Method Base: Miss (no inherit)' 275.5 ns 3.779 ns 3.535 ns 276.0 ns 269.5 ns 280.3 ns - -

After

BenchmarkDotNet=v0.11.3.886-nightly, OS=Windows 10.0.17763.134 (1809/October2018Update/Redstone5)
Intel Core i7-7700HQ CPU 2.80GHz (Kaby Lake), 1 CPU, 8 logical and 4 physical cores
.NET Core SDK=3.0.100-preview-009820
  [Host]     : .NET Core 3.0.0-preview-27205-02 (CoreCLR 4.6.27204.02, CoreFX 4.7.18.60501), 64bit RyuJIT
  Job-FZOPMH : .NET Core ? (CoreCLR 4.6.27207.0, CoreFX 4.7.18.60402), 64bit RyuJIT

Runtime=Core  Toolchain=CoreRun  IterationTime=250.0000 ms
MaxIterationCount=20  MinIterationCount=15  WarmupCount=1
Method Mean Error StdDev Median Min Max Gen 0/1k Op Allocated Memory/Op
'GetCustomAttributes - Class: Hit (inherit)' 1,866.7 ns 36.295 ns 40.341 ns 1,860.8 ns 1,808.5 ns 1,971.1 ns 0.0656 224 B
'GetCustomAttributes - Class: Miss (inherit)' 468.7 ns 6.401 ns 5.988 ns 466.8 ns 459.8 ns 479.3 ns - -
'GetCustomAttributes - Class: Hit (no inherit)' 1,623.1 ns 31.073 ns 34.537 ns 1,608.3 ns 1,576.1 ns 1,698.8 ns 0.0652 224 B
'GetCustomAttributes - Class: Miss (no inherit)' 291.9 ns 5.004 ns 3.907 ns 292.5 ns 285.0 ns 298.0 ns - -
'GetCustomAttributes - Method Override: Hit (inherit)' 2,875.5 ns 52.543 ns 46.578 ns 2,878.0 ns 2,781.0 ns 2,947.3 ns 0.1479 496 B
'GetCustomAttributes - Method Override: Miss (inherit)' 1,713.2 ns 21.842 ns 20.431 ns 1,718.2 ns 1,676.8 ns 1,745.0 ns 0.0683 224 B
'GetCustomAttributes - Method Override: Hit (no inherit)' 1,669.4 ns 28.507 ns 26.665 ns 1,671.7 ns 1,628.2 ns 1,715.3 ns 0.0723 240 B
'GetCustomAttributes - Method Override: Miss (no inherit)' 1,682.3 ns 27.461 ns 25.687 ns 1,677.2 ns 1,647.7 ns 1,741.5 ns 0.0659 224 B
'GetCustomAttributes - Method Base: Hit (inherit)' 1,729.9 ns 33.490 ns 34.392 ns 1,728.0 ns 1,679.2 ns 1,787.5 ns 0.0693 224 B
'GetCustomAttributes - Method Base: Miss (inherit)' 325.6 ns 4.064 ns 3.802 ns 326.5 ns 317.6 ns 333.0 ns - -
'GetCustomAttributes - Method Base: Hit (no inherit)' 1,665.2 ns 32.926 ns 30.799 ns 1,666.3 ns 1,616.3 ns 1,716.5 ns 0.0659 224 B
'GetCustomAttributes - Method Base: Miss (no inherit)' 285.1 ns 5.131 ns 4.549 ns 285.0 ns 278.3 ns 293.5 ns - -

cc @jkotas @AviAvni @stephentoub

…only generic attributes

This is a trivial quick-fix for #21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

Copy link

@AviAvni AviAvni left a comment

Choose a reason for hiding this comment

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

Thanks

@adamsitnik adamsitnik merged commit 130cec3 into dotnet:master Dec 10, 2018
@adamsitnik
Copy link
Member

@NickCraver thank you!

morganbr pushed a commit that referenced this pull request Dec 14, 2018
…attributes) (#21462)

* Fix for #21456 - restrict increased generic attribute allocations to only generic attributes

This is a trivial quick-fix for #21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.

* Update comments
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…r non-generic attributes) (dotnet/coreclr#21462)

* Fix for dotnet/coreclr#21456 - restrict increased generic attribute allocations to only generic attributes

This is a trivial quick-fix for dotnet/coreclr#21456 where regressions between 2.1 and 3.0 were discovered on most attibute pathways due to the allocation overhead in the generic-supporting pathways. The workaround is to simply not take that slow/expensive path for non-generics.

While I'd like to optimize `RuntimeModule.ResolveMethod` further, there's a public surface area in play there that makes the changes non-trivial. There, we'll have to choose overhead on the public path (which may still be a net win), or duplication in code for another path.

* Update comments


Commit migrated from dotnet/coreclr@130cec3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants