Skip to content

Perf quick wins Q1–Q4 (+ handoff/fix-plan refresh) - #21

Merged
guy-lud merged 4 commits into
masterfrom
perf/quick-wins-q1-q5
Jul 12, 2026
Merged

Perf quick wins Q1–Q4 (+ handoff/fix-plan refresh)#21
guy-lud merged 4 commits into
masterfrom
perf/quick-wins-q1-q5

Conversation

@guy-lud

@guy-lud guy-lud commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Implements the FIX-PLAN perf quick wins. Five items were planned; Q5 was already resolved by B4 (#8) — the dead string.Equals(x, null, …) checks in BindingContext are gone — so this is Q1–Q4. Each is a small, behavior-preserving optimization with a regression test where behavior could subtly shift.

Changes

  • Q1 — SettingsCollection.GetEnumerator rebuilt a whole Dictionary via ToDictionary on every enumeration. Now yield returns over the backing dictionary — same pairs, same order, no per-enumeration allocation.
  • Q2 — SettingsTypesExtractor matched the suffix with Name.ToLower().EndsWith(suffix.Trim().ToLower()) (two ToLower allocations per scanned type, under CurrentCulture). Now EndsWith(suffix, StringComparison.OrdinalIgnoreCase) with the trimmed suffix hoisted once out of the per-type predicate. Also removes the culture-sensitivity smell. (Regression test: a type ending …SETTINGS still matches the default Settings suffix — a case-sensitive EndsWith would miss it.)
  • Q3 — EnvironmentVariableBinder allocated a StringBuilder per property and did a double lookup (Contains + indexer). Now fast-paths context.Key when there's no prefix and no formatter (the common case, zero allocation), and does a single IDictionary indexer read (returns null when absent; env values are never null when present). (Regression test added for the prefix branch.)
  • Q4 — SettingsClassGenerator re-queried the module by mangled type name (Assembly.GetType(name.Replace("+","\\+"))) on every GenerateType. Now caches the generated impl by interface Type in a ConcurrentDictionary<Type, Type>. (Regression test: the same interface returns the cached Type.)
    • Note: this makes the cache thread-safe but does not close the pre-existing unsynchronized check-then-DefineType race — that stays a T7 concurrency item.

Verification

  • Build clean on net8.0 + net10.0, 0 warnings.
  • 55/55 tests green on net10.0 (was 52; +3 new regression tests).

Also

guy-lud added 4 commits July 12, 2026 19:54
…–Q4)

Five FIX-PLAN quick wins; Q5 was already resolved by B4 (#8), so this is Q1–Q4.

- Q1 SettingsCollection.GetEnumerator: was rebuilding a whole Dictionary via
  ToDictionary on every enumeration; now yields over the backing dictionary.
- Q2 SettingsTypesExtractor: replace Name.ToLower().EndsWith(suffix.Trim().ToLower())
  with EndsWith(suffix, OrdinalIgnoreCase) and hoist the trimmed suffix out of the
  per-type predicate. Also fixes a culture-sensitivity smell (ToLower was
  CurrentCulture). Regression test locks case-insensitive matching.
- Q3 EnvironmentVariableBinder: fast-path context.Key when there's no prefix and no
  formatter (skips the per-property StringBuilder), and collapse the Contains+indexer
  double lookup to a single IDictionary indexer read. Regression test covers the
  prefix branch.
- Q4 SettingsClassGenerator: cache the generated impl by interface Type in a
  ConcurrentDictionary instead of re-querying the module by mangled type name on every
  call. Regression test asserts the same interface returns the cached Type.

Build clean on net8.0 + net10.0; 55/55 tests green on net10.0 (was 52).
Bring the running status current: P2 (#18) and the docs tutorials (#20) merged,
the #8#20 workstream branches pruned, and Q1–Q4 quick wins in flight (Q5 was
already resolved by B4). Flip the merged checklist items and re-rank next
priorities to P3 → P4 → P5, then engine tests (T7 concurrency race still open)
and architecture.
Q4's Type-keyed cache exposed a latent collision: GenerateType derived the impl
type name from the *simple* interface name (GetNormalizeInterfaceName = Type.Name
minus leading I), so two settings interfaces sharing a simple name across
namespaces (Foo.ISettings + Bar.ISettings) both mapped to the same module type
name. Under the old name-keyed lookup the second silently reused the first's
(wrong) type; under the Type-keyed cache the second DefineType now throws and
aborts the whole scan.

Fix derives the impl name from the namespace-qualified FullName (sanitized),
kept deliberately separate from GetNormalizeInterfaceName — that helper also
backs SettingsOptions.SectionNameFormatter (the config section name), which must
stay simple-name-based. + a regression test generating two same-simple-name
interfaces. 56/56 green on net10.0.
The macro ScanBenchmark can't resolve the quick wins (they're <1% of the
IL-emit/populate cost). These isolate each changed hot path so the wins are
measurable and trackable:
- EnumerateBenchmark (Q1): enumerate a ~2000-entry ISettingsCollection.
- EnvBinderBenchmark (Q3): EnvironmentVariableBinder.BindPropertySettings fast path.
- GenerateTypeBenchmark (Q4): warm SettingsClassGenerator.GenerateType (cache hit).

Grants the benchmark assembly InternalsVisibleTo (Q4 uses the internal generator)
and references the Binders project (Q3 uses EnvironmentVariableBinder).
@guy-lud

guy-lud commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

Update: M1 fix + micro-benchmarks (proof)

M1 (from review) fixed (656905e): the generated impl type name is now namespace-qualified in the generator onlyGetNormalizeInterfaceName is left alone because it also backs the default config section name (SettingsOptions.SectionNameFormatter). Two settings interfaces sharing a simple name across namespaces (Foo.ISettings + Bar.ISettings) now generate distinct impls instead of aborting the scan. Regression test added; 56/56 green.

Micro-benchmarks added (f65890c) isolating each hot path — the macro ScanBenchmark can't resolve these (they're <1% of the IL-emit/populate cost). Before = master's impl, after = this branch (measured by overlaying the four production files, same benchmark code, ShortRun, [MemoryDiagnoser]):

Benchmark Before (master) After Delta
Q1 Enumerate — 2000-entry ISettingsCollection 23.74 µs · 64.03 KB 8.66 µs · 88 B 2.7× faster, ~745× fewer bytes
Q3 BindFastPath — env binder fast path 29.16 ns · 152 B 10.99 ns · 0 B 2.65× faster, 0 alloc
Q4 GenerateWarm — warm GenerateType (cache hit) 74.81 ns · 224 B 2.33 ns · 0 B 32× faster, 0 alloc

Honest framing: these are per-operation wins on repeated paths (warm resolves, enumeration, per-property env binding). Cold startup is dominated by Reflection.Emit, so the macro scan number is unchanged — Q1–Q4 are allocation-hygiene + repeated-path speedups, not a first-scan speedup.

(Q2 is a culture-correctness fix — OrdinalIgnoreCase suffix match — with negligible, hard-to-isolate perf; no dedicated micro-benchmark.)

@guy-lud
guy-lud merged commit 4dd002a into master Jul 12, 2026
1 check passed
@guy-lud
guy-lud deleted the perf/quick-wins-q1-q5 branch July 12, 2026 18:56
guy-lud added a commit that referenced this pull request Jul 12, 2026
…22)

Runs BenchmarkDotNet on every push to master and on PRs, then feeds per-benchmark
allocated-bytes into benchmark-action/github-action-benchmark:
- push to master records the new baseline on the gh-pages data branch (dev/bench);
- PRs compare against that baseline, comment on a >10% allocation jump, and fail the check.

Gates on allocated bytes rather than time: allocation counts are deterministic and
stable on shared runners, so they can safely fail a build; time stays informational
(in the run logs). JSON export -> jq reshape -> customSmallerIsBetter was validated
locally against ScanBenchmark. Mirrors ci.yml's SDK/cache setup. The micro-benchmark
filters activate automatically once PR #21 lands them on master.
guy-lud added a commit that referenced this pull request Jul 12, 2026
…23)

* Session wrap: refresh handoff + fix-plan; gitignore BenchmarkDotNet output

- 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 .claude/settings.local.json (personal SessionStart hook)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant