Optimize the binder's happy path and settle the v1 selector API - #201
Conversation
|
@codex Please perform an in-depth review of the code introduced or modified by this pull request, which implements a new feature. The objective is to determine whether the feature is correctly designed, correctly implemented, sufficiently tested, and consistently integrated into the existing project. Review scopeStart by reading:
Do not review the diff in isolation. Verify that the changes are consistent with the abstractions, behaviors, contracts, and conventions already established in the project. Review areas1. Requirements and expected behaviorVerify that the implementation actually satisfies the described requirements. Look specifically for:
Do not treat a behavior as incorrect merely because a different design would also have been possible. Report an issue only when there is a demonstrable contradiction, omission, defect, or concrete risk. 2. Design and architectureEvaluate:
Also verify that the feature does not introduce a second competing way to achieve an existing operation without an explicit and justified reason. 3. Public API and developer experienceFor every public API, fluent API, extension method, overload, or generic abstraction added or modified, review:
Mentally validate several realistic usage scenarios, including edge cases and invalid usages. 4. Code correctnessLook for concrete defects, including:
Do not report micro-optimizations without a realistic impact. 5. TestsAssess whether the tests genuinely demonstrate the intended behavior of the feature. Verify coverage of:
Identify tests that may pass without actually proving the announced behavior, as well as tests that are excessively coupled to implementation details. Do not mechanically request a test for every line or method. Recommend tests only when they protect an important behavior or an identified risk. 6. DocumentationVerify that:
Do not criticize writing style unless it creates a real technical ambiguity or communicates incorrect behavior. 7. Regressions and integrationAnalyze the impact on existing code, including:
Do not perform a general audit of the entire repository. Inspect existing code only when it is necessary to evaluate the changes introduced by this pull request. Expected outputStart with a concise summary containing:
Then report only genuinely actionable findings, classified using these severity levels:
For each finding, include:
Group findings that share the same root cause instead of reporting multiple variations of the same issue. After the findings, add a separate section for open questions. Do not present a question as a defect unless the answer establishes that an actual issue exists. Finish with:
Review rules
|
Re-measured issue #151 on the current code: ~70-75% of the ~700 B per-property happy-path cost is the call-site expression tree, which no internal change can remove; the removable remainder was uncached reflection reads and path strings built for arguments that never fail. Remove that remainder, at constant observable behavior: * Read DTO properties through compiled getters cached per (DTO type, property) in PropertyGetters<TDto,TValue> — deleting the per-bind reflection invoke, the nullable value-type box, and the Nullable.GetUnderlyingType check on every re-bind. The mis-declaration guard moves into the compile step and still throws on every call; environments without IL emission fall back to the interpreter, then to the original reflection read. * Defer argument-path building to failure recording: converter stages carry the selected PropertyInfo (or the already-resolved out-of-DTO name) in a single union field resolved through ArgumentPaths, so an all-valid bind builds no path string and never consults the IArgumentNameProvider for scalar and list properties. A bound complex property still resolves its one prefix segment, exactly what the eager code paid. * Defer list-element paths ("Tags[2]") and complex-list element prefixes ("Guests[2]") through IElementPathSource — built only for the elements that actually fail — and pre-size the result list from ICollection<T>.Count. * Create the per-binding error list on first failure instead of eagerly, including for nested and per-element bindings. Byte-exact before/after on the benchmark scenarios: lists of 10 go from 2504 B to 1624 B (-35%), a nullable-int property from 920 B to 816 B, the full booking request from 13256 B to 11992 B; list binds are ~2.7x faster and nullable value-type binds ~1.6x. Error paths, codes, messages, recording order, and the eager selector guards are unchanged; the one intended bug-channel delta is that a throwing DTO getter now surfaces its own exception instead of TargetInvocationException. Refs: #151
Pin what the perf change promised: one compiled getter reused per (DTO type, property) with no cross-type collision and safe concurrent first binds; the non-nullable value-type guard thrown on every call, never cached away; a throwing DTO getter surfacing its own exception (the binder's bug channel, no reflection wrapper); zero name-provider calls on an all-valid bind of scalar and list properties, one per bound complex property, one per failing argument otherwise; and byte-identical prefixed, indexed error paths on nested and list failures. Refs: #151
Issue #151 asked for a re-measurement before any remedy: add the BenchmarkDotNet project that produced those numbers, so the next perf question starts from a runnable harness instead of a stale figure. It measures the happy path against a hand-written floor, isolates the call-site expression-tree cost (hoisted-selector and micro scenarios), covers lists, nesting, out-of-DTO arguments and the failure path, and its README carries the measured tables and their reading — including the decomposition behind ADR-0023. The project builds with the solution (never packed, never run by CI's test step) so it cannot bit-rot; BenchmarkDotNet is pinned centrally. Refs: #151
The benchmark project is a measurement harness: never shipped, never unit-tested. Excluding it from coverage only — it still gets the SonarAnalyzer pass — keeps the new-code coverage gate about the library. Refs: #151
Issue #151 gated its remedy on a decision to take before the v1 API freeze. The re-measurement shows ~70-75% of the per-property cost is the call-site expression tree — out of the library's reach — while the internal remainder is now removed. Draft the decision that closes the gate as Proposed: keep expression-tree selectors as the sole v1 selector API, accept the per-call expression cost (hoisted selectors remain the measured, non-breaking mitigation), and defer any delegate-based fast path to a post-v1, additive decision. English and French versions, indexed; status left to the maintainer. Refs: #151
The maintainer validated the proposed decision: the v1 binder keeps expression-tree selectors as its sole selector API, the per-call expression cost is accepted (hoisted selectors remain the measured, non-breaking mitigation for hot call sites), and any delegate-based fast path stays a post-v1, additive option. Recorded on the maintainer's explicit instruction. Refs: #151
f385328 to
b4f9342
Compare
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
PR #201 merged and claimed ADR-0023 (keep expression-tree selectors for the v1 binder API) while this branch was still open with its own ADR-0023 (the one-time editorial-refactoring exception). Renumbers the latter to ADR-0024 and updates every cross-reference so no two records share a number.
2 participants
Summary
Re-measures issue #151 on the current code, removes every library-internal happy-path cost it found (cached compiled getters, argument paths deferred to failure recording, lazy error lists), and settles the issue's v1 gate with ADR-0023: the call-site expression tree — ~70–75 % of the per-property cost — stays, as an accepted API decision rather than an optimization target.
Type of change
Changes
PropertyGetters<TDto,TValue>: DTO properties are read through compiled getters cached per (DTO type, property) — removing the per-bind reflection invoke, the nullable value-type box, and theNullable.GetUnderlyingTypecheck on every re-bind. The mis-declaration guard moves into the compile step and still throws on every call; a nullable-selector coercion unboxes to the underlying type first, preserving the CLR's boxed-enum/underlying compatibility the reflection path allowed; environments without IL emission fall back to the interpreter, then to the original reflection read.PropertyInfo(or the already-resolved out-of-DTO name) in one union field resolved throughArgumentPathsonly when a path is first needed — an all-valid bind of scalar and list properties builds no path string and never consults theIArgumentNameProvider; a bound complex property still resolves its one prefix segment, exactly what the eager code paid.Tags[2]) and complex-list element prefixes (Guests[2]) throughIElementPathSource; result lists pre-sized fromICollection<T>.Count.List<>created on first failure, including nested and per-element bindings.FirstClassErrors.RequestBinder.Benchmarks(in the solution, never packed, not a test project): BenchmarkDotNet harness with a hand-written floor, hoisted-selector and expression-vs-delegate micro scenarios, and a README carrying the measured tables and their reading.Measured (byte-exact, back-to-back probes; details in the benchmark README): lists of 10 −35 % allocated (−63 % time), nullable value-type property −104 B (−39 % time), full realistic booking −1 264 B (−16 % time); failure path unchanged (−96 B). Two intended bug-channel deltas, both pinned by tests: a throwing DTO getter surfaces its own exception instead of
TargetInvocationException, and the name provider is consulted at record time, never more than the historical once-per-property bound.Testing
dotnet build FirstClassErrors.slndotnet test FirstClassErrors.slnFirstClassErrors.Analyzers.UnitTests)1 085 tests pass on net10.0 (Release), zero warnings with the CI ratchet flags simulated locally. The net472 floor leg of
FirstClassErrors.RequestBinder.PropertyTestsbuilds locally (-p:EnableNet472Floor=true) but executes only on the Windows CI leg — not run in this Linux session. Benchmarks were run locally before and after; results are inFirstClassErrors.RequestBinder.Benchmarks/README.md.Documentation
doc/updateddoc/handwritten/for-users/README.fr.md) updated if user-facing behavior changedBenchmark README (measurement tables and cost decomposition) and ADR-0023 (EN + FR). No user-facing behavior change, so the user README and its French translation are untouched.
Architecture decisions
Proposed: ADR-0023ADR-0023 was drafted as
Proposedand flipped toAcceptedin this branch on the maintainer's explicit instruction (@Reefact selected the decision in the working session); the acceptance is its own commit so it can be reverted independently if that recording is not wanted.Related issues
Closes #151
🤖 Generated with Claude Code
https://claude.ai/code/session_01Edeoi56UaLxHqwg4q2HLo8
Generated by Claude Code