Skip to content

perf(compilers/openapi): index the mappings a pointer descends - #379

Open
OmarAlJarrah wants to merge 1 commit into
mainfrom
perf/openapi-index-lookups
Open

perf(compilers/openapi): index the mappings a pointer descends#379
OmarAlJarrah wants to merge 1 commit into
mainfrom
perf/openapi-index-lookups

Conversation

@OmarAlJarrah

Copy link
Copy Markdown
Member

Summary

Resolving an internal $ref walks the pointer from the document root, and each hop scanned every
effective pair of the mapping it descended. The mapping a pointer passes through is
components/schemas, so the scan cost grows with the number of components while the number of
walks grows with the number of references — and both grow together in a real document. Pointer
resolution was therefore quadratic in the document's own size.

nodeview.View already memoizes each mapping's expansion. It now projects that memo into a key map
on first descent, so a hop is a map read instead of a scan. Three properties keep that from being a
behaviour change:

  • The index is built from MappingPairs itself, so it is not a second statement of how a mapping is
    read. expandContent yields each key once, so a map cannot answer differently from the first-match
    scan it replaces — asserted key by key, and reddened by letting a repeated key survive an expansion.
  • Its entries are charged to the existing maxCachedPairs budget and gated on the same test
    memoize applies. One bound still covers everything a view retains, and since cachedPairs only
    grows, a mapping memoize turned away fails the identical test here — so the index can never hold
    an expansion the pairs do not.
  • The view is still a value created per walk. Nothing is memoized across a Compile.

Pairs read while resolving pointers, over specs whose every component is referenced once:

components before after
100 5,550 105
400 82,200 405
800 324,400 805
1,600 1,288,800 1,605
3,200 5,137,600 3,205

Before, that count quadruples per doubling; after, it doubles. On the two specs the issue names it
falls from 63 to 12 (testdata/golden/openapi/petstore.yaml) and from 53 to 16
(testdata/conformance/openapi/allof-oneof-cooccurrence.yaml).

In wall time, BenchmarkPointerPath_IntoAWideMapping — added here to hold the shape rather than the
number — goes from 897 µs to 243 µs per pass over a 1,024-entry mapping, and its per-component cost
stops growing with width (242 → 315 → 876 ns before; 277 → 232 → 237 ns after). The whole
pre-lowering scan phase falls 11.4% at 3,200 components, 6.4% at 1,600 and 5.1% at 400. On petstore
it is unchanged (145.0 → 145.9 µs, +8 allocations), so nothing regresses at ordinary sizes.

What this deliberately does not do

The issue also asks for a per-node key map behind annotation.RawChildNode and RawPropertyNode.
That is not done, because measuring it first showed it costs more than it saves.

RawChildNode is only ever handed an OpenAPI object — a schema, parameter, response, header,
media type or security scheme — whose key set is the spec's fixed field list. It is never handed the
wide mappings a pointer descends. Instrumenting a compile of every source under testdata/ counts
9,715 calls in which the largest mapping scanned holds six pairs and the mean holds 1.81. The
call counts the issue quotes are real, but each call is a scan of about two elements.

Prototyping the index confirmed the arithmetic: a per-node key map takes a petstore compile from
1.207 ms to 1.590 ms (+31.7%), and from 15,004 to 15,085 allocations. Building a map for a mapping
of 1/2/3/6 pairs costs 19/28/35/59 ns against 6.8/9.5/11.2/16.8 ns for the scan it replaces, and the
per-call node→map lookup then costs about as much again as the scan did. Since the whole cost of
these lookups is under 0.3% of a compile, no index can win more than that, and this one loses.

So the plumbing the issue anticipates — carrying an index on lowering.Ctx and widening the
signatures beneath it into annotation — is not introduced. Nothing in this change reaches
lowering, schema, operation or auth, and no call site is left half-converted: the keyword
readers are exactly as they were.

What that half of the issue did surface is that the constraint protecting it was untested.
RawChildNode reads the raw tree — first match wins, no alias dereference, no << expansion —
which is deliberately not what nodeview does, and the corpus cannot see the difference: its only
<< fixture is refused before lowering. Rewriting RawChildNode to answer through the view leaves
every compiler test green. TestRawChildNode_IsNotTheMergeAwareView now pins the three ways the two
trees diverge, and all three of its cases redden under that rewrite.

Two adjacent findings, not touched here: schema.declaresResourceIDAbove and
schema.rawMappingKeys each construct a fresh nodeview.View per call, so neither shares the
memoization with anything. The first is reached only from the $dynamicRef path and neither is hot
on any corpus spec.

Test plan

  • Full gate passes in order: gofmt, go vet, golangci-lint (0 issues), go build, and
    ./scripts/check-coverage.sh at exactly 100%.
  • Output does not move. No golden or conformance snapshot changed and none was regenerated.
    Beyond that, both binaries compiled all 176 sources under testdata/, capturing the emitted
    document, stderr diagnostics and exit code for each; diff -r over the two output trees is empty.
    cmd/morphic-harness testdata — the oracles, including the two-order comparison — produces
    byte-identical output before and after.
  • Each new test was checked to bite by planting the defect it names: removing the budget gate reddens
    TestKeyIndex_PastTheBudgetTheScanStillAnswers, not charging the index reddens
    TestKeyIndex_IsChargedToThePairBudget, letting a repeated key survive an expansion reddens
    TestChildByToken_IndexAgreesWithTheScanItReplaces, and routing RawChildNode through the view
    reddens all three cases of TestRawChildNode_IsNotTheMergeAwareView while the rest of the
    compiler suite stays green.
  • A gate that only checks the pairs are memoized was written and then removed: memoize declines on
    exactly the same budget test, so it could not fire on its own and no planted defect reddened
    anything through it.

Closes #338

Resolving an internal $ref walks the pointer from the document root, and
each hop scanned every effective pair of the mapping it descended. The
mapping a pointer passes through is components/schemas, so the scan cost
grew with the number of components while the number of walks grew with
the number of references -- both of which grow together in a real
document, making pointer resolution quadratic in the document's own size.

nodeview.View already memoizes each mapping's expansion. It now projects
that memo into a key map on first descent, so a hop is a map read rather
than a scan. The index is built from MappingPairs itself, which is what
keeps it from becoming a second statement of how a mapping is read:
expandContent yields each key once, so a map cannot answer differently
from the first-match scan it replaces. Its entries are charged to the
existing pair budget and gated on the same test memoize applies, so one
bound still covers everything the view retains and the index never holds
an expansion the pairs do not.

Measured as pairs read while resolving pointers, over specs whose every
component is referenced once: 5,550 -> 105 at 100 components, 82,200 ->
405 at 400, and 5,137,600 -> 3,205 at 3,200. The scan phase falls 11.4%
at 3,200 components and is unchanged on petstore.

The keyword lookups named in the same issue are deliberately left alone.
RawChildNode reads OpenAPI objects, never the wide mappings a pointer
descends: across the whole corpus it is called 9,715 times and the
largest mapping it ever scans holds six pairs. Indexing them measured
slower, and RawChildNode's raw reading -- first match wins, no alias
dereference, no merge expansion -- is deliberately not what this view
does, so the two now have a test pinning where they diverge.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openapi: keyword and pointer lookups still rescan the nodes the index already walked

1 participant