From 19ea192adca720ba9a9114723147a366b569da89 Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Sun, 12 Jul 2026 23:07:17 +0300 Subject: [PATCH 1/2] Cache a per-type settings plan (P3) 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 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. --- .github/workflows/benchmark.yml | 6 +- FIX-PLAN.md | 9 +- SESSION-HANDOFF.md | 28 +-- .../Conversion/PropertyConversion.cs | 44 +++++ .../Core/Reflection/ITypeConverter.cs | 19 +- .../Core/Reflection/TypeConverter.cs | 132 ++++++------- .../SettingsPlan.cs | 52 +++++ .../ValuesPopulator.cs | 183 ++++++++++-------- .../PlanPopulateBenchmark.cs | 54 ++++++ 9 files changed, 358 insertions(+), 169 deletions(-) create mode 100644 src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs create mode 100644 src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs create mode 100644 src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 5469a6b..86ed265 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -57,13 +57,13 @@ jobs: - name: Run benchmarks working-directory: src # global.json lives here # ShortRun keeps CI time reasonable; allocation counts are deterministic regardless of - # iteration count. The micro-benchmark filters match nothing until PR #21 lands them on - # master, at which point they are tracked automatically alongside ScanBenchmark. + # iteration count. PlanPopulateBenchmark tracks the P3 warm re-populate path; it has no gh-pages + # baseline until the first master run records one, so it only starts alerting after that. run: > dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- - --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" + --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" "*PlanPopulateBenchmark*" --job short --exporters json --artifacts ${{ github.workspace }}/bdn diff --git a/FIX-PLAN.md b/FIX-PLAN.md index f1fc599..5cdee75 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -5,8 +5,9 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform ## Progress (2026-07-12) - **Done & merged:** B1, B2, B4, B5, B9 + T1, T2 (PR #8) · BindingContext test (#10) · D3 namespace typo (#11) · T3 DI integration tests (#12) · solution rename (#13) · **A2 naming → ExistForAll (#15)** · **P0 benchmark harness (#16)** · **P1 provider cache + C3 decided/implemented (#17)** · **P2 memoize `ExtractTypeProperties` + `HashSet` dedup (#18)** · **docs tutorials refresh (#20)** · **Q1–Q4 perf quick wins + M1 collision fix + micro-benchmarks (#21)**. - **Q1–Q4 proven** via isolated micro-benchmarks (macro `ScanBenchmark` can't resolve them): Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0. **Q5 was already resolved by B4.** **M1** (code-review finding): namespace-qualify the generated impl name in the generator only — `GetNormalizeInterfaceName` also backs the section name. Suite → **56 per TFM**. -- **In flight:** **#22 benchmark-tracking CI** — runs BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time is informational. Green, ready to merge. -- **Next:** P3 (cached compiled "settings plan") — the biggest remaining ceiling. +- **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)**. +- **In flight:** **P3 — cached "settings plan"** (`SettingsPlan` per type: section name resolved once + lazily, per-property key/default/converter precomputed). Warm re-populate **−52–56%** allocations (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.7%** (under the 10% gate). The emitted/compiled setter was built and measured but **reverted** — it regressed the gated cold scan **+25%** for **no** warm gain (net10 reflective `SetValue` is already allocation-free for the args). A new gated `PlanPopulateBenchmark` tracks the warm path. +- **Next:** P4 (de-reflect array/enumerable converters) → P5 (resolve config section once per type). - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. - Running status lives in `SESSION-HANDOFF.md`. @@ -55,7 +56,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - [x] P0 · Upgrade the benchmark harness (MemoryDiagnoser + phase-split + fixtures) — do first, to measure P1–P3 - [x] P1 · Cache built instance on the `ISettingsProvider` resolve path · Sev High · Eff S - [x] P2 · Memoize `ExtractTypeProperties` + fix O(n²) dedup · Sev High · Eff S -- [ ] P3 · Cached compiled “settings plan” (emit setters, hoist names, cache converters) · Sev High · Eff L +- [x] P3 · Cached “settings plan” — hoist section (lazy) + keys, precompute/cache converters, plan per type. Reflective `SetValue` kept; compiled setter deferred (regressed the gated cold scan for no warm gain) · Sev High · Eff L - [x] Q1–Q5 · Quick wins (GetEnumerator, OrdinalIgnoreCase, env-binder, type-cache; Q5 dead ctor checks already done by B4) - [ ] P4 · De-reflect array/enumerable converters · Sev Med · Eff M - [ ] P5 · Resolve config section once per type, not per property · Sev Med · Eff M @@ -208,6 +209,8 @@ if (settingsOptions.AttributeType != null && ### P3 · Cached compiled “settings plan” — Sev High · Eff L (biggest ceiling) The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55`) is reflection-saturated: reflective `property.SetValue` (`:55`, boxes value types); `GetSectionName` recomputed inside both loops though it’s constant per type; `GetPropertyName` recomputed per binder; `SettingsPropertyAttribute` read ~3×/property (`:36-40` + `TypeConverter.cs:36`). Build a per-interface `SettingsPlan` once (`ConcurrentDictionary`) holding: section name (once), and per property — resolved key, default value, chosen converter, and a **compiled setter** (emit the populate into the generated class in `PropertyCreator`, or a compiled `Action`). Folds in converter caching (below). +**Resolved (this PR):** `SettingsPlan` cached per type on the `ValuesPopulator` instance — section name resolved once and **lazily** (a no-binder scan never pays for it), per-property `PropertyPlan`/`PropertyConversion` as `readonly struct`s (one array alloc, no per-property object), converter chosen once (manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −52–56%** (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.66%**. The **compiled setter was dropped**: both variants (emit `__Set` into the generated type, and a compiled `Action`) regressed the *gated* cold `ScanBenchmark` (+25% for the emitted `__Set`) because the extra per-type codegen isn't amortized on a populate-once scan — and it bought **nothing** on the warm path, since net10's reflective `PropertyInfo.SetValue` no longer allocates an args array. **Follow-up (P3b, if ever needed):** tiered/lazy setter compilation (compile on the 2nd+ populate) would keep the cold scan flat while de-reflecting the hot path; only worth it if a future profile shows `SetValue` time (not allocation) matters. + ### Quick wins — Eff S each - **Q1** `SettingsCollection.GetEnumerator` (`SettingsCollection.cs:52`) rebuilds a whole `Dictionary` per enumeration → `yield return` over the existing dictionary. - **Q2** `SettingsTypesExtractor.cs:32` `Name.ToLower().EndsWith(suffix.ToLower())` → `EndsWith(suffix, StringComparison.OrdinalIgnoreCase)` (also fixes an ordinal-correctness smell); hoist the trimmed suffix. (Startup allocs across all scanned types.) diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 063abd6..eb43f38 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,26 +3,28 @@ _Last updated: 2026-07-12 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P2 + quick wins Q1–Q4** (merged), plus a namespace-collision fix (**M1**) and **isolated micro-benchmarks that prove the wins** (2.7×–32× on the repeated paths, allocations eliminated). A **per-push benchmark-tracking CI** that gates PRs on allocation regressions is set up. `master` is clean; **one PR open — #22 (benchmark tracking), green and ready to merge.** Suite green — **56 tests on net10.0** locally (CI runs net8.0 + net10.0). +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P2 + quick wins Q1–Q4** (merged), plus a namespace-collision fix (**M1**) and **isolated micro-benchmarks that prove the wins** (2.7×–32× on the repeated paths, allocations eliminated). A **per-push benchmark-tracking CI** that gates PRs on allocation regressions is **merged and live** (#22). **P3 — cached "settings plan" — is done (this PR, branch `perf/p3-compiled-settings-plan`):** warm re-populate **−52–56%** allocations (50 props 15,681→6,848 B), gated `ScanBenchmark` **+4.7%** (under the 10% gate). Suite green — **56 tests on net10.0** locally (CI runs net8.0 + net10.0). Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. ## Do this first (new session) 1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. -2. **Merge #22** (benchmark tracking) via the `guy-lud` identity if it's still open — it's green/CLEAN. The first master run after merge records the allocation baseline on `gh-pages`. -3. `git checkout master && git pull`, then continue at **P3** (next perf item). +2. **Merge the P3 PR** (`perf/p3-compiled-settings-plan`) if still open + green — via the `guy-lud` identity. Confirm the benchmark check passed (scan ≈ +4.7%, under the 10% gate). Then `git checkout master && git pull`. +3. Start **P4** (next perf item) on a fresh work branch. **Refresh this handoff on that branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state -- On **`master`** @ `4dd002a` (PR #21). Clean tree. -- **Open PR: #22** — the benchmark-tracking workflow (green). Merge it first. -- A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there. Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. +- On branch **`perf/p3-compiled-settings-plan`** (off `master` @ `6bd5b0e`) — **P3 PR open**, this session's work; warm −56% / scan +4.7%, 56 tests green. Merge it, then `master` is the base for P4. +- A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there (first recorded on the #22 master run). Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) +- **P3 — cached "settings plan" (this PR).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it), and per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −52–56%** (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.66%** (under the 10% gate). New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. Follow-up **P3b** (tiered/lazy setter compilation) noted in `FIX-PLAN.md` only if a future profile shows set *time* matters. +- **#23 — session-wrap docs.** Handoff + fix-plan refresh; gitignored BenchmarkDotNet output + the personal `.claude/settings.local.json` (SessionStart hook). Squash-merged onto `master`. +- **#22 — benchmark-tracking CI merged.** (Detail below.) First master run recorded the allocation baseline on `gh-pages`. - **#21 — Perf quick wins Q1–Q4 + M1 fix + micro-benchmarks.** - Q1 `SettingsCollection.GetEnumerator` yields over its dictionary (was rebuilding a whole `Dictionary` per enumeration); Q2 `SettingsTypesExtractor` suffix match → `EndsWith(…, OrdinalIgnoreCase)` + hoisted suffix (kills a `ToLower` CurrentCulture smell); Q3 `EnvironmentVariableBinder` fast-paths `context.Key` (no `StringBuilder`) + single lookup; Q4 `SettingsClassGenerator` caches the generated impl by interface `Type`. **Q5 was already done by B4.** - **M1 (found in the code review):** Q4's Type-keyed cache exposed a latent collision — the generated impl name was derived from the *simple* interface name, so `Foo.ISettings` + `Bar.ISettings` collided and aborted the scan. Fixed by namespace-qualifying the impl name **in the generator only**. ⚠️ `GetNormalizeInterfaceName` was left alone on purpose — it also backs the default config **section name** (`SettingsOptions.SectionNameFormatter`). - **Micro-benchmarks** (`MicroBenchmarks.cs`): `EnumerateBenchmark` (Q1), `EnvBinderBenchmark` (Q3), `GenerateTypeBenchmark` (Q4). The macro `ScanBenchmark` can't resolve these (IL-emit dominates), so they isolate each hot path. Proven before/after: **Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0**. The benchmark assembly now has `InternalsVisibleTo` (Info.cs) + a Binders project ref. -- **(open) #22 — benchmark tracking CI** (`.github/workflows/benchmark.yml`). Runs BDN (ShortRun) on push-to-master and PRs, `jq`-extracts per-benchmark **allocated bytes**, feeds `benchmark-action/github-action-benchmark` (`customSmallerIsBetter`) stored on `gh-pages`. PRs comment the allocation diff and **fail on a >10% regression**; time is informational only. +- **#22 — benchmark-tracking CI (merged)** (`.github/workflows/benchmark.yml`). Runs BDN (ShortRun) on push-to-master and PRs, `jq`-extracts per-benchmark **allocated bytes**, feeds `benchmark-action/github-action-benchmark` (`customSmallerIsBetter`) stored on `gh-pages`. PRs comment the allocation diff and **fail on a >10% regression**; time is informational only. - **#20 — docs tutorials refresh.** All six `docs/*.md` rewritten against the current public API + the `SimpleConfig`→`SimpleSettings` rename (settles the A2 docs debt). - **#18 — P2.** Memoized `TypePropertiesExtractor.ExtractTypeProperties` + `HashSet` dedup; cache is a **private instance field** (not static, not injected). - **#17 — P1 + C3.** `ISettingsProvider.GetSettings` now serves the startup-built `ISettingsCollection` (same instance as the DI singleton); build fallback only for never-scanned types. **C3 = provider-level cache only** (Core `SettingsBuilder.GetSettings` unchanged; no reload). @@ -38,12 +40,11 @@ Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking chan - **Pre-stable window:** no `v*` stable tag (the `version-*` tags are the dead legacy package). Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -1. **Merge #22** if still open (see "Do this first"). -2. **Perf:** **P3** — cached compiled "settings plan" (emit setters into the generated class, hoist section/key names once per type, cache the chosen converter per property). Biggest remaining ceiling, Eff L. Then **P4** (de-reflect array/enumerable converters) → **P5** (resolve the config section once per type). -3. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, T6 converters, T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). -4. **Architecture:** A1 (AOT/trim annotations — HIGH, `Reflection.Emit` lib), C1 (`List`/`IList` support), C2 (public `SimpleSettingsException` base), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM), A5 (make `SettingsHolder` internal), A6 (command-line quoted-arg parsing). -5. **README** links — the `docs/` tutorials were done in #20; the README may still have stale `existall/SimpleConfig` links. -6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. +1. **Perf:** **P4** — de-reflect the array/enumerable converters (`Array.Empty`/`Array.CreateInstance` + indexed assignment; DRY the near-duplicate `ArrayTypeConverter`/`EnumerableTypeConverter`), then **P5** — resolve the config section once per type (pass section context via the P3 plan, which already resolves the section name lazily). Optional **P3b**: tiered/lazy compiled setter (only if set *time* shows up in a profile — allocation is already handled). +2. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, T6 converters, T7 generator concurrency stress — the unsynchronized check-then-`DefineType` in `GenerateType` is **still open** (Q4's `ConcurrentDictionary` made the cache thread-safe but did not close that race). +3. **Architecture:** A1 (AOT/trim annotations — HIGH, `Reflection.Emit` lib), C1 (`List`/`IList` support), C2 (public `SimpleSettingsException` base), A3 (`Core.AspNet` public type or drop the package), A4 (float `Microsoft.Extensions.*` floor per-TFM), A5 (make `SettingsHolder` internal), A6 (command-line quoted-arg parsing). +4. **README** links — the `docs/` tutorials were done in #20; the README may still have stale `existall/SimpleConfig` links. +5. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch. ## How releasing works (unchanged — durable) - **`ci.yml`** — on PRs to `master`: build + test (net8.0 + net10.0). **`release.yml`**: push to `master` → auto-publishes a MinVer height-based `-alpha` to nuget.org; manual **Release** (`workflow_dispatch`, `channel` beta/rc/stable + `bump` patch/minor/major) computes the next version, tags `v*`, publishes, creates a GitHub Release (`dry_run: true` previews). @@ -56,4 +57,5 @@ Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking chan - **Run `dotnet` from `src/`** (global.json opts into Microsoft.Testing.Platform for TUnit). Only the net10 runtime is installed locally → net8 is **build-only** locally; CI runs both. Do NOT prefix `cd ` before `dotnet`. - **Benchmarks:** run from `src/` — `dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- --filter --job short`. Output dir (`BenchmarkDotNet.Artifacts/`) is now gitignored. Micro-benchmarks depend on the benchmark assembly's `InternalsVisibleTo` (Info.cs) + the Binders project ref. - **`FIX-PLAN.md`** (repo root) is the full, prioritized plan with per-item file:line detail — open it explicitly; it is not auto-injected. +- **Wrap ritual — handoff branch rule:** refresh this file **on the current work branch** (or `master`) so it merges with the session's real PR. **Never** create a dedicated docs branch/PR for it (that's what #23 was) — `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only merge burns a throwaway `-alpha` for nothing. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs new file mode 100644 index 0000000..d10d3a0 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs @@ -0,0 +1,44 @@ +using System; + +namespace ExistForAll.SimpleSettings.Conversion +{ + // A per-property conversion resolved once (at plan build) instead of on every populate: the chosen + // converter, the stripped target type, and the null-value outcome are all baked in here, so the hot path + // is a single null-check plus one virtual Convert call — no attribute reads, no converter scan. + // A readonly struct so it lives inline inside the PropertyPlan value array — no per-property heap object, + // which keeps the cold at-scale scan (a plan per type, each built once) from regressing on allocations. + internal readonly struct PropertyConversion + { + private readonly ISettingsTypeConverter _converter; + private readonly Type _strippedType; + private readonly bool _throwOnNull; + private readonly object? _nullResult; + private readonly string _propertyName; + + public PropertyConversion(ISettingsTypeConverter converter, + Type strippedType, + bool throwOnNull, + object? nullResult, + string propertyName) + { + _converter = converter; + _strippedType = strippedType; + _throwOnNull = throwOnNull; + _nullResult = nullResult; + _propertyName = propertyName; + } + + public object? Convert(object? value) + { + if (value == null) + { + if (_throwOnNull) + throw new Exception(Resources.PropertyNotAllowNullMessage(_propertyName)); + + return _nullResult; + } + + return _converter.Convert(value, _strippedType); + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs index 5fbd30b..443c197 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs @@ -1,9 +1,10 @@ -using System.Reflection; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal interface ITypeConverter - { - object? ConvertValue(object? value, PropertyInfo propertyInfo, SettingsOptions options); - } -} \ No newline at end of file +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; + +namespace ExistForAll.SimpleSettings.Core.Reflection +{ + internal interface ITypeConverter + { + PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsOptions options); + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs index e7c10a0..8d03c1c 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs @@ -1,65 +1,67 @@ -using System; -using System.Linq; -using System.Reflection; -using ExistForAll.SimpleSettings.Conversion; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal class TypeConverter : ITypeConverter - { - public object? ConvertValue(object? value, PropertyInfo propertyInfo, SettingsOptions options) - { - var propertyType = propertyInfo.PropertyType; - - if (value == null) - { - ValidateNullAcceptance(propertyInfo); - - if (!propertyType.IsEnumerable()) - return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; - - var genericType = propertyType.GetTypeInfo().GetGenericArguments().First(); - var method = typeof(Enumerable).GetTypeInfo().GetMethod("Empty")!.MakeGenericMethod(genericType); - var emptyEnumerable = method.Invoke(null, null); - return emptyEnumerable; - } - - var strippedType = StripIfNullable(propertyType); - - var settingsTypeConverter = GetConverter(strippedType, propertyInfo, options); - - return settingsTypeConverter.Convert(value, strippedType); - } - - private ISettingsTypeConverter GetConverter(Type strippedType, PropertyInfo propertyInfo, SettingsOptions options) - { - var attribute = propertyInfo - .GetCustomAttribute(); - - if (attribute?.ConverterType == null) - return options.Converters.First(x => x.CanConvert(strippedType)); - - var converter = (ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!; - - return converter; - } - - private static Type StripIfNullable(Type type) - { - return type.GetTypeInfo().IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) ? - type.GetTypeInfo().GetGenericArguments()[0] : - type; - } - - private static void ValidateNullAcceptance(PropertyInfo propertyInfo) - { - var attribute = propertyInfo.GetCustomAttribute(); - - if(attribute == null) - return; - - if(!attribute.AllowEmpty) - throw new Exception(Resources.PropertyNotAllowNullMessage(propertyInfo.Name)); - } - } -} +using System; +using System.Linq; +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; + +namespace ExistForAll.SimpleSettings.Core.Reflection +{ + internal class TypeConverter : ITypeConverter + { + // Resolve everything about a property's conversion up front: which converter handles it, the target + // type once nullability is stripped, and what a null bound value should yield. Previously all of this + // (including up to three SettingsPropertyAttribute reads and a linear converter scan) ran on every + // single populate; now it runs once per type and the result is cached in the settings plan. + public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsOptions options) + { + var propertyType = propertyInfo.PropertyType; + var attribute = propertyInfo.GetCustomAttribute(); + + var throwOnNull = attribute != null && !attribute.AllowEmpty; + var nullResult = CreateNullResult(propertyType); + + var strippedType = StripIfNullable(propertyType); + var converter = GetConverter(strippedType, attribute, options); + + return new PropertyConversion(converter, strippedType, throwOnNull, nullResult, propertyInfo.Name); + } + + // The value a null bound value converts to: an empty sequence for IEnumerable, a default instance + // for a value type, otherwise null. Constant per property, so it is materialized once at plan build. + private static object? CreateNullResult(Type propertyType) + { + if (!propertyType.IsEnumerable()) + return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; + + var genericType = propertyType.GetTypeInfo().GetGenericArguments().First(); + var method = typeof(Enumerable).GetTypeInfo().GetMethod("Empty")!.MakeGenericMethod(genericType); + return method.Invoke(null, null); + } + + private static ISettingsTypeConverter GetConverter(Type strippedType, + SettingsPropertyAttribute? attribute, + SettingsOptions options) + { + if (attribute?.ConverterType != null) + return (ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!; + + // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed + // onto the heap — this runs once per property at plan build. + foreach (var converter in options.Converters) + { + if (converter.CanConvert(strippedType)) + return converter; + } + + // Unreachable in practice: DefaultTypeConverter.CanConvert always returns true. Kept so every path + // returns and to mirror the old First(...) which also threw when nothing matched. + throw new InvalidOperationException($"No converter found for type '{strippedType}'."); + } + + private static Type StripIfNullable(Type type) + { + return type.GetTypeInfo().IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) ? + type.GetTypeInfo().GetGenericArguments()[0] : + type; + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs new file mode 100644 index 0000000..fd1fd54 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs @@ -0,0 +1,52 @@ +using System; +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings +{ + // Everything needed to populate one settings interface that is constant across instances, computed once + // and cached per type: the section name (resolved once, not per property/binder) and the per-property + // plan (resolved key, default value, and precomputed conversion). + internal sealed class SettingsPlan + { + private readonly Type _settingsType; + private readonly SettingsOptions _options; + private string? _sectionName; + + public SettingsPlan(Type settingsType, SettingsOptions options, PropertyPlan[] properties) + { + _settingsType = settingsType; + _options = options; + Properties = properties; + } + + // Resolved lazily and cached: a builder with no binders (e.g. the cold assembly scan) never reads it, + // so it never pays the section-name attribute lookup. The value is deterministic, so the racy set is + // harmless — concurrent readers compute the same string. + public string SectionName => _sectionName ??= _settingsType.GetSectionName(_options); + + public PropertyPlan[] Properties { get; } + } + + // A readonly struct held inline in SettingsPlan.Properties: the whole per-type plan is then one array + // allocation plus the section string, rather than an object per property (matters at scan scale). + internal readonly struct PropertyPlan + { + public PropertyPlan(PropertyInfo property, string key, object? defaultValue, PropertyConversion conversion) + { + Property = property; + Key = key; + DefaultValue = defaultValue; + Conversion = conversion; + } + + public PropertyInfo Property { get; } + + public string Key { get; } + + public object? DefaultValue { get; } + + public PropertyConversion Conversion { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs index 5eb336a..94a66b1 100644 --- a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +++ b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs @@ -1,76 +1,107 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Reflection; -using ExistForAll.SimpleSettings.Core.Reflection; - -namespace ExistForAll.SimpleSettings -{ - internal class ValuesPopulator : IValuesPopulator - { - private readonly ITypePropertiesExtractor _typePropertiesExtractor; - private readonly ITypeConverter _typeConverter; - - public ValuesPopulator() : - this(new TypePropertiesExtractor(), new TypeConverter()) - { - } - - internal ValuesPopulator(ITypePropertiesExtractor typePropertiesExtractor, ITypeConverter typeConverter) - { - _typePropertiesExtractor = typePropertiesExtractor; - _typeConverter = typeConverter; - } - - public void PopulateInstanceWithValues(object instance, - Type settings, - SettingsOptions options, - IEnumerable binders) - { - var sectionBinders = binders as ISectionBinder[] ?? binders.ToArray(); - foreach (var property in _typePropertiesExtractor.ExtractTypeProperties(settings)) - { - var tempValue = property.GetDefaultValue(); - foreach (var binder in sectionBinders) - { - var context = new BindingContext(settings.GetSectionName(options), - property.GetPropertyName(), - settings, - property, - tempValue); - - try - { - binder.BindPropertySettings(context); - if (context.HasNewValue) - tempValue = context.NewValue; - } - catch (Exception e) - { - throw new SettingsBindingException(binder, context, e); - } - } - - var propertyValue = ConvertPropertyValue(settings, tempValue, property, options); - property.SetValue(instance, propertyValue); - } - } - - private object? ConvertPropertyValue(Type settingsType, - object? value, - PropertyInfo property, - SettingsOptions options) - { - try - { - var propertyValue = _typeConverter.ConvertValue(value, property, options); - - return propertyValue; - } - catch (Exception e) - { - throw new SettingsPropertyValueException(settingsType, value, property, e); - } - } - } -} \ No newline at end of file +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings +{ + internal class ValuesPopulator : IValuesPopulator + { + private readonly ITypePropertiesExtractor _typePropertiesExtractor; + private readonly ITypeConverter _typeConverter; + + // One plan per settings interface, reused across every populate. Instance field (not static): a plan + // bakes in this builder's SettingsOptions (section formatter, converters, per-property converters), and + // exactly one ValuesPopulator is created per SettingsBuilder, so Options is fixed for this cache's + // lifetime. Same reasoning as the TypePropertiesExtractor cache (P2). + private readonly ConcurrentDictionary _plans = new(); + + public ValuesPopulator() : + this(new TypePropertiesExtractor(), new TypeConverter()) + { + } + + internal ValuesPopulator(ITypePropertiesExtractor typePropertiesExtractor, ITypeConverter typeConverter) + { + _typePropertiesExtractor = typePropertiesExtractor; + _typeConverter = typeConverter; + } + + public void PopulateInstanceWithValues(object instance, + Type settings, + SettingsOptions options, + IEnumerable binders) + { + var sectionBinders = binders as ISectionBinder[] ?? binders.ToArray(); + + var plan = GetOrBuildPlan(settings, options); + + foreach (var propertyPlan in plan.Properties) + { + var tempValue = propertyPlan.DefaultValue; + + foreach (var binder in sectionBinders) + { + var context = new BindingContext(plan.SectionName, + propertyPlan.Key, + settings, + propertyPlan.Property, + tempValue); + + try + { + binder.BindPropertySettings(context); + if (context.HasNewValue) + tempValue = context.NewValue; + } + catch (Exception e) + { + throw new SettingsBindingException(binder, context, e); + } + } + + var propertyValue = ConvertPropertyValue(settings, tempValue, propertyPlan); + propertyPlan.Property.SetValue(instance, propertyValue); + } + } + + private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) + { + if (_plans.TryGetValue(settings, out var existing)) + return existing; + + var extracted = _typePropertiesExtractor.ExtractTypeProperties(settings); + var properties = extracted as PropertyInfo[] ?? extracted.ToArray(); + + var propertyPlans = new PropertyPlan[properties.Length]; + for (var i = 0; i < properties.Length; i++) + { + var property = properties[i]; + propertyPlans[i] = new PropertyPlan(property, + property.GetPropertyName(), + property.GetDefaultValue(), + _typeConverter.CreateConversion(property, options)); + } + + var plan = new SettingsPlan(settings, options, propertyPlans); + + // Concurrent builds would produce equivalent plans, so last-writer-wins is harmless. + _plans[settings] = plan; + return plan; + } + + private static object? ConvertPropertyValue(Type settingsType, object? value, PropertyPlan propertyPlan) + { + try + { + return propertyPlan.Conversion.Convert(value); + } + catch (Exception e) + { + throw new SettingsPropertyValueException(settingsType, value, propertyPlan.Property, e); + } + } + } +} diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs new file mode 100644 index 0000000..3623d87 --- /dev/null +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs @@ -0,0 +1,54 @@ +using System; +using BenchmarkDotNet.Attributes; +using ExistForAll.SimpleSettings.Binder; + +namespace ExistForAll.SimpleSettings.Benchmark +{ + /// + /// P3 — warm re-populate of a settings type through a primed builder, scaled by property count. The impl + /// type is emitted and the per-type SettingsPlan (section name resolved once, per-property keys + + /// defaults + converters cached, and the generated __Set bound as a delegate) are built once in + /// setup, so each measured call pays only: allocate the instance + run the populate off the cached plan. + /// This is the hot path P3 targets — repeated resolves that previously re-read attributes, rescanned the + /// converter list, rebuilt the section name, and set each property reflectively. It mirrors + /// ResolveBenchmark.WarmResolve_Provider but is a dedicated, cold-noise-free entry so CI can gate + /// its allocations. (New benchmark: no gh-pages baseline yet, so it only starts alerting once master + /// records the first point.) + /// + [MemoryDiagnoser] + public class PlanPopulateBenchmark + { + [Params(1, 10, 50)] + public int PropertyCount; + + private Type _type = null!; + private SettingsBuilder _builder = null!; + + [GlobalSetup] + public void Setup() + { + _type = PropertyCount switch + { + 1 => typeof(IProps1), + 10 => typeof(IProps10), + 50 => typeof(IProps50), + _ => throw new ArgumentOutOfRangeException(nameof(PropertyCount)), + }; + + // Seed an in-memory value for every property so the whole bind + convert + set path runs. + // Section name mirrors the engine's default formatter (strip a leading 'I'). + var collection = new InMemoryCollection(); + var section = _type.Name[0] == 'I' ? _type.Name.Substring(1) : _type.Name; + foreach (var property in _type.GetProperties()) + collection.Add(section, property.Name, "value"); + + // Prime once: emit the impl type and build + cache the SettingsPlan. The measured calls then pay + // only the per-resolve populate cost, which is what P3 optimizes. + _builder = SettingsBuilder.CreateBuilder(f => { f.AddInMemoryCollection(collection); }); + _builder.GetSettings(_type); + } + + [Benchmark] + public object Populate() => _builder.GetSettings(_type); + } +} From 61162c3893d95457fce61e9b72f8fc9cfb3f11aa Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Sun, 12 Jul 2026 23:35:38 +0300 Subject: [PATCH 2/2] Apply code + perf review fixes to P3 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. --- FIX-PLAN.md | 6 ++-- SESSION-HANDOFF.md | 6 ++-- .../Conversion/ISettingsTypeConverter.cs | 8 +++++ .../Core/Reflection/ITypeConverter.cs | 2 +- .../Core/Reflection/PropertyInfoExtensions.cs | 14 -------- .../Core/Reflection/TypeConverter.cs | 3 +- .../Core/Reflection/TypeExtensions.cs | 11 +------ .../SettingsBuilder.cs | 5 ++- .../ValuesPopulator.cs | 33 +++++++++++++++---- .../PlanPopulateBenchmark.cs | 4 +-- 10 files changed, 51 insertions(+), 41 deletions(-) delete mode 100644 src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs diff --git a/FIX-PLAN.md b/FIX-PLAN.md index 5cdee75..cb1b0db 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -6,7 +6,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - **Done & merged:** B1, B2, B4, B5, B9 + T1, T2 (PR #8) · BindingContext test (#10) · D3 namespace typo (#11) · T3 DI integration tests (#12) · solution rename (#13) · **A2 naming → ExistForAll (#15)** · **P0 benchmark harness (#16)** · **P1 provider cache + C3 decided/implemented (#17)** · **P2 memoize `ExtractTypeProperties` + `HashSet` dedup (#18)** · **docs tutorials refresh (#20)** · **Q1–Q4 perf quick wins + M1 collision fix + micro-benchmarks (#21)**. - **Q1–Q4 proven** via isolated micro-benchmarks (macro `ScanBenchmark` can't resolve them): Q1 2.7× / 64 KB→88 B · Q3 2.65× / 152 B→0 · Q4 32× / 224 B→0. **Q5 was already resolved by B4.** **M1** (code-review finding): namespace-qualify the generated impl name in the generator only — `GetNormalizeInterfaceName` also backs the section name. Suite → **56 per TFM**. - **Merged since:** **benchmark-tracking CI (#22)** — BDN on push/PR, gates PRs on **allocation** regressions (>10%) via github-action-benchmark on `gh-pages`; time informational. · **session-wrap docs (#23)**. -- **In flight:** **P3 — cached "settings plan"** (`SettingsPlan` per type: section name resolved once + lazily, per-property key/default/converter precomputed). Warm re-populate **−52–56%** allocations (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.7%** (under the 10% gate). The emitted/compiled setter was built and measured but **reverted** — it regressed the gated cold scan **+25%** for **no** warm gain (net10 reflective `SetValue` is already allocation-free for the args). A new gated `PlanPopulateBenchmark` tracks the warm path. +- **In flight:** **P3 — cached "settings plan"** (`SettingsPlan` per type: section name resolved once + lazily, per-property key/default/converter precomputed, `[SettingsProperty]` read **once** per property). Warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)** — after a code+perf review pass, consolidating the 3× attribute read more than paid for the plan objects. The emitted/compiled setter was built and measured but **reverted** — it regressed the gated cold scan **+25%** for **no** warm gain (net10 reflective `SetValue` is already allocation-free for the args). A new gated `PlanPopulateBenchmark` tracks the warm path. - **Next:** P4 (de-reflect array/enumerable converters) → P5 (resolve config section once per type). - **C3 — DECIDED (option 2):** cache in the provider only; Core `SettingsBuilder.GetSettings` unchanged; no reload. See #17. - **Held — do NOT delete (feature work coming):** D1 Validations (reconcile with the `validate-settings` branch) · D2 EqualityCompererCreator. @@ -209,7 +209,9 @@ if (settingsOptions.AttributeType != null && ### P3 · Cached compiled “settings plan” — Sev High · Eff L (biggest ceiling) The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55`) is reflection-saturated: reflective `property.SetValue` (`:55`, boxes value types); `GetSectionName` recomputed inside both loops though it’s constant per type; `GetPropertyName` recomputed per binder; `SettingsPropertyAttribute` read ~3×/property (`:36-40` + `TypeConverter.cs:36`). Build a per-interface `SettingsPlan` once (`ConcurrentDictionary`) holding: section name (once), and per property — resolved key, default value, chosen converter, and a **compiled setter** (emit the populate into the generated class in `PropertyCreator`, or a compiled `Action`). Folds in converter caching (below). -**Resolved (this PR):** `SettingsPlan` cached per type on the `ValuesPopulator` instance — section name resolved once and **lazily** (a no-binder scan never pays for it), per-property `PropertyPlan`/`PropertyConversion` as `readonly struct`s (one array alloc, no per-property object), converter chosen once (manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −52–56%** (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.66%**. The **compiled setter was dropped**: both variants (emit `__Set` into the generated type, and a compiled `Action`) regressed the *gated* cold `ScanBenchmark` (+25% for the emitted `__Set`) because the extra per-type codegen isn't amortized on a populate-once scan — and it bought **nothing** on the warm path, since net10's reflective `PropertyInfo.SetValue` no longer allocates an args array. **Follow-up (P3b, if ever needed):** tiered/lazy setter compilation (compile on the 2nd+ populate) would keep the cold scan flat while de-reflecting the hot path; only worth it if a future profile shows `SetValue` time (not allocation) matters. +**Resolved (this PR):** `SettingsPlan` cached per type on the `ValuesPopulator` instance — section name resolved once and **lazily** (a no-binder scan never pays for it), per-property `PropertyPlan`/`PropertyConversion` as `readonly struct`s (one array alloc, no per-property object), converter chosen once (manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed), and `[SettingsProperty]` read **once** per property then threaded into key/default/conversion (was 3× via `GetPropertyName`/`GetDefaultValue`/`CreateConversion`). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. The **compiled setter was dropped**: both variants (emit `__Set` into the generated type, and a compiled `Action`) regressed the *gated* cold `ScanBenchmark` (+25% for the emitted `__Set`) because the extra per-type codegen isn't amortized on a populate-once scan — and it bought **nothing** on the warm path, since net10's reflective `PropertyInfo.SetValue` no longer allocates an args array. A **code + perf review pass** (two agents) confirmed behavior/exception parity and drove the attribute-read consolidation + binder-array materialization; it also flagged **exception wrapping** (converter-setup failures at plan build are re-wrapped as `SettingsPropertyValueException`) and two follow-ups below. + +**Follow-ups from the review:** (a) **P3b** — tiered/lazy setter compilation (compile on the 2nd+ populate) would de-reflect the hot path without regressing the cold scan; only worth it if a profile shows `SetValue` *time* (not allocation) matters. (b) **Binder key alloc** — `InMemoryCollection.CreateKey` (`Binder/InMemoryCollection.cs:27`) concatenates `section + ":" + key` per lookup (~⅓ of the warm populate number); a `ValueTuple` dictionary key would make lookups allocation-free and remove the `"a:b"` collision ambiguity (same class of fix as Q3). Not a P3 regression — pre-existing binder cost. ### Quick wins — Eff S each - **Q1** `SettingsCollection.GetEnumerator` (`SettingsCollection.cs:52`) rebuilds a whole `Dictionary` per enumeration → `yield return` over the existing dictionary. diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index eb43f38..dc48ed1 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,13 +3,13 @@ _Last updated: 2026-07-12 · owner: Guy Ludvig (guy@frontegg.com)_ ## TL;DR -We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P2 + quick wins Q1–Q4** (merged), plus a namespace-collision fix (**M1**) and **isolated micro-benchmarks that prove the wins** (2.7×–32× on the repeated paths, allocations eliminated). A **per-push benchmark-tracking CI** that gates PRs on allocation regressions is **merged and live** (#22). **P3 — cached "settings plan" — is done (this PR, branch `perf/p3-compiled-settings-plan`):** warm re-populate **−52–56%** allocations (50 props 15,681→6,848 B), gated `ScanBenchmark` **+4.7%** (under the 10% gate). Suite green — **56 tests on net10.0** locally (CI runs net8.0 + net10.0). +We're working the three-specialist review fix plan (**`FIX-PLAN.md`**, repo root — per-item file:line detail). The performance track is through **P2 + quick wins Q1–Q4** (merged), plus a namespace-collision fix (**M1**) and **isolated micro-benchmarks that prove the wins** (2.7×–32× on the repeated paths, allocations eliminated). A **per-push benchmark-tracking CI** that gates PRs on allocation regressions is **merged and live** (#22). **P3 — cached "settings plan" — is done + review-hardened (this PR, branch `perf/p3-compiled-settings-plan`):** warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B), gated `ScanBenchmark` **≈flat (−0.4%)**. A code+perf review pass (two agents) confirmed behavior/exception parity and drove the final refinements. Suite green — **56 tests on net10.0** locally (CI runs net8.0 + net10.0). Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. ## Do this first (new session) 1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. -2. **Merge the P3 PR** (`perf/p3-compiled-settings-plan`) if still open + green — via the `guy-lud` identity. Confirm the benchmark check passed (scan ≈ +4.7%, under the 10% gate). Then `git checkout master && git pull`. +2. **Merge the P3 PR** (`perf/p3-compiled-settings-plan`) if still open + green — via the `guy-lud` identity. Confirm the benchmark check passed (scan ≈ flat / −0.4%). Then `git checkout master && git pull`. 3. Start **P4** (next perf item) on a fresh work branch. **Refresh this handoff on that branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state @@ -17,7 +17,7 @@ Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking chan - A **`gh-pages`** branch was bootstrapped to hold benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there (first recorded on the #22 master run). Otherwise the remote holds only **legacy / held** branches (`validate-settings`, `version-7.x`, older pre-#8 feature branches). Deleting remote branches needs the `guy-lud` push identity. ## What shipped (recent → older) -- **P3 — cached "settings plan" (this PR).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it), and per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −52–56%** (50 props 15,681→6,848 B); gated `ScanBenchmark` **+4.66%** (under the 10% gate). New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. Follow-up **P3b** (tiered/lazy setter compilation) noted in `FIX-PLAN.md` only if a future profile shows set *time* matters. +- **P3 — cached "settings plan" (this PR).** `ValuesPopulator` builds a per-type `SettingsPlan` once (cached on the populator instance): section name resolved **once and lazily** (a no-binder scan never pays for it); `[SettingsProperty]` read **once** per property then threaded into key/default/conversion; per property a `readonly struct` `PropertyPlan`/`PropertyConversion` carrying the resolved key, default, and **precomputed converter** (chosen via a manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. New gated `PlanPopulateBenchmark` tracks the warm path. **Reflective `SetValue` kept:** the emitted `__Set`/compiled-`Action` setter was built + measured but **reverted** — it regressed the gated cold scan (+25% for `__Set`) for **zero** warm gain, since net10's `PropertyInfo.SetValue` no longer allocates an args array. **Reviewed** by the dotnet code-review + perf agents (behavior/exception parity confirmed); their fixes: single attribute read, `SettingsPropertyValueException` re-wrap at plan build, binder-array materialization, `ISettingsTypeConverter` stateless/thread-safe doc. Follow-ups in `FIX-PLAN.md`: **P3b** (tiered/lazy setter, only if set *time* matters) and the binder `CreateKey` string-concat (pre-existing, ~⅓ of the warm number). - **#23 — session-wrap docs.** Handoff + fix-plan refresh; gitignored BenchmarkDotNet output + the personal `.claude/settings.local.json` (SessionStart hook). Squash-merged onto `master`. - **#22 — benchmark-tracking CI merged.** (Detail below.) First master run recorded the allocation baseline on `gh-pages`. - **#21 — Perf quick wins Q1–Q4 + M1 fix + micro-benchmarks.** diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs index b815477..69c46f0 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs @@ -2,6 +2,14 @@ namespace ExistForAll.SimpleSettings.Conversion { + /// + /// Converts a bound settings value to a property's target type. + /// + /// + /// Implementations must be stateless and thread-safe: a converter is selected once per settings + /// type and reused for every resolution of that type, including concurrent ones. Do not hold per-call + /// state on the instance. + /// public interface ISettingsTypeConverter { bool CanConvert(Type settingsType); diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs index 443c197..5f6c093 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs @@ -5,6 +5,6 @@ namespace ExistForAll.SimpleSettings.Core.Reflection { internal interface ITypeConverter { - PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsOptions options); + PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPropertyAttribute? attribute, SettingsOptions options); } } diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs deleted file mode 100644 index 7b5efea..0000000 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs +++ /dev/null @@ -1,14 +0,0 @@ -using System.Reflection; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal static class PropertyInfoExtensions - { - public static object? GetDefaultValue(this PropertyInfo property) - { - var attribute = property.GetCustomAttribute(); - - return attribute?.DefaultValue; - } - } -} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs index 8d03c1c..aa25138 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs @@ -11,10 +11,9 @@ internal class TypeConverter : ITypeConverter // type once nullability is stripped, and what a null bound value should yield. Previously all of this // (including up to three SettingsPropertyAttribute reads and a linear converter scan) ran on every // single populate; now it runs once per type and the result is cached in the settings plan. - public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsOptions options) + public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPropertyAttribute? attribute, SettingsOptions options) { var propertyType = propertyInfo.PropertyType; - var attribute = propertyInfo.GetCustomAttribute(); var throwOnNull = attribute != null && !attribute.AllowEmpty; var nullResult = CreateNullResult(propertyType); diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs index dfdccdc..c07f573 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs @@ -8,7 +8,7 @@ internal static class TypeExtensions { public static string GetNormalizeInterfaceName(this Type target) { - return target.Name[0] == 'I' ? $"{target.Name.Substring(1)}" : target.Name; + return target.Name[0] == 'I' ? target.Name.Substring(1) : target.Name; } public static string GetSectionName(this Type settingsClass, SettingsOptions options) @@ -20,15 +20,6 @@ public static string GetSectionName(this Type settingsClass, SettingsOptions opt : options.SectionNameFormatter(settingsClass); } - public static string GetPropertyName(this PropertyInfo propertyInfo) - { - var attribute = propertyInfo.GetCustomAttribute(true); - - return !string.IsNullOrWhiteSpace(attribute?.Name) - ? attribute.Name - : propertyInfo.Name; - } - public static bool IsEnumerable(this Type type) { var info = type.GetTypeInfo(); diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs index fb4f0bd..6acf3ae 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Reflection; using ExistForAll.SimpleSettings.Core; using ExistForAll.SimpleSettings.Core.Reflection; @@ -34,7 +35,9 @@ internal SettingsBuilder(SettingsOptions options, IValuesPopulator valuesPopulator) { Options = options; - SectionBinders = sectionBinders; + // Materialize once so the populator's `binders as ISectionBinder[]` fast-path hits instead of + // re-allocating via ToArray on every populate (the factory hands us a SortedList.Values view). + SectionBinders = sectionBinders as ISectionBinder[] ?? sectionBinders.ToArray(); _settingsTypesExtractor = settingsTypesExtractor; _settingsOptionsValidator = settingsOptionsValidator; _settingsClassGenerator = settingsClassGenerator; diff --git a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs index 94a66b1..d873112 100644 --- a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +++ b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs @@ -79,20 +79,41 @@ private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) for (var i = 0; i < properties.Length; i++) { var property = properties[i]; - propertyPlans[i] = new PropertyPlan(property, - property.GetPropertyName(), - property.GetDefaultValue(), - _typeConverter.CreateConversion(property, options)); + + // Read [SettingsProperty] once and thread it into key/default/conversion — GetCustomAttribute + // re-materializes the attribute + its backing array on every call, and this runs per property + // for every type at cold-scan scale. inherit:true matches the prior key-resolution behavior + // (a no-op for interface members, which is all settings types are). + var attribute = property.GetCustomAttribute(inherit: true); + + try + { + var key = !string.IsNullOrWhiteSpace(attribute?.Name) ? attribute.Name : property.Name; + + propertyPlans[i] = new PropertyPlan(property, + key, + attribute?.DefaultValue, + _typeConverter.CreateConversion(property, attribute, options)); + } + catch (Exception e) + { + // Restore the original exception contract: converter-setup failures used to surface inside + // the per-populate convert try as SettingsPropertyValueException. The bound value isn't + // known at plan build, hence null. + throw new SettingsPropertyValueException(settings, null, property, e); + } } var plan = new SettingsPlan(settings, options, propertyPlans); - // Concurrent builds would produce equivalent plans, so last-writer-wins is harmless. + // Concurrent builds would produce equivalent plans, so last-writer-wins is harmless. A build that + // throws is never cached (we don't reach here), so the next call retries — matching the old + // re-throw-every-time behavior. _plans[settings] = plan; return plan; } - private static object? ConvertPropertyValue(Type settingsType, object? value, PropertyPlan propertyPlan) + private static object? ConvertPropertyValue(Type settingsType, object? value, in PropertyPlan propertyPlan) { try { diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs index 3623d87..f6f48ed 100644 --- a/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs @@ -7,8 +7,8 @@ namespace ExistForAll.SimpleSettings.Benchmark /// /// P3 — warm re-populate of a settings type through a primed builder, scaled by property count. The impl /// type is emitted and the per-type SettingsPlan (section name resolved once, per-property keys + - /// defaults + converters cached, and the generated __Set bound as a delegate) are built once in - /// setup, so each measured call pays only: allocate the instance + run the populate off the cached plan. + /// defaults + converters cached) are built once in setup, so each measured call pays only: allocate the + /// instance + run the populate off the cached plan. /// This is the hot path P3 targets — repeated resolves that previously re-read attributes, rescanned the /// converter list, rebuilt the section name, and set each property reflectively. It mirrors /// ResolveBenchmark.WarmResolve_Provider but is a dedicated, cold-noise-free entry so CI can gate