Repository navigation
Restore native constructor graphs for decorated originals - #124
Conversation
|
Implemented in draft PR #124: #124 The original-type activator checked registration availability but did not plan nested dependency graphs. A rejected constructor could hide a missing nested dependency; a selected constructor could run an earlier application factory before the later graph failed. The exact issue probe independently reproduces both behaviors on develop and published 0.8.0 across all supported targets. The fix validates only original implementation-type descriptors before actual argument resolution. It copies immutable provider registrations in order, restores the original as the final exact binding for its effective key, and uses a temporary native provider to plan an internal generic root. The root's first argument is a guard factory that throws a private marker exception; its second argument requests the original service with the inherited lookup key. Native DI plans the complete graph first. Invalid graphs therefore throw native errors before activation; valid graphs reach the guard, which stops resolution before any application factory or constructor executes. Successful checks are cached per provider snapshot, original descriptor and effective key. Failures remain uncached. Ordered duplicate descriptors are retained so an invalid earlier enumerable member cannot be hidden by the last binding. Actual resolution continues through the application's provider, retaining its lifetimes, scopes, diagnostic tracking and disposal ownership. Factory originals, earlier decorator factories and nonempty DependsOn maps keep their existing policies. This uses public DI planning/resolution APIs and adds no private DI member lookup or invocation. Metadata still prefers Mammoth's snapshot and reuses the existing guarded copied-descriptor fallback for plain native providers. Custom probes without metadata retain their availability contract. The cold validation creates a temporary provider; later successful requests use the cache. The guard depends on native DI planning the full graph and resolving constructor arguments in order. Those assumptions and the scope/factory limits are explained in the architecture document, referenced by the code comment and README Architecture section. The vNext changelog is updated. Tests were committed first: the initial 156 cases produced 144 failures and 12 passing controls per target; the additional 12 enumerable-order/provider-isolation cases all failed against unchanged production code. All 168 new cases now pass. Verification for final commit
Paused for review before another issue. #122 remains open and should be resolved and reviewed before completing the release-validation checkpoint. No release actions were taken. |
|
Performance review of PR #124 after the concern about complexity and resolution overhead Compared unchanged PR head The concern is justified. The successful-validation cache prevents repeated temporary providers, but does not eliminate the ordinary probe, snapshot lookup, weak-table lookup and dictionary lookup on each original transient activation. A cold check copies the entire registration collection, builds a native provider, constructs a synthetic generic root and throws/catches a marker exception. Failed checks are not cached, and concurrent first requests can duplicate the work. Local probe: a transient original with one transient dependency and one forwarding decorator; plain BuildServiceProvider and Mammoth's factory; 10, 100 and 1,000 additional keyed registrations. Each cold median covers 101 fresh providers with construction and disposal excluded from timing. Each warm median covers nine batches of 100,000 resolutions after 100,000 warmup resolutions. Thread allocations were measured separately. Release builds, DI 10.0.0, DOTNET_TieredCompilation=0. Two complete runs, with reversed variant order in the second run. Every resolution asserted the expected decorator/original graph. Representative second-run medians, Mammoth factory, 1,000 additional registrations:
Repeated-resolution allocations remained unchanged: 1,040 bytes on net472 and 472 bytes on modern targets. Plain-provider first-resolution allocation at 1,000 registrations was approximately 442-444 KB for the PR versus 2.8-3.4 KB for develop, because that path also constructs the immutable snapshot on first use. These are local Stopwatch/GC microprobes, not BenchmarkDotNet confidence intervals or application-load benchmarks. Timing varies between runs (particularly net8 warm timing); both runs show overhead and the same allocation results. The filler registrations share a service type with distinct keys. Results cover a simple unkeyed transient graph and do not establish costs for every keyed, scoped, singleton, failing, concurrent or deeper graph. Cached scoped/singleton instances normally bypass original activation after the first resolution within their lifetime. Recommendation: revise before merging. First investigate using native DI's existing per-provider/key caching for successful validation instead of adding several custom cache/metadata lookups to every transient activation. That could reduce both custom cache machinery and warm overhead without new private-DI reflection, but still needs regression and performance verification and does not itself remove the cold registration-copy cost. A direct native-planner reflection bridge would be a separate compatibility choice, requires additional private APIs, and has not been approved or proven as a robust replacement here. Verification for this review: both performance runs completed on all four supported test targets, with no probe failures. No production changes, commits or pushes were made, so the existing CI result applies to the unchanged PR head; the new performance measurements are local evidence only. Keep #124 in draft and address this review before moving to #122. |
Issue #121: alternatives without additional service resolutionsInvestigated unchanged draft PR #124, head The constraint used here is strict: graph validation must not call GetService/GetRequiredService/GetKeyedService to resolve a synthetic root, guard or validation service. Metadata should reuse the ordinary availability probe that the original activator already acquires. Actual activation still resolves the original constructor's dependencies. RecommendationReplace the separate graph-validation pass and repeated constructor selection with a cached constructor and argument-binding plan. Build that plan without activation, then execute it against the actual application scope. Cache by immutable provider identity, original descriptor and effective key. Keep implementation factories and DependsOn policies separate. The strongest tested route delegates graph planning to native DI in a private registration copy with the original restored as the final exact binding. It calls the internal native planning method directly, extracts the selected constructor and argument call sites, and caches bindings. No temporary-root resolution or throwing guard is needed. Live-provider dependencies and built-ins resolve from the actual application provider; only missing-registration defaults and injected key constants are taken from the plan. No shadow provider or scope factory may escape into actual activation. Start with cached ConstructorInfo.Invoke and unwrap only its invocation wrapper. This already removes repeated reflection metadata inspection, candidate selection and per-call argument-resolution closures. Expression compilation is optional and was more expensive on the first resolution in these measurements; adding another compilation stage is not necessary for this fix. This route needs a compatibility decision before implementation. Beyond the existing guarded CallSiteFactory._descriptors field, the prototype uses ServiceProvider.CallSiteFactory, CallSiteFactory.GetCallSite(ServiceDescriptor, CallSiteChain), CallSiteChain construction, ConstructorCallSite.ConstructorInfo, ConstructorCallSite.ParameterCallSites and ServiceCallSite.Value. All would need explicit shape guards and upgrade regression coverage. These internals are used for both provider modes, not just plain BuildServiceProvider. No private cache mutation or native runtime-resolver invocation is proposed. An independent recursive planner over Mammoth's immutable descriptor snapshot is the alternative if additional private DI access is unacceptable. It can reuse public constructor metadata and the existing activator rules without resolving validation services or building a shadow provider. However, it must reproduce nested constructor selection, exact/open-generic precedence, enumerable order and constraint filtering, built-ins, AnyKey, inherited keys, defaults, cycles and opaque factories. That route adds substantially more code and ongoing native-DI parity responsibility. It was assessed from source; a complete recursive implementation was not built in this investigation. Other alternatives
The relevant pinned implementation is DI 10 CallSiteFactory and ServiceProvider. VerificationThe alternative executable passed 274 checks on each of Windows net472, net8.0, net9.0 and net10.0: 1,096 checks total, zero final failures. Covered rejected/selected constructors, open/closed generic dependencies, duplicate enumerable registrations, keyed/unkeyed roots, unrelated invalid registrations, cycles, key injection/inheritance, provider/scope-factory identity, DateTime/numeric/enum defaults, registered null values, opaque-factory exception identity, native preferred-constructor behavior, lifetimes and disposal. Both compiled and cached-invocation plans ran the main graph matrix. Each planning case asserted zero application factory calls. Executing a successful cached root plan through a counting provider performed exactly the constructor dependency lookups, with no probe or validation-service lookup inside that execution. Separate normal and diagnostic Mammoth-provider checks confirmed that the existing ordinary probe's copied descriptors contain the immutable snapshot instance, including pre-instrumentation type registrations. The snapshot can therefore be reused without resolving its support service; this uses the already-approved native descriptor field, not a new private DI member. The performance harness explicitly retains the activator's existing ordinary-probe lookup. Provider state and snapshots are bound by the experimental Build helper to each constructed provider. Production integration is not completed: it must bind/cache per provider and effective key without sharing plans across different builds of a caller collection. It must also preserve custom-probe behavior, diagnostic context and collectible/provider cleanup. The simple per-provider prototype state is not proposed as a globally captured registration cache. Earlier harness attempts exposed and repaired fixture expectation, reflection-member visibility, dependency-loading and assembly-reference selection problems. In particular, a bare assembly-name reference selected a nested net10 benchmark DLL for net472; explicit absolute assembly references repaired this. These were harness failures, not production regressions. Final runs completed on all four targets. NuGet vulnerability auditing could not reach api.nuget.org and emitted NU1900; package restoration from the local cache, compilation and execution succeeded. The intentional unused-parameter fixture warnings were suppressed locally; no production warning policy was changed. The full production test suite and remote CI were not rerun, because production and the PR head were unchanged. This is feasibility evidence, not a validated replacement PR. Performance evidenceLocal Release Stopwatch/GC probes; Microsoft DI 10.0.0; DOTNET_TieredCompilation=0. One transient dependency, one original and one forwarding decorator through Mammoth's factory. Registration counts: 10 and 1,000 additional keyed registrations. Cold median: 101 first resolutions from fresh providers, excluding provider construction/disposal. Warm median: seven batches of 100,000 resolutions after 100,000 warmups and observation that native dynamic resolver compilation completed. No validation/compilation process ran concurrently with the final benchmark run. Results remain exploratory, without BenchmarkDotNet confidence intervals or application-load evidence. Representative final medians at 1,000 additional registrations:
Cached invocation leaves first-resolution allocation around 203-206 KB at 1,000 registrations, comparable to the current shadow-provider approach. It removes the synthetic-root/guard work, but still copies registrations and builds a planning provider. It is not a universal cold-cost improvement. Compiled plans reduced the .NET 10 warm median further to 196.8 ns, but raised first resolution to 290.2 microseconds. Full public ValidateOnBuild raised .NET 10 cold resolution to 371.1 microseconds and about 786 KB, versus PR 71.6 microseconds and about 200 KB. The cached-invocation route is the more conservative starting point. These measured gains are optimistic until integrated: production provider/key cache access, synchronization and custom-provider handling may add costs. The benchmark's baseline uses a delegate to the unchanged original NativeConstructorActivator in a factory-original fixture; it is not a separately rebuilt develop package. PR mode uses its current normal implementation-type path. Do not present the prototype results as guaranteed shipped gains. Run |
|
On hold at the maintainer's request. Issue #121 stays open and this PR stays draft; preserve its branch, implementation and regression tests. The issue comment records the decision, DI's opaque-factory boundary, the remaining graph-error/timing differences, the complexity and resolution costs of this fix, and the compatibility tradeoffs of the alternatives. Additional private planning APIs and a permanent factory-semantics contract change are not approved. Develop and PR head |
|
Implemented #121 in draft PR #124, head The design and limits explain that Verification:
Performance uses the same Release harness and DI 10.0.12 for candidate
Across keyed/unkeyed cases on both measured runtimes, provider build/first-resolution medians improved 44–72% versus the release candidate and were also lower than the held planner. Warm transient medians improved 71–81%; new-scope medians improved 77–86%. These are local fixture measurements. Small differences between allocation-free cached lookups are process/tiering noise, not a claimed speedup. No package was published and no PR was merged. The issue remains open for review. Co-authored-by: Codex codex@openai.com |
|
Documentation reference for this PR: decoration design and ownership. The document explains the constructor regression with a runnable failure-before-activation example; native implementation-type registrations and private identities; the scoped holder and native compiled-cache limitation; factory layers, supplied instances and keyed scope isolation; diagnostic/validation boundaries; and the tradeoffs of restoring the earlier implementation. It includes a resolution diagram, internal code excerpts and complete consumer programs. Decorators must not dispose their injected inner service, through either Verified five documentation examples against the proposal on .NET 10 with DI 10.0.12, including async layer disposal, caller ownership, keys/scopes and graph failures. Link/fence/CRLF checks and skill validation passed. Commit CI on the documentation commit passed, including all four test targets and native hot-cache probes. Pack/publish steps were skipped. Renamed the reference to |
Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
Preserve keys and native ownership, with a runtime scoped holder for compiled caches. Update differential coverage and consumer guidance. Co-authored-by: Codex <codex@openai.com>
Document native original activation, private registration identities, scoped holders, factory layers, validation boundaries, and design tradeoffs with executable examples. Make inner-service disposal ownership explicit in the usage guide and packaged skill. Co-authored-by: Codex <codex@openai.com>
Co-authored-by: Codex <codex@openai.com>
3307571 to
a0165ec
Compare
|
Rebased this PR onto the latest Published head: Verification: clean checkout, current develop ancestry, no merge commits in the PR range, exact full-tree equality, and CRLF whitespace checks passed. CI on the rebased head passed, including all four test targets and native DI hot-cache regression probes. Pack/publish steps were skipped. The PR remains ready for review. |
Decorating an implementation-type registration hid its constructor graph. Invalid rejected candidates could resolve successfully, and an invalid selected graph could execute an earlier dependency factory before failing. This regression from 0.7.1 is present in 0.8.0.
Original types now retain native DI registrations under a private reference identity and their original public key. Native DI plans and owns the inner service; factory/map originals keep their existing policy. Scoped originals use a non-disposable holder under a runtime marker so native compiled cache keys retain their identity and disposal happens once. Diagnostics retain their exclusions and singleton context through a small per-provider options service.
The production change is confined to five files, 89 added lines and 8 removed lines. The registration rationale and compiled-cache safeguard are in the architecture document. The guide, skill and changelog are synchronized.
ValidateOnBuildcan now report decorated original graph errors at startup; differential tests compare the same native validation options. Factory/map/decorator constructors remain opaque to native graph inspection.Documentation now includes a detailed registration and ownership walkthrough, a resolution diagram, native-constructor failure examples, the scoped compiled-cache rationale, keyed scope isolation, factory/instance ownership, and alternatives to restoring the older design. Decorators must not dispose their injected inner service, through either
Dispose()orDisposeAsync(); the usage guide and packaged skill make this rule explicit.Verification:
Rebased onto current
develop(bc4e7658b2179945d5794860ec0bcb3fb6b864cc): five linear commits; full file tree is identical to prior PR head3307571e5d617673701113eab8de8eef9487a6c7. Whitespace checks passed. Rebased-head CI passed fora0165ec66f54f3b88ef65f94370ce07734c1bbbb, including all four test targets and native hot-cache probes; pack/publish steps were skipped.Documentation follow-up: five standalone examples compiled and executed on .NET 10 with DI 10.0.12; local links, code fences, CRLF whitespace and skill validation passed. Source/test tree is unchanged.
Documentation-head CI passed for
1e4d926f4cc5119fec953a36075db7cc74a9833d, including all four test targets and native hot-cache probes; pack/publish steps were skipped.Tests added before the fix: 156 failures and 12 passing controls per target on current develop.
Full Release suite: 2,918 passed per target on net472/net8.0/net9.0/net10.0; 11,672 total, zero failures or skips.
Graph errors require zero dependency-factory/constructor calls. Coverage includes rejected/selected constructors, closed/open generics, enumerable ordering, cycles, key identity, repeated layers, disposal and provider isolation.
Native test instrumentation observes compiled accessor replacement for both originals and scoped holders before 100 repeated resolutions and cache-isolation checks. This instrumentation is excluded from the production library.
Release solution build with CI settings and skill validation passed. Only the two existing net472 Microsoft.Extensions support warnings remain.
Local packed consumers with DI 10.0.12 passed on all four targets: 48 exact issue outcomes, 144 additional graph outcomes, Unkeyed decoration regresses providers without a keyed availability probe #120/DependsOn changes native precedence when FromKeyedServices and ServiceKey share a parameter #122 and 28 original-contract checks.
Implementation-head CI passed for
15e25c9f153b8f0ed28070cfe5c64c805f92f7b4, including all four test targets and native hot-cache probes. Pack/publish steps were skipped.Repeated local .NET 8/10 benchmarks with matched DI 10.0.12: warm transient allocations 752 → 216 B/op; new scoped resolution 1,064 → 552 B/op. Provider build/first-resolution medians were 44–72% faster than the release candidate in this fixture; warm transient medians were 71–81% faster. Cached scoped/singleton lookups remain allocation-free. Detailed method and limits are in the discussion.
Fixes #121. Ready for review; no merge or publication.
Co-authored-by: Codex codex@openai.com