Tier 1 framework-owned performance costs — escalation brief (connectors-pull-hotel-shopping-tor)

tier1-framework-escalation.md

Tier 1 — Framework-owned performance costs (escalation)

Connector: connectors-pull-hotel-shopping-tor (.NET 10) Scope: Search parse hot path (Quote/Book/Cancel share the same framework contracts). Status: These costs are not fixable in the connector repository. This document is the upstream escalation brief for the framework owners.

Framework versions in use

Package	Version	Owns
Connectors.Core.Application	1.1.139	ResultTryParser, Result.BuildResult
Connectors.Core.Domain	1.1.139	Room, RoomPrice, Option, Price
Connectors.Pull.Hotel.Application	3.7.16	CancelPenaltyManager, CancelPenaltyManagerCached, CancelPenaltyManagerServices
Connectors.Pull.Hotel.Api	3.7.16	connector entry point (direct dependency)

Baseline evidence

Full-suite BottleneckMeasure run (perf_20260824_baseline.md), 100 accommodations × 10 room rates × 3 rates = 3,000 rates per request:

Stage	Time	Allocated
XML deserialization	7.75 ms	3.07 MB
Connector parse pipeline (incl. framework OptionsGenerator.Combine)	6.76 ms	10.35 MB
Total CPU-bound work	≈ 14.5 ms	≈ 13.4 MB

≈ 4.5 KB of garbage per rate. The three costs below are the largest remaining targets after all connector-local optimizations (PR #105, PR #108) have been applied.

---

Issue 1 — XmlSerializer fixed deserialization floor (highest impact)

Mechanism

The framework deserializes supplier XML responses with System.Xml.Serialization.XmlSerializer. This imposes a fixed per-response floor of ~20 µs / ~25 KB regardless of payload content — more than 8× the size of a 3 KB document. The control benchmark (XmlDeserializationBenchmarks, real 3 KB MockServer payload) measured 20.67 µs / 25.75 KB and stayed flat across every before/after run, confirming it is a stable floor, not noise.

Impact

• ~30 % of total CPU-bound time and ~23 % of total allocation on the full Search parse pipeline — the single largest cost, and the largest remaining cost after connector-local work.
• Scales linearly in response count (one deserialization per supplier response).
• The per-response cost is the intrinsic allocation of XmlSerializer.Deserialize itself (object-graph materialization + internal reader/serializer state), not assembly generation: XmlSerializerBase constructs the XmlSerializer instances once as fields (_responseSerializer = new XmlSerializer(response)) and reuses them, and the simple new XmlSerializer(Type) ctor uses .NET's type-keyed assembly cache. The ~20 µs / ~25 KB floor is therefore fundamental to XmlSerializer.Deserialize — not fixable by caching instances (already done), only by switching deserializer.

Proposed upstream fix

1. Preferred: migrate the deserialization path to System.Text.Json (source-generated JsonSerializerContext) after an XML-to-JSON or XML-to-DOM normalization step, or to a streaming System.Xml.XmlReader-based parser that materializes only the needed fields.
2. Fallback: if XmlSerializer must stay, instances are already reused/cached as fields (no per-call reconstruction), so the only remaining lever is to reduce what Deserialize materializes — e.g. a streaming XmlReader pass that reads only the needed fields instead of the full object graph.
3. Whichever path is chosen, correctness must be preserved across all supplier response shapes the connector handles — this is the main risk and the reason it is framework-owned.

Risk

High. A deserializer swap is a large, cross-cutting change with correctness exposure across every supplier response variant. Should be gated on a fixture corpus that covers all response shapes.

---

Issue 2 — out List<T> per-element contract in ResultTryParser (medium impact)

Mechanism

Connectors.Core.Application.Result.ResultTryParser declares

TryParse(TProviderType, out List<T>, out ResultInfo, params object[] extraParams)

and Result.BuildResult requires that shape, looping internally and invoking TryParse once per element. The out List<T> forces each parsed element to be wrapped in its own single-element List<T>. That per-element allocation is inherited from the base-class contract, not chosen by the connector.

RateParser and RoomRateParser both inherit this contract, so every parsed rate and every parsed room-rate pays one single-element List<T> allocation. Measured by RateParserBenchmarks.

Corrected note on params object[] extraParams

An earlier analysis overstated this. Result.BuildResult builds the object[] once at the call site and reuses that same array reference for every element in its internal loop, so it costs one array per BuildResult call, not one per parsed element. Both remaining call sites in this connector pass a whole collection in a single call and pass only reference types (string, string, List<Room>), so no boxing occurs on these paths. The one place that genuinely did box — CancelPoliciesParser, which passed a CancelPolicyData struct — no longer uses BuildResult after PR #105. The sole remaining inherited allocation cost is the out List<T> per element.

Impact

• One single-element List<T> (object header + 1-element backing array ≈ 56 B) per parsed element — per rate, per room-rate.
• Scales linearly in element count; at 3,000 rates this is thousands of single-element lists.
• Cannot be removed without changing the base-class contract.

Proposed upstream fix

1. Preferred: add an overload/alternative that accumulates into a caller-provided List<T> (or returns elements via a callback / Span<T> / ref-struct writer), so BuildResult can collect all elements into one shared list instead of wrapping each.
2. Alternative: change BuildResult to coalesce the per-element out List<T> results into a single list internally, so callers never hold the single-element lists.

Risk

Medium. This is a breaking change to the parser base-class contract; every connector inheriting ResultTryParser is affected. Needs a migration path (e.g., default-implemented interface method, or a new base class with an adapter) so existing connectors keep compiling.

---

Issue 3 — CancelPenaltyManager is always uncached (lowest impact, lowest risk)

Mechanism

• CancelPenaltyManagerServices.UseCancelPenaltyManager unconditionally calls AddSingleton<ICancelPenaltyManager, CancelPenaltyManager>() — the uncached implementation — and ignores its own IConfiguration parameter, so there is no way to opt into caching.
• CancelPenaltyManagerCached : CancelPenaltyManager is internal, memoizes parsed deadlines in a Dictionary<string, CancelPenalty>, and is referenced only by the framework's own benchmarks and unit tests (never in src/ production code or DI registration). It is effectively dead code in production.
• The backing Dictionary is not thread-safe, so registering it as a singleton as-is would be unsafe under concurrent requests.
• Latent correctness bug: CancelPenaltyFromDateWithoutTimeZone and CancelPenaltyFromDateWithTimeZone cache HoursBefore under a key that omits checkIn (e.g. $"{dateWithoutTimeZone}#{dateFormat}#{timeZone}"), but HoursBefore = CalculatedHoursBefore(checkIn, …) depends on checkIn. Registered as a singleton, this would return stale HoursBefore across requests with different check-in dates. (Safe only as a scoped instance with constant checkIn per request.) This — plus the missing thread-safety — is why the cached impl cannot be wired up as-is.

Impact

CancelPoliciesParser calls CancelPenaltyManager.CancelPenaltyFromDateWithTimeZone(...) once per cancellation period per rate, always hitting the uncached path. The same date/timezone strings are re-parsed across rates and across requests with no memoization. Impact scales with (rate count × cancellation periods per rate); materiality depends on the production distribution of cancellation-period cardinality.

Proposed upstream fix

1. Preferred: (a) fix the cache key — include checkIn in the key for the …FromDate… overloads, or stop caching HoursBefore and recompute it from the cached deadline; (b) make the cache thread-safe — replace the backing Dictionary with a bounded ConcurrentDictionary (or a size-capped cache); (c) honour the IConfiguration flag so caching can be opted in; (d) register as scoped (not singleton) unless the key fully captures all inputs. Then register CancelPenaltyManagerCached when the flag is set.
2. Alternative: if caching is not wanted, delete CancelPenaltyManagerCached to remove the dead-code confusion and the false impression that caching is available.

Risk

Low for the delete option; low-medium for the cache option (thread-safety + bounded memory growth + cache-key/invalidation semantics must be correct). The cache fix is self-contained and does not affect the parser contract.

---

Priority

#	Issue	Impact	Risk	Recommended order
1	XmlSerializer floor	~30 % time / ~23 % alloc	High	Largest win; gate on response-shape corpus
2	out List<T> per-element contract	Per-element list, linear in rates	Medium (contract break)	Needs migration path for all connectors
3	CancelPenaltyManager uncached	Per-period-per-rate date math	Low–Medium	Self-contained; can ship independently

Issues 2 and 3 are independent and can be progressed in parallel. Issue 1 is the highest-value but also the highest-risk and should be its own dedicated effort.

What the connector has already done (not framework-owned)

• PR #105: hand-rolled cancel-policies loop (dropped per-period BuildResult boxing), Parameter[]→List<Parameter> to avoid per-rate copy, removed dead allocations, pre-sized lists. ~12 % full-pipeline allocation reduction.
• PR #108: hoisted loop-invariant RoomPrice out of the per-room loop; pre-sized single-element lists. ~4.5 % RateParserBenchmarks allocation reduction.

These are the connector-local ceiling. Further gains require the framework changes above.

References

• Baseline (full-suite BottleneckMeasure): perf_20260824_baseline.md
• Prior connector optimization (PR #105): perf_20260825_reduce-search-allocations.md
• Latest comparison (PR #108): perf_20260826_reduce-rate-room-allocations.md
• Benchmark harness + framework-cost notes: ../../test/Benchmarks/README.md
• Control benchmark (stable XmlSerializer floor): XmlDeserializationBenchmarks

Validation against framework source (E:\travelgate\framework)

Every claim above was checked against the framework source (not just the connector's perf docs). Findings:

Claim	Source	Verdict
ResultTryParser.TryParse is out List<T> + params object[]	connectors-common-connectors_core/src/Connectors.Core.Application/Result/ResultTryParser.cs:19	✅ exact match
Result.BuildResult loops, reuses one object[], forces out List<T> per element	…/Result/Result.Builders.cs:93 (BuildResult), :130 (TryParseElement → tryParseMethod(…, extraParams) then parsedData.AddRange(tgxData))	✅ object[] built once at call site, reused per element; out List<T> per element
XML deserialization uses System.Xml.Serialization.XmlSerializer	…/Serializers/Xml/XmlSerializerBase.cs:39 (new XmlSerializer(response)), :74/:85/:145 (_responseSerializer.Deserialize(…))	✅; instances constructed once as fields (not per call)
UseCancelPenaltyManager registers uncached impl, ignores IConfiguration	connectors-pull-connectors_pull_hotel/…/CancelPenaltyManagerServices.cs:17 (body: AddSingleton<ICancelPenaltyManager, CancelPenaltyManager>(), configuration unused)	✅
CancelPenaltyManagerCached is internal, Dictionary-backed, dead in production	…/CancelPenaltyManagerCached.cs:11,13; referenced only in test/Benchmarks/CancelPenaltyManagerBenchmarks.cs and test/UnitTests/CancelPolicyManagerTests.cs	✅ (also in unit tests, not only benchmarks)
CancelPenaltyManagerCached cache key omits checkIn but caches HoursBefore(checkIn,…)	…/CancelPenaltyManagerCached.cs:43 (key "{dateWithoutTimeZone}#{dateFormat}#{timeZone}") vs base …/CancelPenaltyManager.cs:39 (CalculatedHoursBefore(checkIn, …))	⚠️ latent correctness bug if registered as singleton across differing checkIn

Corrections made to this brief based on the source review:

• Issue 1: removed the "transient serialization assemblies per call" claim — XmlSerializer instances are constructed once as fields and the simple ctor uses .NET's type-keyed assembly cache; the floor is the intrinsic cost of XmlSerializer.Deserialize.
• Issue 3: added the checkIn cache-key correctness bug (above) as a second blocker to wiring up the cached impl, and corrected "benchmarks" → "benchmarks and unit tests".