From 6feb440ba6a99e546c498ad4282d9e948f41d7ba Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Sun, 12 Jul 2026 22:07:13 +0300 Subject: [PATCH 1/2] Session wrap: refresh handoff + fix-plan; gitignore BenchmarkDotNet output MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Handoff/fix-plan now reflect #21 merged (Q1–Q4 + M1 collision fix + micro-benchmarks, proven 2.7x–32x on repeated paths) and #22 open (per-push benchmark tracking, gate on allocation regressions). - Records the durable facts: gate on allocations not time, gh-pages holds the baseline, and the M1 rule (generated impl name is separate from the section name). - Next priority remains P3. - gitignore BenchmarkDotNet.Artifacts/ so local benchmark runs don't leave untracked output. --- .gitignore | 5 ++++- FIX-PLAN.md | 5 +++-- SESSION-HANDOFF.md | 53 +++++++++++++++++++++++++++++----------------- 3 files changed, 40 insertions(+), 23 deletions(-) diff --git a/.gitignore b/.gitignore index 2afb9d9..ab4f8d0 100644 --- a/.gitignore +++ b/.gitignore @@ -310,4 +310,7 @@ tools/ /src/.vscode/ /src/params.json /src/params.txt -/.vscode/ +/.vscode/ + +# BenchmarkDotNet output — local benchmark runs leave this under src/ (CI uses --artifacts) +BenchmarkDotNet.Artifacts/ diff --git a/FIX-PLAN.md b/FIX-PLAN.md index 98ef53d..f1fc599 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -3,8 +3,9 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · performance). Every finding below was verified against source with file:line. Work items are self-contained and ordered so they can be implemented one at a time._ ## 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)**. -- **In flight:** **Q1–Q4 perf quick wins** (current PR) — GetEnumerator `yield`, `OrdinalIgnoreCase` suffix match, env-binder fast path, generated-type cache. **Q5 was already resolved by B4** (the dead null-checks are gone). Suite → **55 per TFM** (net8.0 + net10.0). +- **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. - **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. diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 8e84401..063abd6 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,44 +3,57 @@ _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 — read it for per-item file:line detail). The performance track is now through **P2 + quick wins Q1–Q4**, the `docs/` tutorials are refreshed, and every merged workstream branch is pruned. **`master` is clean with no open PRs** once the Q1–Q4 perf PR merges. Suite is **green — 55 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 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). 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). + ## Current state -- On **`master`** (the Q1–Q4 perf PR merge — **verify with `git log` first**; this file can lag reality). Clean tree, no open PRs. Next work branches off `master`. -- Remote now holds only **legacy / held** branches (`validate-settings`, `version-7.x`, and older pre-#8 feature branches); the whole #8–#20 workstream was pruned. Deleting remote branches needs the `guy-lud` push identity. +- 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. ## What shipped (recent → older) -- **Perf quick wins Q1–Q4** (current PR). Q1 `SettingsCollection.GetEnumerator` now yields over its backing dictionary (was rebuilding a whole `Dictionary` per enumeration); Q2 `SettingsTypesExtractor` suffix match → `EndsWith(…, OrdinalIgnoreCase)` with the trimmed suffix hoisted out of the per-type predicate (also kills a `ToLower` CurrentCulture smell); Q3 `EnvironmentVariableBinder` fast-paths `context.Key` when there's no prefix/formatter (no `StringBuilder`) and does a single `IDictionary` lookup; Q4 `SettingsClassGenerator` caches the generated impl by interface `Type` (`ConcurrentDictionary`) instead of a per-call mangled-name `Assembly.GetType`. **Q5 was already resolved by B4** (the dead null-checks are gone). +3 regression tests → 55/TFM. +- **#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. - **#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` and replaced its O(n²) inherited-dedup with a single `HashSet` pass. The cache is a **private instance field** on the extractor — not static, not injected (per review). -- **#17 — P1 + C3.** `ISettingsProvider.GetSettings` used to re-bind a fresh instance on every call while DI registered startup-built singletons (the C3 divergence). The provider now serves the startup-built `ISettingsCollection` — the *same* instance as the DI singleton — falling back to a build only for never-scanned types. **C3 contract = cache in the provider only** (Core's public `SettingsBuilder.GetSettings` unchanged; no reload — settings are immutable snapshots). + regression test (both paths `ReferenceEquals`). -- **#16 — P0 benchmark harness.** Rebuilt the benchmark into a `[MemoryDiagnoser]` BenchmarkDotNet harness: `ScanBenchmark.ColdScan`; `ResolveBenchmark` (`[Params]` 1/10/50 · `ColdBuild` / `WarmResolve_Provider` / `WarmResolve_DiSingleton`); `ShapeBenchmark` (typed / array / deep-hierarchy). Run: `dotnet run -c Release --project src/performance/ExistForAll.SimpleSettings.Benchmark` (fast smoke: append `-- --job dry`). Baseline numbers intentionally NOT committed (machine-specific). -- **#15 — A2 naming consolidation.** All code/projects now spell the org **`ExistForAll`** (was a mix of `ExistAll` / `ExistsForAll`). Renamed the Binders folder (csproj + package id unchanged), the benchmark project, `Company` / `PackageTags`, and the README brand line. -- **#13 — solution file** `ExistAll.SimpleConfig.slnx` → `SimpleSettings.slnx` (first half of A2); the workflows now reference it through a `SOLUTION` env var. -- Earlier: #8 (five correctness bugfixes + tests), #10 (BindingContext test), #11 (D3 namespace typo), #12 (DI integration tests), #14 (fix plan + prior handoff). +- **#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). +- **#16 — P0 benchmark harness** (`[MemoryDiagnoser]` BDN: `ScanBenchmark` / `ResolveBenchmark` / `ShapeBenchmark`). Run: `dotnet run -c Release --project src/performance/ExistForAll.SimpleSettings.Benchmark` (smoke: `-- --job dry`). +- Earlier: #8 (5 bugfixes + tests), #10–#15 (tests, D3 typo, DI tests, solution rename, A2 naming). ## Key decisions & context (carry forward) -- **C3 resolved — option 2 (provider-level cache).** Core builder unchanged; no runtime reload. A reload / `IOptionsMonitor`-style path (change tokens) is the future "option 3" feature if it's ever wanted. -- **Validations (D1) — HELD, do NOT delete.** The public `Validations/*` namespace + `SettingsPropertyAttribute.ValidatorType` are dead but intended for an upcoming feature; reconcile with the existing **`validate-settings`** branch on origin. Wire into `ValuesPopulator` (run validators after binding, aggregate errors, throw typed). -- **`EqualityCompererCreator` (D2) — HELD** (internal, dead, latent invalid-IL bug at `EqualityCompererCreator.cs:38`). Delete only if value-equality on generated types is unwanted. -- **Pre-stable window:** no `v*` stable tag (the `version-*` tags are the dead legacy `ExistAll.SimpleConfig` package). Breaking changes free until the first `v2.0.0-beta`. +- **Benchmark tracking gates on ALLOCATIONS, not time.** Allocated bytes are deterministic → stable on shared CI runners → safe to fail a build on. Time is far too noisy to gate (this is also why the macro `ScanBenchmark` time didn't move for the quick wins). `gh-pages` (`dev/bench/`) holds the historical baseline. +- **M1 / generated names.** The generated impl type name (in `SettingsClassGenerator`) is namespace-qualified and must stay **separate** from `GetNormalizeInterfaceName`, which drives the default config section name. Don't merge them. +- **C3 resolved — option 2 (provider-level cache).** Reload / `IOptionsMonitor` is the future "option 3" if ever wanted. +- **Validations (D1) — HELD, do NOT delete.** Public `Validations/*` + `SettingsPropertyAttribute.ValidatorType` are dead but intended for a feature; reconcile with the `validate-settings` branch. Wire into `ValuesPopulator`. +- **`EqualityCompererCreator` (D2) — HELD** (internal, dead, latent invalid-IL bug at `EqualityCompererCreator.cs:38`). +- **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:** **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, not per property). -2. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, T6 converters, T7 generator concurrency stress — note the unsynchronized check-then-`DefineType` in `SettingsClassGenerator.GenerateType`: the Q4 `ConcurrentDictionary` made the cache thread-safe but did **not** close that generation race (still a T7 item). -3. **Architecture:** A1 (AOT/trim `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` — HIGH, it's a `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 — give the repo/brand links a pass (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. +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. ## 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). +- **`benchmark.yml`** — on push to `master` + PRs: runs BDN, gates PRs on allocation regressions (see Key decisions). - **Versioning = MinVer**, tag prefix `v`, baseline **2.0.0**, keyless publish via NuGet Trusted Publishing (OIDC). First real release: Actions → Release → `channel: beta` (→ `v2.0.0-beta.1`); use `dry_run` first. -- Both workflows invoke the solution through the `SOLUTION` env var (**`SimpleSettings.slnx`**). +- Workflows invoke the solution through the `SOLUTION` env var (**`SimpleSettings.slnx`**). **Any push to `master` publishes an alpha** — so everything goes through PRs. ## Gotchas a new session MUST know - **Pushing / PRs:** the active `git`/`gh` identity (`guy-frontegg`) is **read-only** on this repo; push/PR/merge via the **`guy-lud`** account — SSH alias `github-guy-lud` for `git push`, `gh auth switch --user guy-lud` for `gh` writes (switch back to `guy-frontegg` after). Full recipe in the assistant's private project memory (`simplesettings-push-access`). - **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. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). From 3aa1b7728f94a89fc28a237ad5719fdbe431f64e Mon Sep 17 00:00:00 2001 From: Guy Ludvig Date: Sun, 12 Jul 2026 22:10:48 +0300 Subject: [PATCH 2/2] gitignore .claude/settings.local.json (personal SessionStart hook) --- .gitignore | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.gitignore b/.gitignore index ab4f8d0..2a406c6 100644 --- a/.gitignore +++ b/.gitignore @@ -314,3 +314,6 @@ tools/ # BenchmarkDotNet output — local benchmark runs leave this under src/ (CI uses --artifacts) BenchmarkDotNet.Artifacts/ + +# Personal per-clone Claude Code settings (e.g. the SessionStart handoff hook) +.claude/settings.local.json