From 0ccdff7746878785c2f93fb538c3b6201df993e9 Mon Sep 17 00:00:00 2001 From: guy-lud Date: Mon, 13 Jul 2026 15:04:40 +0300 Subject: [PATCH] Resolve the config section once per type (P5) ConfigurationBinder called _configuration.GetSection(...) on every property, though the section is constant per settings type. Cache the resolved IConfigurationSection per section name in a ConcurrentDictionary, using a zero-capture GetOrAdd (static factory + factoryArgument) so no per-call delegate is allocated. Reload-safe: GetSection returns a live view over the configuration root, so the cached section re-reads providers on each access. Drop the dead ?. (GetSection never returns null) and strip a stray BOM. Chosen over threading the section through the ISectionBinder contract: that would be a layering violation (Core must not reference Microsoft.Extensions.Configuration) and the optimization is single-implementer. Plan reviewed by the architect/perf/security agents; code reviewed via /code-review. Proof (new gated ConfigBinderBenchmark): BindNoRoot 80->40 B (-50%), BindWithRoot 144->56 B (-61%). Adds 3 tests (multi-property with/without RootSection, plus a cached-section-reflects-later-change live-view test). Also wires P4's ConvertArrayBenchmark into the CI filter (it was never gated) and adds Microsoft.Extensions.Configuration to the benchmark project. Refreshes SESSION-HANDOFF.md + FIX-PLAN.md and logs a pre-existing secret-leak finding surfaced by the security review (S1: Resources.cs interpolates the raw bound value into the SettingsPropertyValueException message). Suite: 71 tests per TFM. --- .github/workflows/benchmark.yml | 2 +- FIX-PLAN.md | 15 ++-- SESSION-HANDOFF.md | 19 +++-- .../ConfigurationBinder.cs | 78 +++++++++-------- .../ConfigurationBinderCacheTests.cs | 84 +++++++++++++++++++ .../ConfigBinderBenchmark.cs | 51 +++++++++++ ...xistForAll.SimpleSettings.Benchmark.csproj | 1 + 7 files changed, 200 insertions(+), 50 deletions(-) create mode 100644 src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigurationBinderCacheTests.cs create mode 100644 src/performance/ExistForAll.SimpleSettings.Benchmark/ConfigBinderBenchmark.cs diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 86ed265..9c295a2 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -63,7 +63,7 @@ jobs: dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- - --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" "*PlanPopulateBenchmark*" + --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" "*PlanPopulateBenchmark*" "*ConvertArrayBenchmark*" "*ConfigBinderBenchmark*" --job short --exporters json --artifacts ${{ github.workspace }}/bdn diff --git a/FIX-PLAN.md b/FIX-PLAN.md index f390b46..c44ccb3 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -6,8 +6,9 @@ _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)** · **P3 — cached "settings plan" (#24)** — `SettingsPlan` per type (section name once+lazy, key/default/converter precomputed, `[SettingsProperty]` read once). Warm re-populate **−55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. Reviewed by code+perf agents; emitted/compiled setter reverted (regressed the gated cold scan for **no** warm gain — net10 `SetValue` is already alloc-free). New gated `PlanPopulateBenchmark` tracks the warm path. -- **In flight:** **P4 — de-reflect + DRY the array/enumerable converters** = **PR #25 open, code-reviewed clean** (branch `perf/p4-dereflect-converters`; awaiting CI + merge). New shared `CollectionTypeConverter` builds results via `Array.CreateInstance` + indexed fill and selects the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List`+reflected `Enumerable.ToArray`, no `First` closure. `TypeConverter.CreateNullResult` de-reflected too (`Array.CreateInstance(t,0)` for `Enumerable.Empty()`). Proven via new gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)**. Suite **68 per TFM** (+12 collection-converter parity tests). Branch also carries the pre-P4 doc refresh + post-P3 style tweaks to `TypeConverter.cs`/`TypeExtensions.cs`. -- **Next:** P5 (resolve config section once per type) → optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile). +- **P4 merged (#25)** — de-reflect + DRY the array/enumerable converters: shared `CollectionTypeConverter` (`Array.CreateInstance` + indexed fill, manual `LinkedList` walk); `CreateNullResult` de-reflected. Gated `ConvertArrayBenchmark`: **1.33 KB→688 B (−49%), 5.7×**. Suite 68/TFM. Included a user modernization pass (collection expressions across ~19 files). +- **In flight:** **P5 — resolve config section once per type** = **implemented + reviewed clean on branch `perf/p5-resolve-section-once`** (awaiting commit/PR at time of writing). `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`). Plan reviewed by architect+perf+security (chose internal cache over a contract change — layering); code reviewed via `/code-review`. Gated `ConfigBinderBenchmark`: **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**. Suite **71/TFM** (+3 parity/live-view tests). Also gated P4's `ConvertArrayBenchmark` (was never in the CI filter). **Security review surfaced a pre-existing leak → new item S1.** +- **Next:** S1 (redact secret from exception message) · optional P3b (tiered/lazy compiled setter, only if set *time* shows in a profile). - **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`. @@ -58,8 +59,9 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - [x] P2 · Memoize `ExtractTypeProperties` + fix O(n²) dedup · Sev High · Eff S - [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) -- [x] P4 · De-reflect + DRY array/enumerable converters (shared `CollectionTypeConverter`; `Array.CreateInstance` + manual converter walk; `CreateNullResult` de-reflected) — **1.33 KB→688 B, 5.7×**; branch `perf/p4-dereflect-converters` · Sev Med · Eff M -- [ ] P5 · Resolve config section once per type, not per property · Sev Med · Eff M +- [x] P4 · De-reflect + DRY array/enumerable converters (shared `CollectionTypeConverter`; `Array.CreateInstance` + manual converter walk; `CreateNullResult` de-reflected) — **1.33 KB→688 B, 5.7×**; merged #25 · Sev Med · Eff M +- [x] P5 · Resolve config section once per type — `ConfigurationBinder` caches the `IConfigurationSection` per section name (`ConcurrentDictionary`, zero-capture `GetOrAdd`) — **BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)**; branch `perf/p5-resolve-section-once` · Sev Med · Eff M +- [ ] S1 · Redact secret values from `SettingsPropertyValueException` message (`Resources.cs:34-36` interpolates the raw bound value → leaks secrets into logs); log type/length, not the value · Sev Med · Eff S · **found in P5 security review** **Phase 6 — Architecture strategy** - [ ] A1 · Decide AOT/trim story; annotate `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` and/or plan a source generator · Sev High · Eff M–L @@ -224,8 +226,9 @@ The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55 Was: `TypeConverter.cs` (empty enumerable via `Enumerable.Empty` `MakeGenericMethod().Invoke()`), `ArrayTypeConverter` (`Activator.CreateInstance(List<>)` + reflected `Enumerable.ToArray` `Invoke`), `EnumerableTypeConverter` (`Activator.CreateInstance(List<>)`), both selecting the element converter with LINQ `First` (boxes the `LinkedList` enumerator + a closure). Now: a shared `CollectionTypeConverter` base implements `Convert` once — normalize the value to an array (split delimited string / passthrough / wrap scalar), select the element converter by walking the concrete `LinkedList` (struct enumerator, no boxing/closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` are now thin subclasses differing only in `CanConvert` + element-type extraction; both return `T[]` (safe — `IsEnumerable()` matches only `IEnumerable`, which a `T[]` satisfies). `CreateNullResult` uses `Array.CreateInstance(t,0)` instead of the `Enumerable.Empty()` reflection. **Proof (`ConvertArrayBenchmark`, gated): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** 12 parity tests in `Conversion/CollectionConversionTests.cs` (int/string/enum/DateTime/Uri elements, empty-entry removal, custom delimiter, default passthrough, unbound→empty `T[]`, `T[]`-not-`List` guard, bad-element negative). Residual 688 B = irreducible split-substrings + element boxing + result array (shared by old & new). **Code-reviewed clean** (dotnet code-reviewer verified all parity claims; the enum/DateTime/Uri + null-path + negative tests were its suggestions — partially closes T6). -### P5 · Resolve config section once per type — Sev Med · Eff M -`ConfigurationBinder.BindPropertySettings` (`ConfigurationBinder.cs:25-33`) calls `_configuration.GetSection(...)` **per property**; the section is constant per type. Resolve the `IConfigurationSection` once per (type, section) — pass section context via the plan (P3) or cache per section string. Touches the binder/context contract. +### P5 · Resolve config section once per type — Sev Med · Eff M · **DONE (branch `perf/p5-resolve-section-once`)** +Was: `ConfigurationBinder.BindPropertySettings` called `_configuration.GetSection(...)` per property (a fresh `ConfigurationSection` alloc each time) + a `$"{RootSection}:{Section}"` interpolation per property when a root is set — though the section is constant per type. +Now: the binder caches the resolved `IConfigurationSection` per section name in a `private readonly ConcurrentDictionary`, via a **zero-capture** `GetOrAdd(context.Section, static (name, self) => self.ResolveSection(name), this)` (a capturing lambda would allocate a 64 B delegate per call — measured). Kept the internal-cache approach (Option 2), **not** a contract change: the three-specialist plan review found threading `IConfigurationSection` through the Core `ISectionBinder`/`BindingContext` contract to be a layering violation (Core must not reference `Microsoft.Extensions.Configuration`), and the optimization is single-implementer (env/cmdline/in-memory binders are flat lookups). Reload-safe: `GetSection` returns a live view, so the cached section re-reads providers on each access (locked by a test). Dropped the dead `?.` (`GetSection` never returns null); stripped a stray BOM. **Proof (`ConfigBinderBenchmark`, gated): BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)** — matched the perf review's predicted −40/−88 B deltas. 3 parity/live-view tests; also wired P4's `ConvertArrayBenchmark` into the CI filter (it was never gated). Reviewed clean (`/code-review`, after the `dotnet-claude-kit:code-reviewer` agent misfired 3×). --- diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 1213f1d..29c37a2 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,21 +3,24 @@ _Last updated: 2026-07-13 · 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 **P3** (merged, #24) plus quick wins Q1–Q4, a benchmark-tracking CI that gates PRs on allocation regressions (#22, live), and now **P4 — de-reflect + DRY the array/enumerable converters — SHIPPED as [PR #25](https://github.com/existall/SimpleSettings/pull/25)** (open, base `master`; **reviewed clean by the dotnet code-reviewer** — no correctness/security/concurrency/parity defects). A new shared `CollectionTypeConverter` builds collection results via `Array.CreateInstance` + indexed fill and picks the element converter by walking the concrete `LinkedList` (struct enumerator) — no `List` + reflected `Enumerable.ToArray`, no `First` closure; `TypeConverter.CreateNullResult` de-reflected too. **Proven via the new gated `ConvertArrayBenchmark`: 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×).** Suite green — **68 tests on net10.0** (+12 collection-converter parity tests; 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). Perf track: **P3 (#24) and P4 (#25) are MERGED**; **P5 — resolve the config section once per type — is implemented + reviewed clean on branch `perf/p5-resolve-section-once`, ready to commit + PR.** P5 makes `ConfigurationBinder` cache the resolved `IConfigurationSection` per section name (a `ConcurrentDictionary` with a **zero-capture** `GetOrAdd`) instead of calling `GetSection(...)` per property. Plan was reviewed up front by the architect / perf / security agents (chose the internal cache over a public-contract change — Core must not reference `Microsoft.Extensions.Configuration`); code reviewed via `/code-review`. **Proven via new gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%).** Suite green — **71 tests on net10.0** (+3 P5 parity/live-view tests; CI runs net8.0 + net10.0). (The benchmark-tracking CI (#22) gates PRs on allocation regressions.) Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now. (P4's `EnumerableTypeConverter` now returning `T[]` instead of `List` is safe regardless — `IsEnumerable()` matches only `IEnumerable`, which `T[]` satisfies.) ## Do this first (new session) -1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `faa48d9` (P3, #24) and **[PR #25](https://github.com/existall/SimpleSettings/pull/25) open** (P4, branch `perf/p4-dereflect-converters`). -2. **P4 is committed, pushed, PR'd (#25), and code-reviewed clean** — the only open item is watching CI (`build-test` + the `benchmark` allocation gate) and **merging #25** once green (merge via the `guy-lud` identity — see Gotchas). If CI is already green, merge it. -3. After #25 merges → **P5** (resolve config section once per type) on a fresh branch. Per the workflow the user set (see project memory `[[dotnet-review-workflow]]`): **plan first, review the plan with the `dotnet-architect` / `performance-analyst` / `security-auditor` agents, implement, then review the diff with `code-reviewer`.** **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). +1. **Verify git state** (`git log`, `gh pr list`) — this file can lag. Expect `master` @ `f9a3061` (P4, #25 merged) and a **P5 PR open** from branch `perf/p5-resolve-section-once` (or, if the wrap didn't finish, P5 sitting committed/uncommitted on that branch). +2. **P5 is implemented + reviewed clean.** If the PR isn't open yet: commit on `perf/p5-resolve-section-once`, push + open the PR via the **`guy-lud`** identity (see Gotchas). If it's open and CI is green, it's ready to **merge** (user merges — that's the alpha-publishing step). +3. After P5 merges → **S1** (redact the secret from the exception message — the P5 security review found it) or **P3b**. Per the workflow the user set (project memory `[[dotnet-review-workflow]]`): **plan first, review the plan with `dotnet-architect` / `performance-analyst` / `security-auditor`, implement, then review the diff with the code-reviewer** — note the `dotnet-claude-kit:code-reviewer` agent has been misfiring (returns a leaked skill/role preamble, 0 tool calls); the `/code-review` skill is the reliable fallback. **Refresh this handoff on the work branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state -- On branch **`perf/p4-dereflect-converters`** (off `master` @ `faa48d9`), commit `6d5bfbe` + a follow-up test-hardening commit. **P4 pushed and open as PR #25**, reviewed clean by the code-reviewer. Files: new `Conversion/CollectionTypeConverter.cs` + `Conversion/CollectionConversionTests.cs` (12 tests); rewritten `ArrayTypeConverter`/`EnumerableTypeConverter`; `TypeConverter.cs` (de-reflect + BOM strip); `MicroBenchmarks.cs` (+`ConvertArrayBenchmark`); doc refresh; pre-existing `TypeExtensions.cs` tweak. +- On branch **`perf/p5-resolve-section-once`** (off `master` @ `f9a3061`). **P5 implemented + reviewed clean, at wrap being committed/PR'd.** Changed: `Extensions.Binders/ConfigurationBinder.cs` (section cache + BOM strip + dropped dead `?.`); new `UnitTests/ConfigurationBinderCacheTests.cs` (3 tests); new `Benchmark/ConfigBinderBenchmark.cs`; `benchmark.yml` (+`ConfigBinderBenchmark`, +`ConvertArrayBenchmark`); benchmark `.csproj` (+`Microsoft.Extensions.Configuration`); doc refresh. +- **`master` @ `f9a3061`** now holds P4 (#25) **plus a user modernization pass** (collection expressions across ~19 files, merged with #25). +- A **`gh-pages`** branch holds benchmark data (`dev/bench/`); do **not** delete it — the baseline lives there. The remote also still has the merged **`perf/p4-dereflect-converters`** branch (GitHub didn't auto-delete; optional cleanup, needs `guy-lud`) plus legacy/held branches. Deleting remote branches needs the `guy-lud` push identity. - 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) -- **P4 — de-reflect + DRY the array/enumerable converters ([PR #25](https://github.com/existall/SimpleSettings/pull/25), open, code-reviewed clean).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **12 parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`/`DayOfWeek[]`/`DateTime[]`/`Uri[]`, empty-entry removal, custom delimiter, default-array passthrough, unbound-no-default→empty `T[]` (the `CreateNullResult` line), the enumerable-path-materializes-`T[]` guard, and a bad-element→`SettingsPropertyValueException` negative. Suite **68 net10** (was 56). **Code-reviewed** by the dotnet code-reviewer (the last 5 tests were its suggestions): all 6 adversarial parity claims verified, no defects. +- **P5 — resolve config section once per type (branch `perf/p5-resolve-section-once`; committing/PR'ing at wrap).** `ConfigurationBinder` now caches the resolved `IConfigurationSection` per section name in a `private readonly ConcurrentDictionary`, replacing the per-property `GetSection(...)`. The `GetOrAdd` uses the **zero-capture** `static (name, self) => self.ResolveSection(name), this` form — a capturing lambda would allocate a 64 B delegate per call (measured). **Internal cache, not a contract change:** the plan review (architect/perf/security) found threading `IConfigurationSection` through the Core `ISectionBinder`/`BindingContext` to be a layering violation (Core must not reference `Microsoft.Extensions.Configuration`) and the optimization is single-implementer (env/cmdline/in-memory binders are flat lookups). Reload-safe (cached section is a live view — re-reads providers each access; locked by a test). Dropped the dead `?.` (`GetSection` is non-null); stripped a stray BOM. **Proof — gated `ConfigBinderBenchmark`: BindNoRoot 80→40 B (−50%), BindWithRoot 144→56 B (−61%)** (matched the perf review's −40/−88 B prediction). 3 tests (multi-property no-root/with-root + `Bind_CachedSection_ReflectsLaterConfigChange` live-view). Also **wired P4's `ConvertArrayBenchmark` into the CI filter** — it was never gated. Suite **71 net10**. **Reviewed:** plan by the 3 specialists; code by `/code-review` (the `dotnet-claude-kit:code-reviewer` agent misfired 3× — leaked skill/role preamble, 0 tool calls). **Security review found a pre-existing leak → S1** (see Next priorities): `Resources.cs:34-36` interpolates the raw bound value into `SettingsPropertyValueException` → secrets in logs. +- **P4 — de-reflect + DRY the array/enumerable converters (merged, #25).** New abstract `Conversion/CollectionTypeConverter` owns the shared `Convert`: normalize the incoming value to an array (split a delimited string / passthrough an existing array / wrap a scalar), select the element converter by a **manual walk over the concrete `LinkedList`** (struct enumerator — no boxed enumerator, no predicate closure), then fill an `Array.CreateInstance(elementType, n)` by index. `ArrayTypeConverter`/`EnumerableTypeConverter` collapse to thin subclasses (only `CanConvert` + element-type extraction differ); both now return `T[]`. Gone: the `List` + its backing array, the reflected `Enumerable.ToArray` (`MakeGenericMethod`+`Invoke`+args array), and the `First` closure. `TypeConverter.CreateNullResult` swaps the `Enumerable.Empty()` reflection for `Array.CreateInstance(t,0)`, and its `GetConverter` is now a true manual walk (matching its own comment). **Proof — new gated `ConvertArrayBenchmark` (isolates the hot path like Q1/Q3/Q4): 1.33 KB→688 B (−49%), 1,247→219 ns (5.7×)** (before measured by swapping master's old converter back in). Residual 688 B is the split-substrings + per-element boxing + result array shared by both versions. **12 parity tests** (`Conversion/CollectionConversionTests.cs`): delimited→`int[]`/`string[]`/`IEnumerable`/`DayOfWeek[]`/`DateTime[]`/`Uri[]`, empty-entry removal, custom delimiter, default-array passthrough, unbound-no-default→empty `T[]` (the `CreateNullResult` line), the enumerable-path-materializes-`T[]` guard, and a bad-element→`SettingsPropertyValueException` negative. Suite **68 net10** (was 56). **Code-reviewed** by the dotnet code-reviewer (the last 5 tests were its suggestions): all 6 adversarial parity claims verified, no defects. - **P3 — cached "settings plan" (#24, merged `faa48d9`).** `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`. @@ -41,8 +44,8 @@ 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. **Perf:** **P4 is done (branch, uncommitted — commit + PR it first).** 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 — partially done in P4** (array/enumerable path covered by `CollectionConversionTests`; DateTime/Uri/Enum/Default still thin), 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). +1. **Finish P5:** commit + PR the branch (then user merges). After that the perf track's queued items are **S1** (security: redact the secret value from `SettingsPropertyValueException` — `Resources.cs:34-36`; found in the P5 security review) and 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 — largely done across P4+P5** (array/enumerable/element-type covered by `CollectionConversionTests`; ConfigurationBinder covered by `ConfigurationBinderCacheTests`), 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. diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs index 0b080b5..119747b 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs +++ b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/ConfigurationBinder.cs @@ -1,35 +1,43 @@ -using Microsoft.Extensions.Configuration; - -namespace ExistForAll.SimpleSettings.Binders -{ - public class ConfigurationBinder : ISectionBinder - { - public string? RootSection { get; } - - private readonly IConfiguration _configuration; - - public ConfigurationBinder(IConfiguration configuration, string? rootSection = null) - { - RootSection = rootSection; - _configuration = configuration; - } - - private string GetSection(BindingContext context) - { - if (string.IsNullOrWhiteSpace(RootSection)) - return context.Section; - - return $"{RootSection}:{context.Section}"; - } - - public void BindPropertySettings(BindingContext context) - { - var configurationSection = _configuration.GetSection(GetSection(context)); - - var value = configurationSection?[context.Key]; - - if (value != null) - context.SetNewValue(value); - } - } -} +using System.Collections.Concurrent; +using Microsoft.Extensions.Configuration; + +namespace ExistForAll.SimpleSettings.Binders +{ + public class ConfigurationBinder : ISectionBinder + { + public string? RootSection { get; } + + private readonly IConfiguration _configuration; + + // Resolve each config section once and reuse it across every property of every settings type. P3 makes + // context.Section the same per-type string, so all but the first property (and every repeated resolve) + // is a cache hit. Reload-safe: GetSection returns a live view over the configuration root, so indexing + // the cached section re-reads the providers on each access — a config reload is still reflected. Ordinal + // key; bounded by the number of settings sections, so no eviction is needed (same shape as the P1–P3 + // caches). + private readonly ConcurrentDictionary _sections = new(); + + public ConfigurationBinder(IConfiguration configuration, string? rootSection = null) + { + RootSection = rootSection; + _configuration = configuration; + } + + public void BindPropertySettings(BindingContext context) + { + // static factory + factoryArgument overload so no per-call closure over `this` is allocated — a + // capturing lambda would build a fresh delegate on every call and eat most of the win. + var section = _sections.GetOrAdd(context.Section, static (name, self) => self.ResolveSection(name), this); + + var value = section[context.Key]; + + if (value != null) + context.SetNewValue(value); + } + + private IConfigurationSection ResolveSection(string section) + { + return _configuration.GetSection(string.IsNullOrWhiteSpace(RootSection) ? section : $"{RootSection}:{section}"); + } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigurationBinderCacheTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigurationBinderCacheTests.cs new file mode 100644 index 0000000..5542e8a --- /dev/null +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/ConfigurationBinderCacheTests.cs @@ -0,0 +1,84 @@ +using System.Collections.Generic; +using ExistForAll.SimpleSettings.Binders; +using Microsoft.Extensions.Configuration; + +namespace ExistForAll.SimpleSettings.UnitTests +{ + // P5 — ConfigurationBinder resolves each section once and caches it per section name. These lock the + // observable behavior: every property of a type still binds from the right (cached) section, with and + // without a RootSection, and the cached section stays a live view over the configuration root (so a later + // value change is picked up — caching the section object must not freeze values). + public class ConfigurationBinderCacheTests + { + [Test] + public async Task Bind_MultipleProperties_AllResolveFromCachedSection() + { + var config = BuildConfig(new Dictionary + { + ["MultiProp:A"] = "a", + ["MultiProp:B"] = "b", + ["MultiProp:C"] = "c", + }); + + var result = Build(new ConfigurationBinder(config)); + + await Assert.That(result.A).IsEqualTo("a"); + await Assert.That(result.B).IsEqualTo("b"); + await Assert.That(result.C).IsEqualTo("c"); + } + + [Test] + public async Task Bind_WithRootSection_MultipleProperties_AllResolve() + { + var config = BuildConfig(new Dictionary + { + ["MyRoot:MultiProp:A"] = "a", + ["MyRoot:MultiProp:B"] = "b", + ["MyRoot:MultiProp:C"] = "c", + }); + + var result = Build(new ConfigurationBinder(config, "MyRoot")); + + await Assert.That(result.A).IsEqualTo("a"); + await Assert.That(result.B).IsEqualTo("b"); + await Assert.That(result.C).IsEqualTo("c"); + } + + [Test] + public async Task Bind_CachedSection_ReflectsLaterConfigChange() + { + // The section object is cached on the first populate; a later value change (the config indexer + // writes through to the provider) must still be seen on the next populate. This pins that caching + // the IConfigurationSection does not freeze values — it stays a live view over the root. + var config = BuildConfig(new Dictionary { ["CacheProbe:Value"] = "before" }); + + var builder = SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(new ConfigurationBinder(config))); + + var first = builder.GetSettings(); + await Assert.That(first.Value).IsEqualTo("before"); + + config["CacheProbe:Value"] = "after"; + + var second = builder.GetSettings(); + await Assert.That(second.Value).IsEqualTo("after"); + } + + private static IConfiguration BuildConfig(Dictionary data) + => new ConfigurationBuilder().AddInMemoryCollection(data).Build(); + + private static T Build(ISectionBinder binder) where T : class + => SettingsBuilder.CreateBuilder(x => x.AddSectionBinder(binder)).GetSettings(); + + public interface IMultiProp + { + string A { get; set; } + string B { get; set; } + string C { get; set; } + } + + public interface ICacheProbe + { + string Value { get; set; } + } + } +} diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/ConfigBinderBenchmark.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/ConfigBinderBenchmark.cs new file mode 100644 index 0000000..46e5587 --- /dev/null +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/ConfigBinderBenchmark.cs @@ -0,0 +1,51 @@ +using System.Collections.Generic; +using BenchmarkDotNet.Attributes; +using ExistForAll.SimpleSettings.Binders; +using Microsoft.Extensions.Configuration; + +namespace ExistForAll.SimpleSettings.Benchmark +{ + /// + /// P5 — ConfigurationBinder.BindPropertySettings. The old code called + /// IConfiguration.GetSection(...) (a fresh ConfigurationSection allocation) on every property, + /// plus a $"{RootSection}:{Section}" interpolation per property when a root is configured. The section + /// is constant per settings type, so P5 resolves it once and caches it per section name. Two variants + /// because only the root-set path pays the interpolation. The cache is primed in setup so the measured op is + /// a steady-state hit — the repeated-populate path P5 targets. Mirrors (Q3). + /// + [MemoryDiagnoser] + public class ConfigBinderBenchmark + { + private ConfigurationBinder _noRoot = null!; + private ConfigurationBinder _withRoot = null!; + private BindingContext _context = null!; + + [GlobalSetup] + public void Setup() + { + var config = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Props1:P0"] = "value", + ["Root:Props1:P0"] = "value", + }) + .Build(); + + _noRoot = new ConfigurationBinder(config); + _withRoot = new ConfigurationBinder(config, "Root"); + + var property = typeof(IProps1).GetProperty(nameof(IProps1.P0))!; + _context = new BindingContext("Props1", "P0", typeof(IProps1), property, null); + + // Prime the section cache so each measured call is a pure hit (a harmless no-op on the pre-P5 code). + _noRoot.BindPropertySettings(_context); + _withRoot.BindPropertySettings(_context); + } + + [Benchmark] + public void BindNoRoot() => _noRoot.BindPropertySettings(_context); + + [Benchmark] + public void BindWithRoot() => _withRoot.BindPropertySettings(_context); + } +} diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj b/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj index 245b8ab..0a44251 100644 --- a/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj @@ -9,6 +9,7 @@ +