diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index 5469a6b..86ed265 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -57,13 +57,13 @@ jobs: - name: Run benchmarks working-directory: src # global.json lives here # ShortRun keeps CI time reasonable; allocation counts are deterministic regardless of - # iteration count. The micro-benchmark filters match nothing until PR #21 lands them on - # master, at which point they are tracked automatically alongside ScanBenchmark. + # iteration count. PlanPopulateBenchmark tracks the P3 warm re-populate path; it has no gh-pages + # baseline until the first master run records one, so it only starts alerting after that. run: > dotnet run -c Release --project performance/ExistForAll.SimpleSettings.Benchmark -- - --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" + --filter "*ScanBenchmark*" "*EnumerateBenchmark*" "*EnvBinderBenchmark*" "*GenerateTypeBenchmark*" "*PlanPopulateBenchmark*" --job short --exporters json --artifacts ${{ github.workspace }}/bdn diff --git a/FIX-PLAN.md b/FIX-PLAN.md index f1fc599..cb1b0db 100644 --- a/FIX-PLAN.md +++ b/FIX-PLAN.md @@ -5,8 +5,9 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform ## 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)** · **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. +- **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)**. +- **In flight:** **P3 — cached "settings plan"** (`SettingsPlan` per type: section name resolved once + lazily, per-property key/default/converter precomputed, `[SettingsProperty]` read **once** per property). Warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)** — after a code+perf review pass, consolidating the 3× attribute read more than paid for the plan objects. The emitted/compiled setter was built and measured but **reverted** — it regressed the gated cold scan **+25%** for **no** warm gain (net10 reflective `SetValue` is already allocation-free for the args). A new gated `PlanPopulateBenchmark` tracks the warm path. +- **Next:** P4 (de-reflect array/enumerable converters) → P5 (resolve config section once per type). - **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`. @@ -55,7 +56,7 @@ _Derived from the 2026-07-10 three-part review (architecture · tests · perform - [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 +- [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) - [ ] P4 · De-reflect array/enumerable converters · Sev Med · Eff M - [ ] P5 · Resolve config section once per type, not per property · Sev Med · Eff M @@ -208,6 +209,10 @@ if (settingsOptions.AttributeType != null && ### P3 · Cached compiled “settings plan” — Sev High · Eff L (biggest ceiling) The populate loop (`src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs:36-55`) is reflection-saturated: reflective `property.SetValue` (`:55`, boxes value types); `GetSectionName` recomputed inside both loops though it’s constant per type; `GetPropertyName` recomputed per binder; `SettingsPropertyAttribute` read ~3×/property (`:36-40` + `TypeConverter.cs:36`). Build a per-interface `SettingsPlan` once (`ConcurrentDictionary`) holding: section name (once), and per property — resolved key, default value, chosen converter, and a **compiled setter** (emit the populate into the generated class in `PropertyCreator`, or a compiled `Action`). Folds in converter caching (below). +**Resolved (this PR):** `SettingsPlan` cached per type on the `ValuesPopulator` instance — section name resolved once and **lazily** (a no-binder scan never pays for it), per-property `PropertyPlan`/`PropertyConversion` as `readonly struct`s (one array alloc, no per-property object), converter chosen once (manual walk, not LINQ `First`, so the `LinkedList` enumerator isn't boxed), and `[SettingsProperty]` read **once** per property then threaded into key/default/conversion (was 3× via `GetPropertyName`/`GetDefaultValue`/`CreateConversion`). **Warm re-populate −55–61%** (50 props 15,681→6,816 B); gated `ScanBenchmark` **≈flat (−0.4%)**. The **compiled setter was dropped**: both variants (emit `__Set` into the generated type, and a compiled `Action`) regressed the *gated* cold `ScanBenchmark` (+25% for the emitted `__Set`) because the extra per-type codegen isn't amortized on a populate-once scan — and it bought **nothing** on the warm path, since net10's reflective `PropertyInfo.SetValue` no longer allocates an args array. A **code + perf review pass** (two agents) confirmed behavior/exception parity and drove the attribute-read consolidation + binder-array materialization; it also flagged **exception wrapping** (converter-setup failures at plan build are re-wrapped as `SettingsPropertyValueException`) and two follow-ups below. + +**Follow-ups from the review:** (a) **P3b** — tiered/lazy setter compilation (compile on the 2nd+ populate) would de-reflect the hot path without regressing the cold scan; only worth it if a profile shows `SetValue` *time* (not allocation) matters. (b) **Binder key alloc** — `InMemoryCollection.CreateKey` (`Binder/InMemoryCollection.cs:27`) concatenates `section + ":" + key` per lookup (~⅓ of the warm populate number); a `ValueTuple` dictionary key would make lookups allocation-free and remove the `"a:b"` collision ambiguity (same class of fix as Q3). Not a P3 regression — pre-existing binder cost. + ### Quick wins — Eff S each - **Q1** `SettingsCollection.GetEnumerator` (`SettingsCollection.cs:52`) rebuilds a whole `Dictionary` per enumeration → `yield return` over the existing dictionary. - **Q2** `SettingsTypesExtractor.cs:32` `Name.ToLower().EndsWith(suffix.ToLower())` → `EndsWith(suffix, StringComparison.OrdinalIgnoreCase)` (also fixes an ordinal-correctness smell); hoist the trimmed suffix. (Startup allocs across all scanned types.) diff --git a/SESSION-HANDOFF.md b/SESSION-HANDOFF.md index 063abd6..dc48ed1 100644 --- a/SESSION-HANDOFF.md +++ b/SESSION-HANDOFF.md @@ -3,26 +3,28 @@ _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 — 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). +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 **merged and live** (#22). **P3 — cached "settings plan" — is done + review-hardened (this PR, branch `perf/p3-compiled-settings-plan`):** warm re-populate **−55–61%** allocations (50 props 15,681→6,816 B), gated `ScanBenchmark` **≈flat (−0.4%)**. A code+perf review pass (two agents) confirmed behavior/exception parity and drove the final refinements. 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). +2. **Merge the P3 PR** (`perf/p3-compiled-settings-plan`) if still open + green — via the `guy-lud` identity. Confirm the benchmark check passed (scan ≈ flat / −0.4%). Then `git checkout master && git pull`. +3. Start **P4** (next perf item) on a fresh work branch. **Refresh this handoff on that branch** at wrap — never a dedicated docs branch (see Gotchas). ## Current state -- 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. +- On branch **`perf/p3-compiled-settings-plan`** (off `master` @ `6bd5b0e`) — **P3 PR open**, this session's work; warm −56% / scan +4.7%, 56 tests green. Merge it, then `master` is the base for P4. +- 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) +- **P3 — cached "settings plan" (this PR).** `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`. - **#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. +- **#22 — benchmark-tracking CI (merged)** (`.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` + `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). @@ -38,12 +40,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 package). Breaking changes free until the first `v2.0.0-beta`. ## Next priorities (ranked — detail in FIX-PLAN.md) -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. +1. **Perf:** **P4** — de-reflect the array/enumerable converters (`Array.Empty`/`Array.CreateInstance` + indexed assignment; DRY the near-duplicate `ArrayTypeConverter`/`EnumerableTypeConverter`), 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, 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. ## 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). @@ -56,4 +57,5 @@ Still **pre-stable** (no `v*` tag; only auto-alphas published), so breaking chan - **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. +- **Wrap ritual — handoff branch rule:** refresh this file **on the current work branch** (or `master`) so it merges with the session's real PR. **Never** create a dedicated docs branch/PR for it (that's what #23 was) — `release.yml` fires on *every* `master` push with **no `paths` filter**, so a doc-only merge burns a throwaway `-alpha` for nothing. - Commits/PRs here **omit** the Co-Authored-By / Generated-with trailer (project preference). diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs index b815477..69c46f0 100644 --- a/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/ISettingsTypeConverter.cs @@ -2,6 +2,14 @@ namespace ExistForAll.SimpleSettings.Conversion { + /// + /// Converts a bound settings value to a property's target type. + /// + /// + /// Implementations must be stateless and thread-safe: a converter is selected once per settings + /// type and reused for every resolution of that type, including concurrent ones. Do not hold per-call + /// state on the instance. + /// public interface ISettingsTypeConverter { bool CanConvert(Type settingsType); diff --git a/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs new file mode 100644 index 0000000..d10d3a0 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/Conversion/PropertyConversion.cs @@ -0,0 +1,44 @@ +using System; + +namespace ExistForAll.SimpleSettings.Conversion +{ + // A per-property conversion resolved once (at plan build) instead of on every populate: the chosen + // converter, the stripped target type, and the null-value outcome are all baked in here, so the hot path + // is a single null-check plus one virtual Convert call — no attribute reads, no converter scan. + // A readonly struct so it lives inline inside the PropertyPlan value array — no per-property heap object, + // which keeps the cold at-scale scan (a plan per type, each built once) from regressing on allocations. + internal readonly struct PropertyConversion + { + private readonly ISettingsTypeConverter _converter; + private readonly Type _strippedType; + private readonly bool _throwOnNull; + private readonly object? _nullResult; + private readonly string _propertyName; + + public PropertyConversion(ISettingsTypeConverter converter, + Type strippedType, + bool throwOnNull, + object? nullResult, + string propertyName) + { + _converter = converter; + _strippedType = strippedType; + _throwOnNull = throwOnNull; + _nullResult = nullResult; + _propertyName = propertyName; + } + + public object? Convert(object? value) + { + if (value == null) + { + if (_throwOnNull) + throw new Exception(Resources.PropertyNotAllowNullMessage(_propertyName)); + + return _nullResult; + } + + return _converter.Convert(value, _strippedType); + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs index 5fbd30b..5f6c093 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/ITypeConverter.cs @@ -1,9 +1,10 @@ -using System.Reflection; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal interface ITypeConverter - { - object? ConvertValue(object? value, PropertyInfo propertyInfo, SettingsOptions options); - } -} \ No newline at end of file +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; + +namespace ExistForAll.SimpleSettings.Core.Reflection +{ + internal interface ITypeConverter + { + PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPropertyAttribute? attribute, SettingsOptions options); + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs deleted file mode 100644 index 7b5efea..0000000 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/PropertyInfoExtensions.cs +++ /dev/null @@ -1,14 +0,0 @@ -using System.Reflection; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal static class PropertyInfoExtensions - { - public static object? GetDefaultValue(this PropertyInfo property) - { - var attribute = property.GetCustomAttribute(); - - return attribute?.DefaultValue; - } - } -} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs index e7c10a0..aa25138 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeConverter.cs @@ -1,65 +1,66 @@ -using System; -using System.Linq; -using System.Reflection; -using ExistForAll.SimpleSettings.Conversion; - -namespace ExistForAll.SimpleSettings.Core.Reflection -{ - internal class TypeConverter : ITypeConverter - { - public object? ConvertValue(object? value, PropertyInfo propertyInfo, SettingsOptions options) - { - var propertyType = propertyInfo.PropertyType; - - if (value == null) - { - ValidateNullAcceptance(propertyInfo); - - if (!propertyType.IsEnumerable()) - return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; - - var genericType = propertyType.GetTypeInfo().GetGenericArguments().First(); - var method = typeof(Enumerable).GetTypeInfo().GetMethod("Empty")!.MakeGenericMethod(genericType); - var emptyEnumerable = method.Invoke(null, null); - return emptyEnumerable; - } - - var strippedType = StripIfNullable(propertyType); - - var settingsTypeConverter = GetConverter(strippedType, propertyInfo, options); - - return settingsTypeConverter.Convert(value, strippedType); - } - - private ISettingsTypeConverter GetConverter(Type strippedType, PropertyInfo propertyInfo, SettingsOptions options) - { - var attribute = propertyInfo - .GetCustomAttribute(); - - if (attribute?.ConverterType == null) - return options.Converters.First(x => x.CanConvert(strippedType)); - - var converter = (ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!; - - return converter; - } - - private static Type StripIfNullable(Type type) - { - return type.GetTypeInfo().IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) ? - type.GetTypeInfo().GetGenericArguments()[0] : - type; - } - - private static void ValidateNullAcceptance(PropertyInfo propertyInfo) - { - var attribute = propertyInfo.GetCustomAttribute(); - - if(attribute == null) - return; - - if(!attribute.AllowEmpty) - throw new Exception(Resources.PropertyNotAllowNullMessage(propertyInfo.Name)); - } - } -} +using System; +using System.Linq; +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; + +namespace ExistForAll.SimpleSettings.Core.Reflection +{ + internal class TypeConverter : ITypeConverter + { + // Resolve everything about a property's conversion up front: which converter handles it, the target + // type once nullability is stripped, and what a null bound value should yield. Previously all of this + // (including up to three SettingsPropertyAttribute reads and a linear converter scan) ran on every + // single populate; now it runs once per type and the result is cached in the settings plan. + public PropertyConversion CreateConversion(PropertyInfo propertyInfo, SettingsPropertyAttribute? attribute, SettingsOptions options) + { + var propertyType = propertyInfo.PropertyType; + + var throwOnNull = attribute != null && !attribute.AllowEmpty; + var nullResult = CreateNullResult(propertyType); + + var strippedType = StripIfNullable(propertyType); + var converter = GetConverter(strippedType, attribute, options); + + return new PropertyConversion(converter, strippedType, throwOnNull, nullResult, propertyInfo.Name); + } + + // The value a null bound value converts to: an empty sequence for IEnumerable, a default instance + // for a value type, otherwise null. Constant per property, so it is materialized once at plan build. + private static object? CreateNullResult(Type propertyType) + { + if (!propertyType.IsEnumerable()) + return propertyType.GetTypeInfo().IsValueType ? Activator.CreateInstance(propertyType) : null; + + var genericType = propertyType.GetTypeInfo().GetGenericArguments().First(); + var method = typeof(Enumerable).GetTypeInfo().GetMethod("Empty")!.MakeGenericMethod(genericType); + return method.Invoke(null, null); + } + + private static ISettingsTypeConverter GetConverter(Type strippedType, + SettingsPropertyAttribute? attribute, + SettingsOptions options) + { + if (attribute?.ConverterType != null) + return (ISettingsTypeConverter)Activator.CreateInstance(attribute.ConverterType)!; + + // Manual walk over the concrete LinkedList (not LINQ First) so the struct enumerator isn't boxed + // onto the heap — this runs once per property at plan build. + foreach (var converter in options.Converters) + { + if (converter.CanConvert(strippedType)) + return converter; + } + + // Unreachable in practice: DefaultTypeConverter.CanConvert always returns true. Kept so every path + // returns and to mirror the old First(...) which also threw when nothing matched. + throw new InvalidOperationException($"No converter found for type '{strippedType}'."); + } + + private static Type StripIfNullable(Type type) + { + return type.GetTypeInfo().IsGenericType && type.GetGenericTypeDefinition() == typeof(Nullable<>) ? + type.GetTypeInfo().GetGenericArguments()[0] : + type; + } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs index dfdccdc..c07f573 100644 --- a/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs +++ b/src/Core/ExistForAll.SimpleSettings/Core/Reflection/TypeExtensions.cs @@ -8,7 +8,7 @@ internal static class TypeExtensions { public static string GetNormalizeInterfaceName(this Type target) { - return target.Name[0] == 'I' ? $"{target.Name.Substring(1)}" : target.Name; + return target.Name[0] == 'I' ? target.Name.Substring(1) : target.Name; } public static string GetSectionName(this Type settingsClass, SettingsOptions options) @@ -20,15 +20,6 @@ public static string GetSectionName(this Type settingsClass, SettingsOptions opt : options.SectionNameFormatter(settingsClass); } - public static string GetPropertyName(this PropertyInfo propertyInfo) - { - var attribute = propertyInfo.GetCustomAttribute(true); - - return !string.IsNullOrWhiteSpace(attribute?.Name) - ? attribute.Name - : propertyInfo.Name; - } - public static bool IsEnumerable(this Type type) { var info = type.GetTypeInfo(); diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs index fb4f0bd..6acf3ae 100644 --- a/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs +++ b/src/Core/ExistForAll.SimpleSettings/SettingsBuilder.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Linq; using System.Reflection; using ExistForAll.SimpleSettings.Core; using ExistForAll.SimpleSettings.Core.Reflection; @@ -34,7 +35,9 @@ internal SettingsBuilder(SettingsOptions options, IValuesPopulator valuesPopulator) { Options = options; - SectionBinders = sectionBinders; + // Materialize once so the populator's `binders as ISectionBinder[]` fast-path hits instead of + // re-allocating via ToArray on every populate (the factory hands us a SortedList.Values view). + SectionBinders = sectionBinders as ISectionBinder[] ?? sectionBinders.ToArray(); _settingsTypesExtractor = settingsTypesExtractor; _settingsOptionsValidator = settingsOptionsValidator; _settingsClassGenerator = settingsClassGenerator; diff --git a/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs b/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs new file mode 100644 index 0000000..fd1fd54 --- /dev/null +++ b/src/Core/ExistForAll.SimpleSettings/SettingsPlan.cs @@ -0,0 +1,52 @@ +using System; +using System.Reflection; +using ExistForAll.SimpleSettings.Conversion; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings +{ + // Everything needed to populate one settings interface that is constant across instances, computed once + // and cached per type: the section name (resolved once, not per property/binder) and the per-property + // plan (resolved key, default value, and precomputed conversion). + internal sealed class SettingsPlan + { + private readonly Type _settingsType; + private readonly SettingsOptions _options; + private string? _sectionName; + + public SettingsPlan(Type settingsType, SettingsOptions options, PropertyPlan[] properties) + { + _settingsType = settingsType; + _options = options; + Properties = properties; + } + + // Resolved lazily and cached: a builder with no binders (e.g. the cold assembly scan) never reads it, + // so it never pays the section-name attribute lookup. The value is deterministic, so the racy set is + // harmless — concurrent readers compute the same string. + public string SectionName => _sectionName ??= _settingsType.GetSectionName(_options); + + public PropertyPlan[] Properties { get; } + } + + // A readonly struct held inline in SettingsPlan.Properties: the whole per-type plan is then one array + // allocation plus the section string, rather than an object per property (matters at scan scale). + internal readonly struct PropertyPlan + { + public PropertyPlan(PropertyInfo property, string key, object? defaultValue, PropertyConversion conversion) + { + Property = property; + Key = key; + DefaultValue = defaultValue; + Conversion = conversion; + } + + public PropertyInfo Property { get; } + + public string Key { get; } + + public object? DefaultValue { get; } + + public PropertyConversion Conversion { get; } + } +} diff --git a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs index 5eb336a..d873112 100644 --- a/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs +++ b/src/Core/ExistForAll.SimpleSettings/ValuesPopulator.cs @@ -1,76 +1,128 @@ -using System; -using System.Collections.Generic; -using System.Linq; -using System.Reflection; -using ExistForAll.SimpleSettings.Core.Reflection; - -namespace ExistForAll.SimpleSettings -{ - internal class ValuesPopulator : IValuesPopulator - { - private readonly ITypePropertiesExtractor _typePropertiesExtractor; - private readonly ITypeConverter _typeConverter; - - public ValuesPopulator() : - this(new TypePropertiesExtractor(), new TypeConverter()) - { - } - - internal ValuesPopulator(ITypePropertiesExtractor typePropertiesExtractor, ITypeConverter typeConverter) - { - _typePropertiesExtractor = typePropertiesExtractor; - _typeConverter = typeConverter; - } - - public void PopulateInstanceWithValues(object instance, - Type settings, - SettingsOptions options, - IEnumerable binders) - { - var sectionBinders = binders as ISectionBinder[] ?? binders.ToArray(); - foreach (var property in _typePropertiesExtractor.ExtractTypeProperties(settings)) - { - var tempValue = property.GetDefaultValue(); - foreach (var binder in sectionBinders) - { - var context = new BindingContext(settings.GetSectionName(options), - property.GetPropertyName(), - settings, - property, - tempValue); - - try - { - binder.BindPropertySettings(context); - if (context.HasNewValue) - tempValue = context.NewValue; - } - catch (Exception e) - { - throw new SettingsBindingException(binder, context, e); - } - } - - var propertyValue = ConvertPropertyValue(settings, tempValue, property, options); - property.SetValue(instance, propertyValue); - } - } - - private object? ConvertPropertyValue(Type settingsType, - object? value, - PropertyInfo property, - SettingsOptions options) - { - try - { - var propertyValue = _typeConverter.ConvertValue(value, property, options); - - return propertyValue; - } - catch (Exception e) - { - throw new SettingsPropertyValueException(settingsType, value, property, e); - } - } - } -} \ No newline at end of file +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Reflection; +using ExistForAll.SimpleSettings.Core.Reflection; + +namespace ExistForAll.SimpleSettings +{ + internal class ValuesPopulator : IValuesPopulator + { + private readonly ITypePropertiesExtractor _typePropertiesExtractor; + private readonly ITypeConverter _typeConverter; + + // One plan per settings interface, reused across every populate. Instance field (not static): a plan + // bakes in this builder's SettingsOptions (section formatter, converters, per-property converters), and + // exactly one ValuesPopulator is created per SettingsBuilder, so Options is fixed for this cache's + // lifetime. Same reasoning as the TypePropertiesExtractor cache (P2). + private readonly ConcurrentDictionary _plans = new(); + + public ValuesPopulator() : + this(new TypePropertiesExtractor(), new TypeConverter()) + { + } + + internal ValuesPopulator(ITypePropertiesExtractor typePropertiesExtractor, ITypeConverter typeConverter) + { + _typePropertiesExtractor = typePropertiesExtractor; + _typeConverter = typeConverter; + } + + public void PopulateInstanceWithValues(object instance, + Type settings, + SettingsOptions options, + IEnumerable binders) + { + var sectionBinders = binders as ISectionBinder[] ?? binders.ToArray(); + + var plan = GetOrBuildPlan(settings, options); + + foreach (var propertyPlan in plan.Properties) + { + var tempValue = propertyPlan.DefaultValue; + + foreach (var binder in sectionBinders) + { + var context = new BindingContext(plan.SectionName, + propertyPlan.Key, + settings, + propertyPlan.Property, + tempValue); + + try + { + binder.BindPropertySettings(context); + if (context.HasNewValue) + tempValue = context.NewValue; + } + catch (Exception e) + { + throw new SettingsBindingException(binder, context, e); + } + } + + var propertyValue = ConvertPropertyValue(settings, tempValue, propertyPlan); + propertyPlan.Property.SetValue(instance, propertyValue); + } + } + + private SettingsPlan GetOrBuildPlan(Type settings, SettingsOptions options) + { + if (_plans.TryGetValue(settings, out var existing)) + return existing; + + var extracted = _typePropertiesExtractor.ExtractTypeProperties(settings); + var properties = extracted as PropertyInfo[] ?? extracted.ToArray(); + + var propertyPlans = new PropertyPlan[properties.Length]; + for (var i = 0; i < properties.Length; i++) + { + var property = properties[i]; + + // Read [SettingsProperty] once and thread it into key/default/conversion — GetCustomAttribute + // re-materializes the attribute + its backing array on every call, and this runs per property + // for every type at cold-scan scale. inherit:true matches the prior key-resolution behavior + // (a no-op for interface members, which is all settings types are). + var attribute = property.GetCustomAttribute(inherit: true); + + try + { + var key = !string.IsNullOrWhiteSpace(attribute?.Name) ? attribute.Name : property.Name; + + propertyPlans[i] = new PropertyPlan(property, + key, + attribute?.DefaultValue, + _typeConverter.CreateConversion(property, attribute, options)); + } + catch (Exception e) + { + // Restore the original exception contract: converter-setup failures used to surface inside + // the per-populate convert try as SettingsPropertyValueException. The bound value isn't + // known at plan build, hence null. + throw new SettingsPropertyValueException(settings, null, property, e); + } + } + + var plan = new SettingsPlan(settings, options, propertyPlans); + + // Concurrent builds would produce equivalent plans, so last-writer-wins is harmless. A build that + // throws is never cached (we don't reach here), so the next call retries — matching the old + // re-throw-every-time behavior. + _plans[settings] = plan; + return plan; + } + + private static object? ConvertPropertyValue(Type settingsType, object? value, in PropertyPlan propertyPlan) + { + try + { + return propertyPlan.Conversion.Convert(value); + } + catch (Exception e) + { + throw new SettingsPropertyValueException(settingsType, value, propertyPlan.Property, e); + } + } + } +} diff --git a/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs new file mode 100644 index 0000000..f6f48ed --- /dev/null +++ b/src/performance/ExistForAll.SimpleSettings.Benchmark/PlanPopulateBenchmark.cs @@ -0,0 +1,54 @@ +using System; +using BenchmarkDotNet.Attributes; +using ExistForAll.SimpleSettings.Binder; + +namespace ExistForAll.SimpleSettings.Benchmark +{ + /// + /// P3 — warm re-populate of a settings type through a primed builder, scaled by property count. The impl + /// type is emitted and the per-type SettingsPlan (section name resolved once, per-property keys + + /// defaults + converters cached) are built once in setup, so each measured call pays only: allocate the + /// instance + run the populate off the cached plan. + /// This is the hot path P3 targets — repeated resolves that previously re-read attributes, rescanned the + /// converter list, rebuilt the section name, and set each property reflectively. It mirrors + /// ResolveBenchmark.WarmResolve_Provider but is a dedicated, cold-noise-free entry so CI can gate + /// its allocations. (New benchmark: no gh-pages baseline yet, so it only starts alerting once master + /// records the first point.) + /// + [MemoryDiagnoser] + public class PlanPopulateBenchmark + { + [Params(1, 10, 50)] + public int PropertyCount; + + private Type _type = null!; + private SettingsBuilder _builder = null!; + + [GlobalSetup] + public void Setup() + { + _type = PropertyCount switch + { + 1 => typeof(IProps1), + 10 => typeof(IProps10), + 50 => typeof(IProps50), + _ => throw new ArgumentOutOfRangeException(nameof(PropertyCount)), + }; + + // Seed an in-memory value for every property so the whole bind + convert + set path runs. + // Section name mirrors the engine's default formatter (strip a leading 'I'). + var collection = new InMemoryCollection(); + var section = _type.Name[0] == 'I' ? _type.Name.Substring(1) : _type.Name; + foreach (var property in _type.GetProperties()) + collection.Add(section, property.Name, "value"); + + // Prime once: emit the impl type and build + cache the SettingsPlan. The measured calls then pay + // only the per-resolve populate cost, which is what P3 optimizes. + _builder = SettingsBuilder.CreateBuilder(f => { f.AddInMemoryCollection(collection); }); + _builder.GetSettings(_type); + } + + [Benchmark] + public object Populate() => _builder.GetSettings(_type); + } +}