Cache S101FeatureXmlSource XName instances (perf-report P4)#162
Open
philliphoff wants to merge 1 commit into
Open
Cache S101FeatureXmlSource XName instances (perf-report P4)#162philliphoff wants to merge 1 commit into
philliphoff wants to merge 1 commit into
Conversation
Per the perf report (s101-real-cold and s101-real-warm scenarios),
String.Concat(string, string, string) accounted for 50% / 43% of
all managed allocations during S-101 portrayal. The driver was
S101FeatureXmlSource constructing fresh XName instances for every
geometry/attribute element of every feature — most of which fan
out into MakePointElement, called once per coordinate.
XNamespace+string already memoises through XNamespace.GetName, so
the savings here are smaller than a naive read of the report would
suggest. But the cached XName fields:
- skip the per-call GetName hash lookup;
- keep the names rooted by a Gen2 static field, so they never
participate in GC promotion;
- and, for XAttribute names, route through the XName-typed ctor
instead of the string-typed one, avoiding an extra
XNamespace.None.GetName(name) lookup per attribute.
Behaviourally identical: same namespace, same local names, same
XML output. Existing pipeline + dataset tests pass (49 + 29).
This is option (a) from the optimisation plan. Validation belongs
to a re-run of s101-real-cold / s101-real-warm against this branch
(perf-report Track-A); we expect the String.Concat(s,s,s) row to
drop out of the top-N allocation table. If it doesn't, the next
candidate is option (b) — bypassing XLinq for the FeatureXml
construction entirely.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
Performance Gate✅ PASSED — no regressions. Threshold: 10.0%, MAD multiplier (k): 3.0, retry-zone mult: 2.0× Scenario summary
exchange-set-openIteration statistics
Spans (sum of all iterations)
Metrics
s101-portray-coldIteration statistics
Spans (sum of all iterations)
Metrics
s101-portray-warmIteration statistics
Spans (sum of all iterations)
Metrics
s101-real-coldIteration statistics
s101-real-warmIteration statistics
s101-render-warmIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverageIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-openIteration statistics
Spans (sum of all iterations)
Metrics
s102-coverage-render-largeIteration statistics
Spans (sum of all iterations)
Metrics
s102-real-warmIteration statistics
s111-real-warmIteration statistics
s124-vectorIteration statistics
Spans (sum of all iterations)
Metrics
s201-vectorIteration statistics
Spans (sum of all iterations)
Metrics
Generated by EncDotNet.S100.PerfReport gate command |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements P4 from the optimisation plan / perf-report §3 finding #4.
Evidence
The perf-report flagged
String.Concat(string, string, string)as 50% (cold) / 43% (warm) of all managed allocations during S-101 portrayal of the real UKHO trial cells. The driver wasS101FeatureXmlSourceconstructing freshXNameinstances for every geometry/attribute element of every feature — most fanning out intoMakePointElement, which is called once per coordinate.Change
Factored every
S100Ns + "Foo"and string-typed attribute name inS101FeatureXmlSourceinto staticXNamefields (option (a) from the plan). Net diff: +41 / −16 in one file.Why this should help even though
XNamespace.GetNamealready memoisesXNamespace + stringalready routes through a hash-table cache, so the savings are smaller than a naive read of the report would suggest. But cachedXNamefields:GetNamehash lookup;XAttributenames, route through theXName-typed constructor instead of the string-typed one, avoiding an additionalXNamespace.None.GetName(name)lookup per attribute.Validation
Behaviourally identical — same namespace URI, same local names, same XML output.
tests/EncDotNet.S100.Pipelines.Tests— 49/49 pass.tests/EncDotNet.S100.Datasets.S101.Tests— 29/29 pass.The numerical impact belongs to a re-run of the perf-report
s101-real-coldands101-real-warmscenarios. Expectation: theString.Concat(string, string, string)row drops out of the allocation top-N. If it doesn't, the next candidate (per the plan's option (b)) is bypassing XLinq forFeatureXmlconstruction entirely — but that's a much larger change and shouldn't be done speculatively.Out of scope
Other allocation hot-spots from the same scenarios (e.g. P6
MetricStreamIdentity, P5ClipPatternsByPriority) are deliberately deferred — each will get its own surgical PR with matching before/after measurement.