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

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)**. Suite: 78 → **102 green** (51 per TFM · net8.0 + net10.0).
-- **Next:** P2 (memoize `ExtractTypeProperties` + replace the O(n²) dedup) — in progress.
+- **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).
+- **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.
 - Running status lives in `SESSION-HANDOFF.md`.
@@ -24,43 +25,43 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform
 ## Summary checklist
 
 **Phase 1 — Correctness bugfixes (one-liners, non-breaking, highest ROI)**
-- [ ] B1 · Register `EnumTypeConverter` — enum binding is broken · Sev High · Eff S
-- [ ] B2 · Invariant culture in `DefaultTypeConverter` — locale data corruption · Sev High · Eff S
-- [ ] B4 · Fix `BindingContext.PropertyType` (returns the interface, not the property type) · Sev Med · Eff S
-- [ ] B5 · Fix validator contradiction (can’t null `AttributeType`) · Sev Med · Eff S
-- [ ] B9 · Fix broken error-message interpolation in `Resources` · Sev Low · Eff S
+- [x] B1 · Register `EnumTypeConverter` — enum binding is broken · Sev High · Eff S
+- [x] B2 · Invariant culture in `DefaultTypeConverter` — locale data corruption · Sev High · Eff S
+- [x] B4 · Fix `BindingContext.PropertyType` (returns the interface, not the property type) · Sev Med · Eff S
+- [x] B5 · Fix validator contradiction (can’t null `AttributeType`) · Sev Med · Eff S
+- [x] B9 · Fix broken error-message interpolation in `Resources` · Sev Low · Eff S
 
 **Phase 2 — Dead code & naming (do while pre-stable; breaking)**
-- [ ] D1 · Delete (or wire) the dead `Validations` API + `ValidatorType` · Sev Med · Eff M · **Break**
-- [ ] D2 · Delete (or fix+wire) `EqualityCompererCreator` (dead + invalid IL) · Sev Med · Eff S
-- [ ] D3 · Fix the `ExistsForAll` namespace typo in the Binders package · Sev Med · Eff S · **Break**
+- [ ] D1 · Delete (or wire) the dead `Validations` API + `ValidatorType` · Sev Med · Eff M · **Break** · **HELD — feature work coming**
+- [ ] D2 · Delete (or fix+wire) `EqualityCompererCreator` (dead + invalid IL) · Sev Med · Eff S · **HELD**
+- [x] D3 · Fix the `ExistsForAll` namespace typo in the Binders package · Sev Med · Eff S · **Break**
 
 **Phase 3 — Correctness/feature (needs design)**
 - [ ] C1 · Decide: support `List<T>`/`IList<T>`/`ICollection<T>` or document the `IEnumerable<T>`-only limit · Sev Med · Eff M
 - [ ] C2 · Introduce a public `SimpleSettingsException` base; make boundary-crossing exceptions public + structured · Sev Med · Eff M · **Break**
-- [ ] C3 · Decide reload / provider-vs-singleton semantics (see also P1) · Sev High · Eff M–L
+- [x] C3 · Decide reload / provider-vs-singleton semantics (see also P1) · Sev High · Eff M–L · **DECIDED: option 2, provider-level cache (#17)**
 
 **Phase 4 — Tests (write-first shortlist, then broaden)**
-- [ ] T1 · Culture parse test (fails first · pairs with B2)
-- [ ] T2 · Enum-from-string test (fails first · pairs with B1)
-- [ ] T3 · DI / Generic-Host integration tests (headline feature, 0 coverage today)
+- [x] T1 · Culture parse test (fails first · pairs with B2)
+- [x] T2 · Enum-from-string test (fails first · pairs with B1)
+- [x] T3 · DI / Generic-Host integration tests (headline feature, 0 coverage today)
 - [ ] T4 · `ValuesPopulator` unit tests (precedence + exception wrappers)
 - [ ] T5 · `TypeConverter` unit tests (null / nullable / empty-enumerable / attribute)
 - [ ] T6 · Converter unit tests (array / enumerable / Uri / DateTime + `List<T>` doc test)
-- [ ] T7 · `SettingsClassGenerator` caching + concurrency stress; collection not-found; binder edge cases
+- [ ] T7 · `SettingsClassGenerator` caching + concurrency stress; collection not-found; binder edge cases · *(caching now covered; concurrency race still open — see P/Q4 note)*
 
 **Phase 5 — Performance**
-- [ ] P0 · Upgrade the benchmark harness (MemoryDiagnoser + phase-split + fixtures) — do first, to measure P1–P3
-- [ ] P1 · Cache built instance on the `ISettingsProvider` resolve path · Sev High · Eff S
-- [ ] P2 · Memoize `ExtractTypeProperties` + fix O(n²) dedup · Sev High · Eff S
+- [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
-- [ ] Q1–Q5 · Quick wins (GetEnumerator, OrdinalIgnoreCase, env-binder, type-cache, dead ctor checks)
+- [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
 
 **Phase 6 — Architecture strategy**
 - [ ] A1 · Decide AOT/trim story; annotate `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` and/or plan a source generator · Sev High · Eff M–L
-- [ ] A2 · Consolidate naming: `ExistAll` / `ExistsForAll` → `ExistForAll` (folder, `.slnx`, `Company`, benchmark) · Sev Low · Eff M
+- [x] A2 · Consolidate naming: `ExistAll` / `ExistsForAll` → `ExistForAll` (folder, `.slnx`, `Company`, benchmark) · Sev Low · Eff M
 - [ ] A3 · `Core.AspNet` ships no public type — make `Environments` public or drop the package · Sev Med · Eff S
 - [ ] A4 · Float `Microsoft.Extensions.*` floor per-TFM (don’t force net8 consumers to 10.x) · Sev Med · Eff S
 - [ ] A5 · Make `SettingsHolder`/`ISettingsHolder` internal (leaked detail) · Sev Low · Eff S · **Break**

SESSION-HANDOFF.md

@@ -3,15 +3,18 @@
 _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). This session cleared the top of the plan and opened the performance track. **`master` is clean at `ef25761`, no open PRs.** Suite is **green — 51 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 — 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).
 
 Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking changes remain free — keep doing breaking cleanup now.
 
 ## Current state
-- On **`master`** @ `ef25761` (PR #17). Clean tree, no open PRs. Next work branches off `master`.
-- Orphaned merged remote branches (from #8–#17) can be pruned anytime; deleting remote branches needs the `guy-lud` push identity.
+- 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.
 
 ## 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.
+- **#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<string>` 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.
@@ -25,12 +28,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 `ExistAll.SimpleConfig` package). Breaking changes free until the first `v2.0.0-beta`.
 
 ## Next priorities (ranked — detail in FIX-PLAN.md)
-1. **P2** — memoize `TypePropertiesExtractor.ExtractTypeProperties` (shared `ConcurrentDictionary<Type, PropertyInfo[]>`) and replace its O(n²) dedup with a `HashSet<string>`. Sev High, Eff S, no decision; visible on the P0 harness. *(in progress this session)*
-2. **Rest of perf:** quick wins **Q1–Q5** (GetEnumerator `yield`, `OrdinalIgnoreCase` suffix match, env-binder fast path, generated-type cache, dead null-checks) → **P3** (cached compiled "settings plan" — biggest ceiling) → P4 (de-reflect array/enumerable converters) → P5 (resolve config section once per type).
-3. **Engine tests:** T4 `ValuesPopulator`, T5 `TypeConverter`, T6 converters, T7 generator caching + concurrency stress.
-4. **Architecture:** A1 (AOT/trim `[RequiresDynamicCode]`/`[RequiresUnreferencedCode]` — HIGH, it's a `Reflection.Emit` lib), C1 (`List<T>`/`IList<T>` 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. **Docs debt (deferred from A2):** the `SimpleConfig` → `SimpleSettings` refresh — the `docs/*.md` tutorials and the README links still pointing at `existall/SimpleConfig`.
-6. **D1 validations feature** — owner-driven; reconcile the `validate-settings` branch.
+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<T>`/`IList<T>` 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.
 
 ## 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).

src/Core/ExistForAll.SimpleSettings.Extensions.Binders/EnvironmentVariableBinder.cs

@@ -1,6 +1,5 @@
 using System;
 using System.Collections;
-using System.Text;
 
 namespace ExistForAll.SimpleSettings.Binders
 {
@@ -24,19 +23,26 @@ public EnvironmentVariableBinder()
 
         public void BindPropertySettings(BindingContext context)
         {
-            var sb = new StringBuilder();
-
-            if (Prefix != null)
-                sb.Append(Prefix);
-
-            sb.Append(VariableNameFormatter != null
-                ? VariableNameFormatter(context.Section, context.Key)
-                : context.Key);
-
-            var variableName = sb.ToString();
-
-            if(_environmentVariables.Contains(variableName))
-                context.SetNewValue(_environmentVariables[variableName]);
+            // Fast path (the common case): with no prefix and no formatter the variable name is just the
+            // key, so skip the StringBuilder allocation entirely.
+            string variableName;
+            if (Prefix == null && VariableNameFormatter == null)
+            {
+                variableName = context.Key;
+            }
+            else
+            {
+                var name = VariableNameFormatter != null
+                    ? VariableNameFormatter(context.Section, context.Key)
+                    : context.Key;
+                variableName = Prefix != null ? Prefix + name : name;
+            }
+
+            // Single lookup: the non-generic IDictionary indexer returns null for an absent key, and an
+            // environment variable is never null when present.
+            var value = _environmentVariables[variableName];
+            if (value != null)
+                context.SetNewValue(value);
         }
     }
 }

src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs

@@ -1,4 +1,5 @@
 using System;
+using System.Collections.Concurrent;
 using System.Linq;
 using System.Reflection;
 using System.Reflection.Emit;
@@ -11,6 +12,10 @@ internal class SettingsClassGenerator : ISettingsClassGenerator
 		private readonly IPropertyCreator _propertyCreator;
 		private readonly ModuleBuilder _moduleBuilder = null!;
 
+		// A settings interface generates exactly one impl type for the module's lifetime, so cache by the
+		// interface Type instead of re-querying the module by mangled type name on every call.
+		private readonly ConcurrentDictionary<Type, Type> _generatedTypes = new();
+
 		internal SettingsClassGenerator(ITypePropertiesExtractor typePropertiesExtractor,
 			IPropertyCreator propertyCreator)
 		{
@@ -29,14 +34,17 @@ public SettingsClassGenerator()
 
 		public Type GenerateType(Type interfaceType)
 		{
+			if (_generatedTypes.TryGetValue(interfaceType, out var existingType))
+				return existingType;
+
 			try
 			{
-				var name = $"{interfaceType.GetNormalizeInterfaceName()}Impl";
-
-				var existingType = _moduleBuilder.Assembly.GetType(name.Replace("+", "\\+"));
-
-				if (existingType != null)
-					return existingType;
+				// Namespace-qualified so two settings interfaces that share a simple name
+				// (e.g. Foo.ISettings + Bar.ISettings) don't collide on the generated type name and
+				// abort the scan. Deliberately NOT GetNormalizeInterfaceName() — that helper also backs
+				// the default config section name (SettingsOptions.SectionNameFormatter), which must stay
+				// simple-name-based; the generated impl name is an internal detail and can differ.
+				var name = $"{(interfaceType.FullName ?? interfaceType.Name).Replace('.', '_').Replace('+', '_')}Impl";
 
 				var properties = _typePropertiesExtractor.ExtractTypeProperties(interfaceType);
 
@@ -45,10 +53,12 @@ public Type GenerateType(Type interfaceType)
 				typeBuilder.AddInterfaceImplementation(interfaceType);
 
 				_propertyCreator.CreateAnonymousProperties(typeBuilder, properties.ToArray(), out _);
-
-				var result = typeBuilder.CreateTypeInfo();
 
-				return result.AsType();
+				var result = typeBuilder.CreateTypeInfo().AsType();
+
+				_generatedTypes[interfaceType] = result;
+
+				return result;
 			}
 			catch (Exception e)
 			{

src/Core/ExistForAll.SimpleSettings/Core/SettingsTypesExtractor.cs

@@ -12,12 +12,15 @@ public Type[] ExtractSettingsTypes(IEnumerable<Assembly> assemblies, SettingsOpt
 			if (assemblies == null) throw new ArgumentNullException(nameof(assemblies));
 			if (options == null) throw new ArgumentNullException(nameof(options));
 
+			// The suffix is constant for the whole scan, so trim it once here rather than per candidate type.
+			var suffix = options.SettingsSuffix.Trim();
+
 			return assemblies.SelectMany(x=>x.GetExportedTypes())
-				.Where(x => x.GetTypeInfo().IsInterface && IsFromOptions(x, options))
+				.Where(x => x.GetTypeInfo().IsInterface && IsFromOptions(x, options, suffix))
 				.ToArray();
 		}
 
-		private static bool IsFromOptions(Type type, SettingsOptions options)
+		private static bool IsFromOptions(Type type, SettingsOptions options, string suffix)
 		{
 			try
 			{
@@ -29,7 +32,7 @@ private static bool IsFromOptions(Type type, SettingsOptions options)
 				if (options.InterfaceBase.GetTypeInfo().IsAssignableFrom(info))
 					return true;
 
-				if (info.Name.ToLower().EndsWith(options.SettingsSuffix.Trim().ToLower()))
+				if (info.Name.EndsWith(suffix, StringComparison.OrdinalIgnoreCase))
 					return true;
 
 				return false;