Cache a per-type settings plan (P3) - #24
Merged
Merged
Conversation
ValuesPopulator now builds a SettingsPlan once per settings interface (cached on the populator instance, scoped to the builder's Options) instead of re-doing reflection on every populate. The plan resolves the section name once and lazily (a no-binder scan never pays for it), and holds per property a readonly-struct PropertyPlan/PropertyConversion carrying the resolved key, default value, and the converter chosen once up front. Converter selection walks the LinkedList manually rather than via LINQ First, so its struct enumerator is not boxed. Warm re-populate drops 52-56% in allocations (50 props: 15,681 -> 6,848 B). The gated ScanBenchmark rises +4.66% (under the 10% gate) - pure plan-build overhead on the populate-once cold scan. A new gated PlanPopulateBenchmark tracks the warm path. Reflective PropertyInfo.SetValue is retained: an emitted __Set method / compiled Action<object,object?> setter was implemented and measured but reverted - it regressed the gated cold ScanBenchmark (+25% for the emitted __Set) for zero warm gain, since net10's SetValue no longer allocates an args array. A tiered/lazy compiled setter (P3b) is noted as a follow-up only if set time (not allocation) ever matters. 56 tests pass on net8.0 + net10.0.
From a two-agent (code-review + perf) pass over the P3 diff: - Read [SettingsProperty] once per property at plan build instead of 3x (via GetPropertyName / GetDefaultValue / CreateConversion). GetCustomAttribute re-materializes the attribute each call, so at ~2000-type cold-scan scale this dominated: it drops the gated ScanBenchmark from +4.66% to ~flat (-0.40% vs master). The now-inlined GetPropertyName/GetDefaultValue helpers are retired (PropertyInfoExtensions deleted). - Re-wrap converter-setup failures at plan build as SettingsPropertyValueException, restoring the pre-P3 exception contract (a custom ConverterType that throws in its ctor / isn't an ISettingsTypeConverter used to surface wrapped, inside the per-populate convert try). - Materialize the section binders once in SettingsBuilder so the populator's 'as ISectionBinder[]' fast-path hits (the factory hands a SortedList.Values view); removes a per-populate ToArray (~32 B/call). Warm re-populate is now -55 to -61% vs master (50 props 15,681 -> 6,816 B). - Document ISettingsTypeConverter as stateless/thread-safe (custom converters are now selected once per type and shared). Minor: fix stale __Set benchmark docstring, drop a redundant string interpolation in GetNormalizeInterfaceName, pass PropertyPlan by 'in'. 56 tests pass on net8.0 + net10.0.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
P3 — cached "settings plan"
ValuesPopulatornow builds a per-typeSettingsPlanonce (cached on the populator instance, scoped to the builder'sSettingsOptions) instead of re-doing reflection on every populate:readonly structPropertyPlan/PropertyConversioncarrying the resolved key, default value, and the converter chosen once up front (folds in converter caching). Converter selection walks theLinkedListmanually rather than via LINQFirst, so its struct enumerator isn't boxed onto the heap.Results (BenchmarkDotNet, ShortRun — allocations)
ScanBenchmark.ColdScan(gated)The gated
ScanBenchmarkrise is pure plan-build overhead on the populate-once cold scan; it's well under the 10% allocation gate. A new gatedPlanPopulateBenchmarktracks the warm path going forward.On the "compiled setter"
The fix-plan floated emitting the setter into the generated type (or a compiled
Action). I built and measured the emitted__Setvariant: it regressed the gated coldScanBenchmarkby +25% (extra per-type IL-emit, not amortized on a populate-once scan) for zero warm gain — net10'sPropertyInfo.SetValueno longer allocates an args array, so the reflective set is already allocation-free. Reverted; reflectiveSetValueretained. A tiered/lazy compiled setter (P3b) is noted as a follow-up only if set time (not allocation) ever shows up in a profile.Notes
SettingsBindingException/SettingsPropertyValueException/ not-allow-null throw order) and identical section / key / converter resolution.FIX-PLAN.mdrefresh ride this PR (no separate docs branch).