Skip to content

fix: reduce O(types × inherited-members) registration cost from [ServiceKey] scan - #1495

Merged
tillig merged 2 commits into
autofac:developfrom
npease18:fix/reflectionactivator-servicekey-scan
Aug 1, 2026
Merged

fix: reduce O(types × inherited-members) registration cost from [ServiceKey] scan#1495
tillig merged 2 commits into
autofac:developfrom
npease18:fix/reflectionactivator-servicekey-scan

Conversation

@npease18

Copy link
Copy Markdown
Contributor

As initially reported in #1480, a performance regression was discovered in the start time of Autofac applications with large numbers of derived types and inherited members. I was able to concretely nail this down using this example codebase where I was able to determine that the ServiceKey scan introduces a O(types × inherited-members) regression, that can be simplified down to O(distinct declared members).

Here is the result of the benchmark run in that other repo (./scripts/benchmark.sh 9.0.0 9.1.0 9.3.1 fix)

  Autofac     scan         Build() cold    vs baseline     keyed
  ----------  ---------  --------------  -------------  --------
  9.0.0       absent             0.522s          1.00x         0
  9.1.0       present            3.022s          5.79x         0
  9.3.1       present            2.902s          5.56x         0
  local       present            0.909s          1.74x         0

Let me know if there is any additional information I can provide or change!

@tillig

tillig commented Jul 31, 2026

Copy link
Copy Markdown
Member

I've enabled the build to run and while I look over the code proper, it seems there are some formatting/style issues to fix to allow the build to progress.

@npease18 npease18 closed this Jul 31, 2026
@npease18
npease18 force-pushed the fix/reflectionactivator-servicekey-scan branch from c4d5a06 to 7f0d04b Compare July 31, 2026 19:22
…iceKey] scan

Co-authored-by: Brett Gabor <brett.gabor@tylertech.com>
@npease18 npease18 reopened this Jul 31, 2026
@npease18

Copy link
Copy Markdown
Contributor Author

I've enabled the build to run and while I look over the code proper, it seems there are some formatting/style issues to fix to allow the build to progress.

Misfire there, looks like I forgot to run the linters before I pushed up. The latest push should have the correct styling.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.26%. Comparing base (7f0d04b) to head (d16833f).
⚠️ Report is 1 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1495      +/-   ##
===========================================
- Coverage    78.33%   78.26%   -0.07%     
===========================================
  Files          218      218              
  Lines         5944     5948       +4     
  Branches      1273     1274       +1     
===========================================
- Hits          4656     4655       -1     
- Misses         748      753       +5     
  Partials       540      540              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs
Comment thread src/Autofac/Core/Activators/Reflection/ReflectionActivator.cs Outdated
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs Outdated
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs Outdated
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs Outdated
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs
Comment thread test/Autofac.Specification.Test/Features/KeyedServiceTests.cs
Comment thread bench/Autofac.Benchmarks/ContainerBuildInheritedMembersBenchmark.cs Outdated
@tillig

tillig commented Jul 31, 2026

Copy link
Copy Markdown
Member

Thank you for this — outside PRs are a rarity for us and genuinely appreciated, and this is a good one. You found a real, non-obvious performance bug, diagnosed it correctly, and fixed it in the right place, with benchmarks and tests to back it up. That's the whole package.

I reproduced the root cause and the improvement independently before reviewing, since a caching fix is the kind of thing worth confirming from scratch.

Reflection hands back a distinct PropertyInfo instance per reflecting type, so the ServiceKeyPropertyAttributes cache (keyed on PropertyInfo) missed on every (derived type x inherited member) pair:

D1 prop ReflectedType=D1 DeclaringType=WideBase
ReferenceEquals(p1,p2)=False  p1.Equals(p2)=False
distinct cache entries for the same logical property across D1/D2/Base = 3

Measuring distinct derived types over a 20-property base (net10.0, Release):

types develop this PR
250 53.1 ms 43.5 ms
1000 199.8 ms 42.3 ms
2000 932.8 ms 84.7 ms

Baseline growth is clearly super-linear (4.7x from 1000 to 2000); this PR is roughly linear. Matches the numbers in your PR description.

Also verified:

  • src/Autofac Release across all 4 TFMs: 0 warnings, 0 errors.
  • Autofac.Test 870/870 and Autofac.Specification.Test 504/504 pass.
  • Both new suppressions are genuinely necessary — I removed each and confirmed the build fails (S6612, IL2072). Neither is defensive noise, which is nice to see.
  • Your new tests really do guard the recursion, not just the DeclaredOnly switch: I applied a naive half-fix (DeclaredOnly with no BaseType walk) and 2 of the 6 fail. They're pinning the right thing.

Everything I've left in the inline comments is cosmetic — comment wording, one suppression written as [SuppressMessage] instead of a pragma pair, and one sentence in a benchmark comment that I measured and found isn't accurate. No correctness concerns, and nothing that changes your approach. The code itself is good to go.

The one item worth a real look is the comment on ReflectionActivator.cs line 148: the rewrite intentionally detects strictly more than the old scan did, which is both safe and the right call here, but nothing currently says so out loud. I'd just like that written down for whoever reads it next.

@npease18

npease18 commented Aug 1, 2026

Copy link
Copy Markdown
Contributor Author

Just addressed those changes via the fixup! commit. If everything looks good, I will go ahead and squash it back down!

@tillig tillig left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is great! Thanks for your help and your patience. I'll see about getting a release out with this in it shortly.

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