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

Eliminate allocations from ReflectionUtility.GetImplementedInterfaces #955

Merged

Conversation

MattKotsenas
Copy link
Contributor

ReflectionUtility.GetImplementedInterfaces allocated an iterator object for every call to GetImplementedInterfaces. This results in an allocation per object serialized in the benchmark.

I refactored the implementation to avoid the iterator by using Type.FindIterfaces. In order to ensure there are no captures / allocations in the delegate, I made the lambda static (which necessitates the move to lang version 9).

This change saves about 2% of CPU and memory allocations in the benchmark:

Before

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22635.4010)
Intel Core i9-10940X CPU 3.30GHz, 1 CPU, 28 logical and 14 physical cores
.NET SDK 9.0.100-preview.7.24407.12
  [Host]                       : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX-512F+CD+BW+DQ+VL
  MediumRun-.NET 8.0           : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX-512F+CD+BW+DQ+VL
  MediumRun-.NET Framework 4.7 : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256

IterationCount=15  LaunchCount=2  WarmupCount=10

| Method     | Job                          | Runtime            | Mean      | Error    | StdDev   | Gen0      | Gen1     | Allocated |
|----------- |----------------------------- |------------------- |----------:|---------:|---------:|----------:|---------:|----------:|
| Serializer | MediumRun-.NET 8.0           | .NET 8.0           |  51.52 ms | 0.914 ms | 1.368 ms | 2000.0000 | 500.0000 |  24.44 MB |
| Serializer | MediumRun-.NET Framework 4.7 | .NET Framework 4.7 | 114.88 ms | 5.786 ms | 8.111 ms | 7750.0000 | 500.0000 |     48 MB |

// * Warnings *
MultimodalDistribution
  SerializationBenchmarks.Serializer: MediumRun-.NET Framework 4.7 -> It seems that the distribution is bimodal (mValue = 3.85)
MinIterationTime
  SerializationBenchmarks.Serializer: MediumRun-.NET 8.0 -> The minimum observed iteration time is 97.056ms which is very small. It's recommended to increase it to at least 100ms using more operations.

// * Hints *
Outliers
  SerializationBenchmarks.Serializer: MediumRun-.NET 8.0 -> 1 outlier  was  removed, 2 outliers were detected (48.53 ms, 54.46 ms)

After

// * Summary *

BenchmarkDotNet v0.14.0, Windows 11 (10.0.22635.4010)
Intel Core i9-10940X CPU 3.30GHz, 1 CPU, 28 logical and 14 physical cores
.NET SDK 9.0.100-preview.7.24407.12
  [Host]                       : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX-512F+CD+BW+DQ+VL
  MediumRun-.NET 8.0           : .NET 8.0.8 (8.0.824.36612), X64 RyuJIT AVX-512F+CD+BW+DQ+VL
  MediumRun-.NET Framework 4.7 : .NET Framework 4.8.1 (4.8.9261.0), X64 RyuJIT VectorSize=256

IterationCount=15  LaunchCount=2  WarmupCount=10

| Method     | Job                          | Runtime            | Mean      | Error    | StdDev   | Gen0      | Gen1     | Gen2     | Allocated |
|----------- |----------------------------- |------------------- |----------:|---------:|---------:|----------:|---------:|---------:|----------:|
| Serializer | MediumRun-.NET 8.0           | .NET 8.0           |  50.79 ms | 1.039 ms | 1.556 ms | 2000.0000 | 500.0000 |        - |  23.83 MB |
| Serializer | MediumRun-.NET Framework 4.7 | .NET Framework 4.7 | 123.98 ms | 2.078 ms | 3.046 ms | 7600.0000 | 600.0000 | 200.0000 |  47.39 MB |

// * Warnings *
MinIterationTime
  SerializationBenchmarks.Serializer: MediumRun-.NET 8.0 -> The minimum observed iteration time is 94.463ms which is very small. It's recommended to increase it to at least 100ms using more operations.

// * Hints *
Outliers
  SerializationBenchmarks.Serializer: MediumRun-.NET Framework 4.7 -> 1 outlier  was  removed (132.06 ms)

@MattKotsenas
Copy link
Contributor Author

Note that this PR uses the same benchmark introduced in #953 and #954. So if / when one of these PRs merge, the other should be rebased on top of latest main to remove the duplicate commit.

I'm happy to do this housekeeping. I just mention it for clarity, and so that each PR can be reviewed independently, rather than forcing them to all be chained together.

@MattKotsenas MattKotsenas force-pushed the refactor/reflection-interfaces-allocs branch from 5de17a7 to ed6e48d Compare August 16, 2024 00:23
@MattKotsenas MattKotsenas force-pushed the refactor/reflection-interfaces-allocs branch from ed6e48d to d49e964 Compare August 31, 2024 23:50
@EdwardCooke EdwardCooke merged commit de005eb into aaubry:master Sep 1, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants