diff --git a/FIX-PLAN.md b/FIX-PLAN.md index 55b483b..98ef53d 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)**. 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`/`IList`/`ICollection` or document the `IEnumerable`-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` 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** diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 21a28a8..8e84401 100644 --- a/SESSION-HANDOFF.md +++ b/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` 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`) and replace its O(n²) dedup with a `HashSet`. 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`/`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. **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`/`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. ## 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). diff --git a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/EnvironmentVariableBinder.cs b/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/EnvironmentVariableBinder.cs index da397b7..9630732 100644 --- a/src/Core/ExistForAll.SimpleSettings.Extensions.Binders/EnvironmentVariableBinder.cs +++ b/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); } } } \ No newline at end of file diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs index 5390c7e..e96b9e0 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/SettingsClassGenerator.cs +++ b/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 _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) { diff --git a/src/Core/ExistForAll.SimpleSettings/Core/SettingsTypesExtractor.cs b/src/Core/ExistForAll.SimpleSettings/Core/SettingsTypesExtractor.cs index 67f5809..58102bc 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/SettingsTypesExtractor.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/SettingsTypesExtractor.cs @@ -12,12 +12,15 @@ public Type[] ExtractSettingsTypes(IEnumerable 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; diff --git a/src/Core/ExistForAll.SimpleSettings/Info.cs b/src/Core/ExistForAll.SimpleSettings/Info.cs index ff44f89..4f8341b 100644 --- a/src/Core/ExistForAll.SimpleSettings/Info.cs +++ b/src/Core/ExistForAll.SimpleSettings/Info.cs @@ -1,3 +1,4 @@ using System.Runtime.CompilerServices; [assembly:InternalsVisibleTo("ExistForAll.SimpleSettings.UnitTests")] +[assembly:InternalsVisibleTo("ExistForAll.SimpleSettings.Benchmark")] diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs b/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs index d2cd2b7..c2575cf 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsCollection.cs @@ -1,7 +1,6 @@ using System; using System.Collections; using System.Collections.Generic; -using System.Linq; using System.Reflection; namespace ExistForAll.SimpleSettings @@ -49,9 +48,8 @@ IEnumerator IEnumerable.GetEnumerator() public IEnumerator> GetEnumerator() { - var dictionary = _settingsHolders.ToDictionary(x => x.Key, y => y.Value.SettingsImplementation); - - return dictionary.GetEnumerator(); + foreach (var holder in _settingsHolders) + yield return new KeyValuePair(holder.Key, holder.Value.SettingsImplementation); } } } \ No newline at end of file diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/EnvironmentVariableAttributeTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/EnvironmentVariableAttributeTests.cs index 5256b79..3ac4ba4 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/EnvironmentVariableAttributeTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/EnvironmentVariableAttributeTests.cs @@ -26,6 +26,25 @@ public async Task Build_WhereVariableHasValue_ShouldSetProperty() } } + [Test] + public async Task Build_WithPrefix_ShouldLookUpPrefixedVariable() + { + const string prefix = "PREFIX_"; + var guid = Guid.NewGuid().ToString(); + + using (new DisposableEnvironmentVariable(prefix + EnvironmentVariable, guid)) + { + var sut = SettingsBuilder.CreateBuilder(x => + x.AddEnvironmentVariable(o => o.Prefix = prefix)); + + var result = sut.ScanAssemblies(GetType().Assembly); + + var settings = result.GetSettings(); + + await Assert.That(settings.EnvironmentVariable).IsEqualTo(guid); + } + } + [Test] public async Task TryGetValue_WhenEnvVariableIsLast_ShouldSetValueFromEnv() { diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs index 20d0356..76dce07 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsClassGeneratorTests.cs @@ -55,6 +55,33 @@ public async Task GenerateType_WhenGivenAnInterfaceInheritance_ShouldCreateTypeF await Assert.That(isAssignableFrom).IsTrue(); } + [Test] + public async Task GenerateType_WhenCalledTwiceForSameInterface_ReturnsCachedType() + { + var generator = new SettingsClassGenerator(); + + var first = generator.GenerateType(typeof(ITestInterface)); + var second = generator.GenerateType(typeof(ITestInterface)); + + await Assert.That(ReferenceEquals(first, second)).IsTrue(); + } + + [Test] + public async Task GenerateType_ForTwoInterfacesSharingASimpleName_GeneratesDistinctTypes() + { + // DupA.IDuplicateName and DupB.IDuplicateName share the simple name "IDuplicateName". + // The generated impl name must be namespace-qualified, otherwise both map to the same + // module type name and the second DefineType throws (aborting the whole scan). + var generator = new SettingsClassGenerator(); + + var a = generator.GenerateType(typeof(DupA.IDuplicateName)); + var b = generator.GenerateType(typeof(DupB.IDuplicateName)); + + await Assert.That(ReferenceEquals(a, b)).IsFalse(); + await Assert.That(typeof(DupA.IDuplicateName).GetTypeInfo().IsAssignableFrom(a.GetTypeInfo())).IsTrue(); + await Assert.That(typeof(DupB.IDuplicateName).GetTypeInfo().IsAssignableFrom(b.GetTypeInfo())).IsTrue(); + } + [Test] public async Task GenerateType_WhenDerivedHidesABasePropertyName_DeduplicatesByName() { @@ -76,3 +103,21 @@ public interface IHidingChild : IRoot int Extra { get; set; } } } + +namespace ExistForAll.SimpleSettings.UnitTests.SimpleSettings.DupA +{ + // Same simple name as DupB.IDuplicateName; no suffix/attribute/base so neither is auto-discovered + // by assembly scans — they only exercise the generator directly via GenerateType. + public interface IDuplicateName + { + string Value { get; set; } + } +} + +namespace ExistForAll.SimpleSettings.UnitTests.SimpleSettings.DupB +{ + public interface IDuplicateName + { + string Value { get; set; } + } +} diff --git a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs index 3080fe5..db3941c 100644 --- a/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs +++ b/src/Tests/ExistForAll.SimpleSettings.UnitTests/SimpleSettings/SettingsTypesExtractorTests.cs @@ -55,9 +55,27 @@ public async Task ExtractSettingsTypes_WhenTypeHasSettingsSuffixIndications_Shou await Assert.That(results).Contains(typeof(IIndicationInterfaceSettings)); } + [Test] + public async Task ExtractSettingsTypes_WhenSuffixDiffersOnlyByCase_ShouldExtractType() + { + var sut = new SettingsTypesExtractor(); + + // The name ends with "SETTINGS" while the default suffix is "Settings"; matching must be + // case-insensitive (a case-sensitive EndsWith would miss this). + var assemblyCollection = MockAssemblies(typeof(ICasingMismatchSETTINGS)); + + var results = sut.ExtractSettingsTypes(assemblyCollection, new SettingsOptions()); + + await Assert.That(results).Contains(typeof(ICasingMismatchSETTINGS)); + } + private IEnumerable MockAssemblies(Type returnType) { return new[] {returnType.GetTypeInfo().Assembly}; } + + public interface ICasingMismatchSETTINGS + { + } } } diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj b/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj index 9661b17..245b8ab 100644 --- a/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/ExistForAll.SimpleSettings.Benchmark.csproj @@ -14,6 +14,7 @@ + diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs new file mode 100644 index 0000000..b6610ec --- /dev/null +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/MicroBenchmarks.cs @@ -0,0 +1,87 @@ +using System; +using System.Reflection; +using BenchmarkDotNet.Attributes; +using ExistForAll.SimpleSettings.Binders; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings.Benchmark +{ + /// + /// Q1 — SettingsCollection.GetEnumerator. The old implementation rebuilt an entire + /// Dictionary via ToDictionary on every enumeration; the new one yields over the + /// backing store. The ~2000 IPerformanceInterfaceN scan fixtures give a collection large + /// enough that the per-enumeration Dictionary allocation is unmistakable under MemoryDiagnoser. + /// The collection is built once in setup so only enumeration is measured. + /// + [MemoryDiagnoser] + public class EnumerateBenchmark + { + private ISettingsCollection _collection = null!; + + [GlobalSetup] + public void Setup() + => _collection = SettingsBuilder.CreateBuilder() + .ScanAssemblies(typeof(EnumerateBenchmark).Assembly); + + [Benchmark] + public int Enumerate() + { + // Explicit foreach (not LINQ Count) so enumeration always runs — no ICollection short-circuit. + var count = 0; + foreach (var _ in _collection) + count++; + return count; + } + } + + /// + /// Q3 — EnvironmentVariableBinder.BindPropertySettings on the fast path (no prefix, no + /// formatter — the common case). The old code allocated a StringBuilder per property and did a + /// double lookup (Contains then indexer); the new code uses context.Key directly and a + /// single lookup. The is built once in setup so the only per-op + /// allocation is whatever the binder itself does. + /// + [MemoryDiagnoser] + public class EnvBinderBenchmark + { + private const string Key = "BENCH_ENV_KEY"; + private EnvironmentVariableBinder _binder = null!; + private BindingContext _context = null!; + + [GlobalSetup] + public void Setup() + { + // The binder snapshots the environment in its constructor, so set the variable first. + Environment.SetEnvironmentVariable(Key, "value"); + _binder = new EnvironmentVariableBinder(); + + var property = typeof(IProps1).GetProperty(nameof(IProps1.P0))!; + _context = new BindingContext("Props1", Key, typeof(IProps1), property, null); + } + + [Benchmark] + public void BindFastPath() => _binder.BindPropertySettings(_context); + } + + /// + /// Q4 — warm SettingsClassGenerator.GenerateType (cache hit). The old code re-derived the + /// mangled type name and did a reflective Assembly.GetType lookup on every call; the new code + /// returns from a ConcurrentDictionary keyed by interface Type before building the name. + /// Primed in setup so the measured call is a pure cache hit. + /// + [MemoryDiagnoser] + public class GenerateTypeBenchmark + { + private SettingsClassGenerator _generator = null!; + + [GlobalSetup] + public void Setup() + { + _generator = new SettingsClassGenerator(); + _generator.GenerateType(typeof(IProps50)); // emit + cache the type once + } + + [Benchmark] + public Type GenerateWarm() => _generator.GenerateType(typeof(IProps50)); + } +}