From 3435a47f1a749b676a03dc5301e2b42e46d67716 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 07:32:53 +0300 Subject: [PATCH 1/5] perf(compilers/openapi): index the mappings a pointer descends 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. --- .../annotation/readers_internal_test.go | 58 ++++++++++++ .../openapi/internal/nodeview/nodeview.go | 66 ++++++++++++- .../nodeview/nodeview_internal_test.go | 94 +++++++++++++++++++ .../nodeview/pointerpath_bench_test.go | 61 ++++++++++++ 4 files changed, 274 insertions(+), 5 deletions(-) create mode 100644 compilers/openapi/internal/nodeview/pointerpath_bench_test.go diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index 4ffaf5f1..210d7cfa 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -10,6 +10,7 @@ import ( "github.com/stretchr/testify/require" yaml "gopkg.in/yaml.v3" + "github.com/dexpace/morphic/compilers/openapi/internal/nodeview" "github.com/dexpace/morphic/ir" ) @@ -447,6 +448,63 @@ func TestRawChildNode_ReadsOnlyAMappingChild(t *testing.T) { assert.Nil(t, RawChildNode(&yaml.Node{Kind: yaml.DocumentNode}, "a"), "nor an empty document") } +// TestRawChildNode_IsNotTheMergeAwareView pins the difference between this +// reader and nodeview's, which is the reason the two exist side by side: what a +// keyword is preserved *as* is what the source spelled at it, while what a +// pointer or a $ref *resolves to* is what the parser will see. +// +// The three cases are the three ways the trees diverge, and each is a keyword +// this package would preserve verbatim. Answering a raw read through the view +// would silently rewrite all of them — a merged keyword would appear at a +// schema that never wrote it, an alias would be replaced by its target, and a +// key written twice would change which of the two survives. +// +// It reaches across packages because that is where the mistake would be made: +// nothing inside either reader can see that the other answers differently. +func TestRawChildNode_IsNotTheMergeAwareView(t *testing.T) { + t.Parallel() + + // use is the mapping under test in each case; the raw read of a top-level + // key is unambiguous, so it is safe to navigate with. + useOf := func(t *testing.T, src string) *yaml.Node { + t.Helper() + use := RawChildNode(yamlNode(t, src), "use") + require.NotNil(t, use, "the fixture must declare a `use` mapping") + return use + } + + t.Run("a merge key contributes nothing to the raw read", func(t *testing.T) { + t.Parallel() + use := useOf(t, "base: &b {title: merged}\nuse:\n <<: *b\n") + + assert.Nil(t, RawChildNode(use, "title"), + "the source wrote `<<`, not `title`, so nothing is preserved at title") + merged := nodeview.New().ChildByToken(use, "title") + require.NotNil(t, merged, "the parser, however, does see it") + assert.Equal(t, "merged", merged.Value) + }) + + t.Run("an aliased value is not dereferenced by the raw read", func(t *testing.T) { + t.Parallel() + use := useOf(t, "base: &b anchored\nuse: {title: *b}\n") + + raw := RawChildNode(use, "title") + require.NotNil(t, raw) + assert.Equal(t, yaml.AliasNode, raw.Kind, "the raw tree keeps the alias the source wrote") + assert.Equal(t, "anchored", nodeview.New().ChildByToken(use, "title").Value, + "where the view stands the anchor in its place") + }) + + t.Run("a repeated key resolves to opposite ends", func(t *testing.T) { + t.Parallel() + use := useOf(t, "use: {title: first, title: last}\n") + + assert.Equal(t, "first", RawChildNode(use, "title").Value, "first matching key wins") + assert.Equal(t, "last", nodeview.New().ChildByToken(use, "title").Value, + "where the view follows the parser and takes the last") + }) +} + // TestRawPropertyNode_NilSchemaReadsNothing pins the nil guard on the schema // side of the same reader, which every caller relies on to ask about a position // that may have no body written at it. diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index dc1e0660..a3b5e384 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -47,6 +47,10 @@ const MergeDepthLimit = 64 // expansion depth, this one caps a document with many merged mappings. Past the // budget the view still answers correctly — it just stops memoizing, trading a // cache hit for a recomputation. +// +// Both of the view's memos are charged to it: a mapping's expanded pairs, and +// the key index keyIndex projects from them. One bound covering both is what +// keeps a second memo from doubling the memory the first one was capped at. const maxCachedPairs = 1 << 21 // DocumentRoot returns the effective root node to scan: the content of a @@ -87,8 +91,13 @@ type Pair struct { // that first reached it. MergeDepthLimit and maxCachedPairs bound the chain // depth and cache size respectively, so unlimited memoization can't trade the // crash for exhausted memory instead. +// +// It memoizes one thing more, for the walk rather than the expansion: keyIndex +// projects a memoized mapping into a key map, so descending a JSON pointer costs +// a map read per token instead of a scan of every pair at each one. type View struct { pairs map[*yaml.Node][]Pair + keys map[*yaml.Node]map[string]*yaml.Node cachedPairs int inFlight map[*yaml.Node]bool exhausted bool @@ -105,6 +114,7 @@ func (v *View) Exhausted() bool { return v.exhausted } func New() *View { return &View{ pairs: map[*yaml.Node][]Pair{}, + keys: map[*yaml.Node]map[string]*yaml.Node{}, inFlight: map[*yaml.Node]bool{}, } } @@ -398,11 +408,7 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { } switch n.Kind { case yaml.MappingNode: - for _, p := range v.MappingPairs(n) { - if p.Key == token { - return p.Val - } - } + return v.mappingChild(n, token) case yaml.SequenceNode: idx, err := strconv.Atoi(token) if err != nil || idx < 0 || idx >= len(n.Content) { @@ -413,6 +419,56 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { return nil } +// mappingChild answers one key of a mapping through the key index, falling back +// to a scan of its pairs for a mapping the index declines to cover. +// +// n is known to be a mapping node here, so it is its own Deref and keys the +// index under the same node MappingPairs memoizes the pairs under. +func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { + pairs := v.MappingPairs(n) + if index := v.keyIndex(n, pairs); index != nil { + return index[token] + } + for _, p := range pairs { + if p.Key == token { + return p.Val + } + } + return nil +} + +// keyIndex returns n's expansion as a key map, building it on first use, or nil +// when the view holds no memo to project. +// +// It is what stops a pointer walk rescanning the mappings it descends through. +// Resolving R references into a components mapping of M entries scans R×M pairs +// without it — quadratic in a document's own size, since both grow together — +// where an index makes each hop a map read. A key map cannot answer differently +// from the scan it replaces: expandContent yields each key once, so the pairs it +// is built from hold no duplicate for a first-match scan to prefer. +// +// The index is charged to the pair budget and gated on it by the same test +// memoize applies, which is what makes one bound cover both memos — and, since +// cachedPairs only ever grows, what makes the index cover exactly the mappings +// whose pairs the view retained: a mapping memoize declined fails this test too, +// so there is no expansion the index keeps and the pairs do not. +func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { + if index, built := v.keys[n]; built { + return index + } + if v.cachedPairs+len(pairs) > maxCachedPairs { + return nil + } + + index := make(map[string]*yaml.Node, len(pairs)) + for _, p := range pairs { + index[p.Key] = p.Val + } + v.keys[n] = index + v.cachedPairs += len(pairs) + return index +} + // Deref follows AliasNode links to the anchored node, bounded against an alias // chain that loops (the anchor-cycle detector reports those separately). func Deref(n *yaml.Node) *yaml.Node { diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index de8affb1..cc79a1f9 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -505,3 +505,97 @@ func TestPointerPath_SegmentCapStopsTheWalk(t *testing.T) { assert.Len(t, path, maxPointerSegments+1, "the walk stops at the cap: the root plus one node per followed token") } + +// TestChildByToken_IndexAgreesWithTheScanItReplaces holds the key index to the +// scan it stands in for, over the mappings whose effective pairs are not their +// literal ones: a merge source, an alias standing in for a whole mapping, and a +// key written twice. +// +// A map answers by key where a scan answers by position, so the two agree only +// because expandContent yields each key once. That is the property under test — +// asserting the index against MappingPairs itself, key by key, is what would +// redden if a duplicate ever survived into an expansion. +// +// Each mapping is read twice through one view, because the two reads take +// different paths: the first builds the index, the second reads it back. +func TestChildByToken_IndexAgreesWithTheScanItReplaces(t *testing.T) { + t.Parallel() + base := ymap(yscalar("a"), yscalar("1"), yscalar("b"), yscalar("2")) + tests := []struct { + name string + n *yaml.Node + }{ + {name: "explicit keys", n: ymap(yscalar("a"), yscalar("1"))}, + {name: "merged keys", n: ymap(ymerge(), yalias(base), yscalar("c"), yscalar("3"))}, + {name: "explicit beats merged", n: ymap(ymerge(), yalias(base), yscalar("a"), yscalar("9"))}, + {name: "alias for the whole value", n: ymap(yscalar("a"), yalias(yscalar("1")))}, + {name: "a key written twice", n: ymap(yscalar("a"), yscalar("1"), yscalar("a"), yscalar("2"))}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + v := New() + pairs := v.MappingPairs(tc.n) + require.NotEmpty(t, pairs, "the fixture must expand to something to compare") + + for _, read := range []string{"builds the index", "reads it back"} { + for _, p := range pairs { + assert.Same(t, p.Val, v.ChildByToken(tc.n, p.Key), "%s: key %q", read, p.Key) + } + assert.Nil(t, v.ChildByToken(tc.n, "absent"), "%s: an unwritten key names nothing", read) + } + }) + } +} + +// TestKeyIndex_IsChargedToThePairBudget pins the index to the bound that already +// covers the pairs it projects. A memo added outside that budget would double +// the memory maxCachedPairs was set to cap. +func TestKeyIndex_IsChargedToThePairBudget(t *testing.T) { + t.Parallel() + n := ymap(yscalar("a"), yscalar("1"), yscalar("b"), yscalar("2")) + v := New() + + require.Len(t, v.MappingPairs(n), 2) + require.Equal(t, 2, v.cachedPairs, "the expansion is charged") + require.Same(t, n.Content[1], v.ChildByToken(n, "a")) + assert.Equal(t, 4, v.cachedPairs, "the index charges its own entries too") + + require.Same(t, n.Content[3], v.ChildByToken(n, "b")) + assert.Equal(t, 4, v.cachedPairs, "a second read builds nothing and charges nothing") +} + +// TestKeyIndex_PastTheBudgetTheScanStillAnswers covers the gate that makes the +// index optional, from both states a read can reach it in: a mapping whose pairs +// the budget also declined, and one memoized while the budget still allowed it. +// +// The second is the case the gate exists for. The first is the case that makes +// the gate sufficient on its own: cachedPairs never falls, so a mapping memoize +// turned away fails the identical test here, and the index cannot end up holding +// an expansion the pairs do not. +func TestKeyIndex_PastTheBudgetTheScanStillAnswers(t *testing.T) { + t.Parallel() + tests := []struct { + name string + expandCold bool + }{ + {name: "the pairs were declined too", expandCold: false}, + {name: "the pairs were memoized first", expandCold: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + n := ymap(yscalar("a"), yscalar("1")) + v := New() + if tc.expandCold { + require.Len(t, v.MappingPairs(n), 1) + } + v.cachedPairs = maxCachedPairs + require.Equal(t, tc.expandCold, len(v.pairs) == 1, "the fixture must set up the state it names") + + assert.Same(t, n.Content[1], v.ChildByToken(n, "a"), "the scan still answers") + assert.Nil(t, v.ChildByToken(n, "absent")) + assert.Empty(t, v.keys, "and nothing was indexed") + }) + } +} diff --git a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go new file mode 100644 index 00000000..ba5ee122 --- /dev/null +++ b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go @@ -0,0 +1,61 @@ +package nodeview + +import ( + "fmt" + "testing" + + yaml "gopkg.in/yaml.v3" +) + +// componentsDoc builds `{components: {schemas: {S0..Sn-1: {type: object}}}}`, +// the shape every internal $ref in an OpenAPI document points into. +func componentsDoc(n int) *yaml.Node { + schemas := &yaml.Node{Kind: yaml.MappingNode} + for i := range n { + schemas.Content = append(schemas.Content, + &yaml.Node{Kind: yaml.ScalarNode, Value: fmt.Sprintf("S%d", i)}, + &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "type"}, + {Kind: yaml.ScalarNode, Value: "object"}, + }}) + } + components := &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "schemas"}, schemas, + }} + return &yaml.Node{Kind: yaml.MappingNode, Content: []*yaml.Node{ + {Kind: yaml.ScalarNode, Value: "components"}, components, + }} +} + +// BenchmarkPointerPath_IntoAWideMapping resolves one pointer per component of a +// components mapping, which is what the reference scan does to a document whose +// every schema is referenced once. +// +// It guards a shape rather than a number. The walk descends the same mapping +// once per reference, so the pairs it reads grow as references × components +// without keyIndex — and those two grow together in a real document, making the +// scan quadratic in the document's own size. Each width here does n times the +// work of a single resolution, so the *per-component* cost is what to read: +// divide by n and compare across widths. It should stay flat, and a run where it +// grows with n is the index no longer being reached. +func BenchmarkPointerPath_IntoAWideMapping(b *testing.B) { + for _, n := range []int{64, 256, 1024} { + root := componentsDoc(n) + pointers := make([]string, n) + for i := range pointers { + pointers[i] = fmt.Sprintf("/components/schemas/S%d", i) + } + + b.Run(fmt.Sprintf("components%d", n), func(b *testing.B) { + b.ReportAllocs() + for b.Loop() { + v := New() // one view per pass: a view never outlives its compile + for _, p := range pointers { + if _, complete := v.PointerPath(root, p); !complete { + b.Fatalf("pointer %s must resolve", p) + } + } + } + }) + } +} From d5295f03126e9c7e51c0e9ecf9ae39c249d98dc1 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 17:45:57 +0300 Subject: [PATCH 2/5] perf(compilers/openapi): index the mappings a pointer descends MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on the same branch, from reviewing what the index actually bought. Four changes, each measured rather than reasoned about. The quadratic was only half removed. refScan.traverse follows every PointerPath with a PureRefTarget call over the same nodes, and that scanned every pair of the mapping ChildByToken had just stopped scanning. Reading the index there too takes the pairs it scans from 330,807 / 1,301,607 / 5,163,207 at N=400/800/1,600 — quadrupling per doubling — to 10,807 / 21,607 / 43,207. The index was a loss at the widths a document is mostly made of, so minIndexedPairs declines one below 16 pairs. Nearly every mapping a pointer descends is narrow and read once; a map allocated to answer one lookup costs more than the scan it replaced. It is no longer charged to maxCachedPairs. An index holds one entry per pair of a mapping the memo kept, so that bound already covers it, while charging it halved a memo that exists to stop a merge chain going cubic — a bound against a hang, not a speed budget. It is gated on the memo's presence rather than on a copy of memoize's arithmetic, which answers the same today and stops tracking it the day that test changes. An empty mapping charged nothing and so was indexed unconditionally, leaving v.keys growing after the budget was spent; the width gate ends that. The warm path reads the built index before re-deriving the pairs, and New no longer allocates a map most views never use. Tests: the agreement table was green with the index disabled, and every fixture was narrower than the gate now admits — both fixed, and the index asserted present. PureRefTarget's test plants a divergence between index and pairs, because both answer alike on any real document and asserting the answer alone cannot see the index bypassed. Corpus output is byte-identical to main across 127 specs. --- .../annotation/readers_internal_test.go | 21 ++- .../openapi/internal/nodeview/nodeview.go | 114 ++++++++++-- .../nodeview/nodeview_internal_test.go | 172 ++++++++++++------ .../nodeview/pointerpath_bench_test.go | 6 +- 4 files changed, 234 insertions(+), 79 deletions(-) diff --git a/compilers/openapi/internal/annotation/readers_internal_test.go b/compilers/openapi/internal/annotation/readers_internal_test.go index e5cfd9a8..0efe40bd 100644 --- a/compilers/openapi/internal/annotation/readers_internal_test.go +++ b/compilers/openapi/internal/annotation/readers_internal_test.go @@ -565,17 +565,21 @@ func TestRawChildNode_IsNotTheMergeAwareView(t *testing.T) { raw := RawChildNode(use, "title") require.NotNil(t, raw) assert.Equal(t, yaml.AliasNode, raw.Kind, "the raw tree keeps the alias the source wrote") - assert.Equal(t, "anchored", nodeview.New().ChildByToken(use, "title").Value, - "where the view stands the anchor in its place") + viewed := nodeview.New().ChildByToken(use, "title") + require.NotNil(t, viewed, "the view resolves the key the raw tree kept aliased") + assert.Equal(t, "anchored", viewed.Value, "where the view stands the anchor in its place") }) t.Run("a repeated key resolves alike on both sides", func(t *testing.T) { t.Parallel() use := useOf(t, "use: {title: first, title: last}\n") - assert.Equal(t, "last", RawChildNode(use, "title").Value, + raw, viewed := RawChildNode(use, "title"), nodeview.New().ChildByToken(use, "title") + require.NotNil(t, raw, "the raw read finds the key") + require.NotNil(t, viewed, "and so does the view") + assert.Equal(t, "last", raw.Value, "the raw read takes the pair the parser reads, not the first written") - assert.Equal(t, "last", nodeview.New().ChildByToken(use, "title").Value, + assert.Equal(t, "last", viewed.Value, "and the view takes the same one, so this is no longer a divergence") }) } @@ -603,9 +607,12 @@ func TestRawChildNode_FindsAKeyWrittenAsAnAlias(t *testing.T) { // last, so returning the first would describe the mapping by a value nothing // else in the compiler uses. // -// Spelled with an alias, because that is how the case is reachable — yaml.v3 -// refuses a key written twice the same way, while an explicit pair and an -// aliased one are two nodes here and one key to the parser. +// Spelled with an alias, because that is how the case is reachable from a parsed +// document — yaml.v3 refuses a key written twice when it decodes into a typed +// value, as the model parse does, so a plainly repeated key faults the document +// before any reader sees it. Decoding into a *yaml.Node, which is how a fixture +// builds a tree directly, accepts one; an explicit pair and an aliased one are +// two nodes here and one key to the parser either way. func TestRawChildNode_RepeatedKeyReadsTheLastPair(t *testing.T) { t.Parallel() for _, tc := range []struct{ name, body, want string }{ diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index e258b39c..84ebce33 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -43,15 +43,17 @@ const maxPointerSegments = 1024 // View.expand and refCycles), not silently truncated. const MergeDepthLimit = 64 -// maxCachedPairs bounds total expanded pairs one View retains, roughly -// 50 MB at 2²¹. It complements MergeDepthLimit: that bound caps one mapping's +// maxCachedPairs bounds total expanded pairs one View retains, on the order of +// 50 MB at 2²¹ for the pairs themselves — more with the key indexes below, whose +// entries cost more apiece than a Pair does. It complements MergeDepthLimit: that bound caps one mapping's // expansion depth, this one caps a document with many merged mappings. Past the // budget the view still answers correctly — it just stops memoizing, trading a // cache hit for a recomputation. // -// Both of the view's memos are charged to it: a mapping's expanded pairs, and -// the key index keyIndex projects from them. One bound covering both is what -// keeps a second memo from doubling the memory the first one was capped at. +// It bounds the key index beside the pairs, without being charged for it twice: +// an index exists only for a mapping whose pairs this retained and holds one +// entry per pair, so what every index holds together is bounded by what this +// already caps. See keyIndex. const maxCachedPairs = 1 << 21 // DocumentRoot returns the effective root node to scan: the content of a @@ -113,9 +115,10 @@ func (v *View) Exhausted() bool { return v.exhausted } // New returns an empty view; a view must not outlive the node tree whose // expansions it caches. func New() *View { + // keys is left nil: most views never index anything, and keyIndex allocates + // it on the first mapping wide enough to earn one. return &View{ pairs: map[*yaml.Node][]Pair{}, - keys: map[*yaml.Node]map[string]*yaml.Node{}, inFlight: map[*yaml.Node]bool{}, } } @@ -311,6 +314,13 @@ func dedupeFirstWins(pairs []Pair) []Pair { // $ref node with a type or properties sibling still drives the crash. The chain // terminates only at a node with no top-level $ref at all. func (v *View) PureRefTarget(n *yaml.Node) (string, bool) { + // Through the index where the walk already built one. This runs on every node + // a pointer descended, immediately after the walk that descended it, so a + // scan here re-reads exactly the mappings ChildByToken just stopped scanning + // — leaving the quadratic the index removes standing in its sibling. + if index, built := v.keys[n]; built { + return pureRefFrom(index["$ref"]) + } return PureRefTargetOf(v.MappingPairs(n)) } @@ -323,14 +333,21 @@ func PureRefTargetOf(pairs []Pair) (string, bool) { if p.Key != "$ref" { continue } - if p.Val == nil || p.Val.Kind != yaml.ScalarNode { - return "", false - } - return InternalPointer(p.Val.Value) + return pureRefFrom(p.Val) } return "", false } +// pureRefFrom is the decision both readings share, once the value written at +// `$ref` is in hand: a key that is absent and one whose value is not a scalar +// are the same answer, so reading the index cannot part company with the scan. +func pureRefFrom(val *yaml.Node) (string, bool) { + if val == nil || val.Kind != yaml.ScalarNode { + return "", false + } + return InternalPointer(val.Value) +} + // InternalPointer reports the JSON pointer a $ref value names inside this // document, and whether it names this document at all. // @@ -464,9 +481,16 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { // mappingChild answers one key of a mapping through the key index, falling back // to a scan of its pairs for a mapping the index declines to cover. // +// The built index is read before the pairs are, because on the path this exists +// to speed up they are the same answer: re-deriving the pairs first would spend +// a Deref and a memo lookup to reach a map read that never needed them. +// // n is known to be a mapping node here, so it is its own Deref and keys the // index under the same node MappingPairs memoizes the pairs under. func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { + if index, built := v.keys[n]; built { + return index[token] + } pairs := v.MappingPairs(n) if index := v.keyIndex(n, pairs); index != nil { return index[token] @@ -479,8 +503,28 @@ func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { return nil } +// minIndexedPairs is the width below which a mapping is scanned rather than +// indexed. +// +// An index costs a map allocation and one insert per pair to save a comparison +// per pair per later read, so a mapping narrow enough, or read few enough times, +// never repays it — and nearly every mapping a pointer descends is both. A +// document is mostly narrow mappings: a schema body, a media-type entry, a +// response. The wide ones a walk returns to over and over are the few a +// components block holds, and those are what this admits. +// +// 16 is where the two costs meet closely enough that either side is cheap; the +// benchmark beside this file carries the widths that show it, narrow ones +// included, so a run that regresses at n=2 or n=8 is this gate having stopped +// paying for itself. +// +// The bound is a width rather than a read count because width is known at the +// first read: counting reads would need its own per-node state, and would still +// index a mapping the walk never returns to. +const minIndexedPairs = 16 + // keyIndex returns n's expansion as a key map, building it on first use, or nil -// when the view holds no memo to project. +// for a mapping this view does not index. // // It is what stops a pointer walk rescanning the mappings it descends through. // Resolving R references into a components mapping of M entries scans R×M pairs @@ -489,16 +533,44 @@ func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { // from the scan it replaces: expandContent yields each key once, so the pairs it // is built from hold no duplicate for a first-match scan to prefer. // -// The index is charged to the pair budget and gated on it by the same test -// memoize applies, which is what makes one bound cover both memos — and, since -// cachedPairs only ever grows, what makes the index cover exactly the mappings -// whose pairs the view retained: a mapping memoize declined fails this test too, -// so there is no expansion the index keeps and the pairs do not. +// It is gated on the pairs memo rather than on a second reading of memoize's +// budget test, which is a choice about coupling rather than about behaviour: at +// the depth this runs at the two select the same mappings. Every read here enters +// expand at depth 0, where isEntryPoint holds, so a truncated expansion is +// memoized deliberately and the only expansion the budget turns away is the one +// memoize turned away for the same reason a moment earlier. A merge cycle is +// refused before memoize is reached, but it yields no pairs at all and is already +// below minIndexedPairs. +// +// The memo is still the better gate, because it is the condition itself rather +// than a restatement of it. An index is a projection of a memo entry, so "is +// there an entry" is what it has to ask; a copy of memoize's arithmetic answers +// the same today and silently stops tracking it the day memoize's own test +// changes. +// +// It is bounded by that memo rather than charged against it. An index holds one +// entry per pair of a mapping the memo kept, so the entries across every index +// are bounded by cachedPairs, which maxCachedPairs already caps. Charging them +// too would halve the memo — and that memo is not a speed budget but the bound +// that keeps a merge chain from going cubic, where the bug being fixed was a +// hang. Halving it would also bring GitHub #404 within reach at half the +// document size, since which mappings keep a memo is what decides the answer +// there. +// +// On #404 itself, which records that the pairs memo is depth-sensitive and asks +// for that to be settled before this lookup work proceeds: this index is a +// projection of that memo and holds no state of its own, so it can be neither +// more nor less correct than the entry it is built from, and it adds no second +// way for a view to answer two things. It inherits #404 rather than widening it, +// and the fix landing there fixes this with it. +// +// It builds unconditionally rather than checking v.keys first: every caller +// reads the built index itself before reaching here, so a hit never arrives. func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { - if index, built := v.keys[n]; built { - return index + if len(pairs) < minIndexedPairs { + return nil } - if v.cachedPairs+len(pairs) > maxCachedPairs { + if _, memoized := v.pairs[n]; !memoized { return nil } @@ -506,8 +578,10 @@ func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { for _, p := range pairs { index[p.Key] = p.Val } + if v.keys == nil { + v.keys = map[*yaml.Node]map[string]*yaml.Node{} + } v.keys[n] = index - v.cachedPairs += len(pairs) return index } diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index b3d353a8..7419e695 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -1,6 +1,7 @@ package nodeview import ( + "fmt" "strings" "testing" @@ -545,96 +546,165 @@ func TestPointerPath_SegmentCapStopsTheWalk(t *testing.T) { "the walk stops at the cap: the root plus one node per followed token") } +// wideMap builds a mapping of minIndexedPairs pairs named k0..kN-1, plus any +// extra pairs given, so a fixture reaches the width at which a view indexes. +func wideMap(extra ...*yaml.Node) *yaml.Node { + var content []*yaml.Node + for i := range minIndexedPairs { + content = append(content, ynode.Scalar(fmt.Sprintf("k%d", i)), ynode.Scalar(fmt.Sprintf("v%d", i))) + } + return ynode.Map(append(content, extra...)...) +} + // TestChildByToken_IndexAgreesWithTheScanItReplaces holds the key index to the // scan it stands in for, over the mappings whose effective pairs are not their -// literal ones: a merge source, an alias standing in for a whole mapping, and a -// key written twice. +// literal ones: a merge source and a key written twice. // // A map answers by key where a scan answers by position, so the two agree only // because expandContent yields each key once. That is the property under test — // asserting the index against MappingPairs itself, key by key, is what would // redden if a duplicate ever survived into an expansion. // -// Each mapping is read twice through one view, because the two reads take -// different paths: the first builds the index, the second reads it back. +// Every fixture is wide enough to be indexed, and the index is asserted present +// before the reads: without that the whole table passes with the index disabled, +// comparing the fallback scan against itself. func TestChildByToken_IndexAgreesWithTheScanItReplaces(t *testing.T) { t.Parallel() - base := ynode.Map(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("b"), ynode.Scalar("2")) + base := wideMap(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("b"), ynode.Scalar("2")) tests := []struct { name string n *yaml.Node }{ - {name: "explicit keys", n: ynode.Map(ynode.Scalar("a"), ynode.Scalar("1"))}, - {name: "merged keys", n: ynode.Map(ynode.Merge(), ynode.Alias(base), ynode.Scalar("c"), ynode.Scalar("3"))}, - {name: "explicit beats merged", n: ynode.Map(ynode.Merge(), ynode.Alias(base), ynode.Scalar("a"), ynode.Scalar("9"))}, - {name: "alias for the whole value", n: ynode.Map(ynode.Scalar("a"), ynode.Alias(ynode.Scalar("1")))}, - {name: "a key written twice", n: ynode.Map(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("a"), ynode.Scalar("2"))}, + {name: "explicit keys", n: wideMap()}, + {name: "merged keys", n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("c"), ynode.Scalar("3"))}, + {name: "explicit beats merged", n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("a"), ynode.Scalar("9"))}, + {name: "an aliased value", n: wideMap(ynode.Scalar("a"), ynode.Alias(ynode.Scalar("1")))}, + {name: "a key written twice", n: wideMap(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("a"), ynode.Scalar("2"))}, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() v := New() pairs := v.MappingPairs(tc.n) - require.NotEmpty(t, pairs, "the fixture must expand to something to compare") + require.GreaterOrEqual(t, len(pairs), minIndexedPairs, "the fixture must be wide enough to index") for _, read := range []string{"builds the index", "reads it back"} { for _, p := range pairs { assert.Same(t, p.Val, v.ChildByToken(tc.n, p.Key), "%s: key %q", read, p.Key) } assert.Nil(t, v.ChildByToken(tc.n, "absent"), "%s: an unwritten key names nothing", read) + require.Contains(t, v.keys, tc.n, "%s: through the index, not the fallback scan", read) } }) } } -// TestKeyIndex_IsChargedToThePairBudget pins the index to the bound that already -// covers the pairs it projects. A memo added outside that budget would double -// the memory maxCachedPairs was set to cap. -func TestKeyIndex_IsChargedToThePairBudget(t *testing.T) { +// TestKeyIndex_IsBoundedByThePairsMemoRatherThanCharged pins the two halves of +// the index's bound: it holds one entry per pair of a mapping the pairs memo +// kept, and it takes nothing from that memo's own budget. +// +// Charging it would halve the memo, and that memo is not a speed budget — it is +// what keeps a merge chain from going cubic, where the bug being fixed was a +// hang. Bounding it by the memo instead costs the memo nothing and still caps +// the index, because a mapping the memo declined is never indexed at all. +func TestKeyIndex_IsBoundedByThePairsMemoRatherThanCharged(t *testing.T) { t.Parallel() - n := ynode.Map(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("b"), ynode.Scalar("2")) + n := wideMap() v := New() - require.Len(t, v.MappingPairs(n), 2) - require.Equal(t, 2, v.cachedPairs, "the expansion is charged") - require.Same(t, n.Content[1], v.ChildByToken(n, "a")) - assert.Equal(t, 4, v.cachedPairs, "the index charges its own entries too") + pairs := v.MappingPairs(n) + require.Len(t, pairs, minIndexedPairs) + require.Equal(t, minIndexedPairs, v.cachedPairs, "the expansion is charged") - require.Same(t, n.Content[3], v.ChildByToken(n, "b")) - assert.Equal(t, 4, v.cachedPairs, "a second read builds nothing and charges nothing") + require.Same(t, n.Content[1], v.ChildByToken(n, "k0")) + require.Contains(t, v.keys, n, "and indexed") + assert.Equal(t, minIndexedPairs, v.cachedPairs, "the index charges the pair budget nothing") + assert.Len(t, v.keys[n], len(pairs), "one entry per pair, so the memo bounds it") } -// TestKeyIndex_PastTheBudgetTheScanStillAnswers covers the gate that makes the -// index optional, from both states a read can reach it in: a mapping whose pairs -// the budget also declined, and one memoized while the budget still allowed it. +// TestKeyIndex_DeclinesWhatThePairsMemoDidNotKeep covers the gate that makes the +// index optional, from each state a read can reach it in. +// +// The three states are the budget, a merge cycle, and a mapping too narrow to +// repay an index. They are asserted together because each reaches the same +// outcome down a different path, and only the first is about the budget at all: +// a cycle yields no pairs, so width turns it away before the memo is consulted. // -// The second is the case the gate exists for. The first is the case that makes -// the gate sufficient on its own: cachedPairs never falls, so a mapping memoize -// turned away fails the identical test here, and the index cannot end up holding -// an expansion the pairs do not. -func TestKeyIndex_PastTheBudgetTheScanStillAnswers(t *testing.T) { +// None of them distinguishes the memo gate from a copy of memoize's budget test, +// which is not what that gate is for — see keyIndex. A test claiming to pin the +// difference would be pinning nothing, since at depth 0 the two select the same +// mappings. +func TestKeyIndex_DeclinesWhatThePairsMemoDidNotKeep(t *testing.T) { t.Parallel() - tests := []struct { - name string - expandCold bool - }{ - {name: "the pairs were declined too", expandCold: false}, - {name: "the pairs were memoized first", expandCold: true}, - } - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - n := ynode.Map(ynode.Scalar("a"), ynode.Scalar("1")) - v := New() - if tc.expandCold { - require.Len(t, v.MappingPairs(n), 1) - } - v.cachedPairs = maxCachedPairs - require.Equal(t, tc.expandCold, len(v.pairs) == 1, "the fixture must set up the state it names") + t.Run("past the budget the scan still answers", func(t *testing.T) { + t.Parallel() + n := wideMap() + v := New() + v.cachedPairs = maxCachedPairs + require.NotContains(t, v.pairs, n, "the pairs were declined") - assert.Same(t, n.Content[1], v.ChildByToken(n, "a"), "the scan still answers") - assert.Nil(t, v.ChildByToken(n, "absent")) - assert.Empty(t, v.keys, "and nothing was indexed") - }) + assert.Same(t, n.Content[1], v.ChildByToken(n, "k0"), "the scan still answers") + assert.Nil(t, v.ChildByToken(n, "absent")) + assert.Empty(t, v.keys, "and nothing was indexed") + }) + + t.Run("a merge cycle expands to nothing, so nothing is indexed", func(t *testing.T) { + t.Parallel() + n := wideMap() + v := New() + v.inFlight[n] = true // expand refuses a mapping already being expanded + + assert.Nil(t, v.ChildByToken(n, "k0"), "an in-flight mapping expands to nothing") + assert.NotContains(t, v.pairs, n, "and is never memoized") + assert.Empty(t, v.keys, "so there is no expansion to index") + }) + + t.Run("a mapping too narrow to repay an index is scanned", func(t *testing.T) { + t.Parallel() + n := ynode.Map(ynode.Scalar("a"), ynode.Scalar("1")) + v := New() + + assert.Same(t, n.Content[1], v.ChildByToken(n, "a"), "the scan answers") + assert.Nil(t, v.ChildByToken(n, "absent")) + assert.Empty(t, v.keys, "and nothing was indexed") + }) +} + +// TestPureRefTarget_ReadsTheIndexWhereTheWalkBuiltOne pins the sibling half of +// the walk to the same index. +// +// refScan.traverse calls this on every node a pointer descended, immediately +// after descending it, so a scan here re-reads exactly the mappings ChildByToken +// stopped scanning — which left the quadratic standing in the sibling of the +// call that removed it. Both answers are asserted through one view: the mapping +// that carries a $ref and the wide one that does not. +func TestPureRefTarget_ReadsTheIndexWhereTheWalkBuiltOne(t *testing.T) { + t.Parallel() + withRef := wideMap(ynode.Scalar("$ref"), ynode.Scalar("#/components/schemas/S")) + without := wideMap() + v := New() + + for _, n := range []*yaml.Node{withRef, without} { + require.NotNil(t, v.ChildByToken(n, "k0"), "the walk descends it") + require.Contains(t, v.keys, n, "so the walk indexed it") } + + target, ok := v.PureRefTarget(withRef) + assert.True(t, ok) + assert.Equal(t, "/components/schemas/S", target) + + _, ok = v.PureRefTarget(without) + assert.False(t, ok, "a mapping with no $ref names no target, index or not") + + // Which side answered, rather than only what it answered. Both readings agree + // on every real document — that is the point of the index — so asserting the + // answer alone passes whether or not the index is consulted, which is how a + // test named for reading it can fail to notice it being bypassed. Planting a + // divergence is what makes the two distinguishable: only a read through the + // index can see this, and only a read through the pairs can miss it. + v.keys[withRef]["$ref"] = ynode.Scalar("#/components/schemas/Planted") + target, ok = v.PureRefTarget(withRef) + require.True(t, ok) + assert.Equal(t, "/components/schemas/Planted", target, + "the index is what answered, not a rescan of the pairs beneath it") } diff --git a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go index ba5ee122..ccd53100 100644 --- a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go +++ b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go @@ -38,8 +38,12 @@ func componentsDoc(n int) *yaml.Node { // work of a single resolution, so the *per-component* cost is what to read: // divide by n and compare across widths. It should stay flat, and a run where it // grows with n is the index no longer being reached. +// The narrow widths are here because they are what a real document is mostly +// made of, and because they are the case an index loses: below minIndexedPairs +// the walk scans, and a run where these regress is that gate having stopped +// paying for itself. func BenchmarkPointerPath_IntoAWideMapping(b *testing.B) { - for _, n := range []int{64, 256, 1024} { + for _, n := range []int{2, 8, 64, 256, 1024} { root := componentsDoc(n) pointers := make([]string, n) for i := range pointers { From 9493a606c92d4e622bc0686b9cc17c4e4be42da2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 17:56:32 +0300 Subject: [PATCH 3/5] perf(compilers/openapi): index a mapping only once a walk returns to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Width was the wrong condition on its own. It says a mapping is expensive to scan, not that anything will scan it twice — and a walk that reads a wide mapping once pays for an index it never reads again. That is not hypothetical. declaresResourceIDAbove builds a view per call and reads each node on the path exactly once, so crossing the width threshold cost it time and half again its allocations for nothing: at 16 pairs, 2,043 ns and 3,416 B against 1,762 ns and 2,240 B just below it. Reuse is now recorded rather than predicted. A mapping arrives with no entry and leaves with a nil one; only a read finding that marker builds the map. An index therefore exists exactly where a walk came back, which is the only place it can be repaid. The same shape now costs 1,768 ns and 2,432 B at width 16 — the build gone, the marker all that is left — while the pointer walk keeps its per-component cost flat, since a walk resolving R references reads each mapping R times. A nil entry cannot be read as an empty index: no mapping below minIndexedPairs is stored, so a marker is the only nil this interprets. Also documents what ChildByToken does not do. MappingPairs and PureRefTarget both dereference an alias standing in for a whole mapping; ChildByToken matches neither arm and answers nil. Every caller reaches a node through a walk that dereferences as it goes, so the difference is unreachable rather than harmless — worth writing down beside a sibling promising the opposite. Corpus output stays byte-identical to main across 127 specs, and the pairs PureRefTargetOf reads still grow linearly: 11,207 / 22,407 / 44,807 at N=400/800/1,600. --- .../openapi/internal/nodeview/nodeview.go | 45 ++++++++++++++----- .../nodeview/nodeview_internal_test.go | 13 ++++-- 2 files changed, 44 insertions(+), 14 deletions(-) diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index 84ebce33..b50a25a9 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -318,7 +318,7 @@ func (v *View) PureRefTarget(n *yaml.Node) (string, bool) { // a pointer descended, immediately after the walk that descended it, so a // scan here re-reads exactly the mappings ChildByToken just stopped scanning // — leaving the quadratic the index removes standing in its sibling. - if index, built := v.keys[n]; built { + if index := v.keys[n]; index != nil { return pureRefFrom(index["$ref"]) } return PureRefTargetOf(v.MappingPairs(n)) @@ -461,6 +461,14 @@ func tokenless(pointer string) bool { // node named by one JSON pointer token, or nil when absent. The mapping arm // reads through the view, so pointer navigation resolves an alias key and an // aliased or merged value exactly as PureRefTarget does. +// +// n itself is not dereferenced, which is where this parts company with its two +// neighbours: MappingPairs and PureRefTarget both take an alias standing in for +// a whole mapping and read the mapping it names, while an alias handed here +// matches neither arm and answers nil. Every caller reaches a node through a +// walk that dereferences as it goes — PointerPath does it at each hop — so the +// difference is unreachable rather than harmless, and it is written down because +// the sibling promising the opposite is one line away. func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { if n == nil { return nil @@ -488,7 +496,7 @@ func (v *View) ChildByToken(n *yaml.Node, token string) *yaml.Node { // n is known to be a mapping node here, so it is its own Deref and keys the // index under the same node MappingPairs memoizes the pairs under. func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { - if index, built := v.keys[n]; built { + if index := v.keys[n]; index != nil { return index[token] } pairs := v.MappingPairs(n) @@ -518,9 +526,12 @@ func (v *View) mappingChild(n *yaml.Node, token string) *yaml.Node { // included, so a run that regresses at n=2 or n=8 is this gate having stopped // paying for itself. // -// The bound is a width rather than a read count because width is known at the -// first read: counting reads would need its own per-node state, and would still -// index a mapping the walk never returns to. +// Width alone is not enough, because it says nothing about reuse: a walk that +// reads a wide mapping once pays for an index it never reads again. That is not +// hypothetical — declaresResourceIDAbove builds a view per call and reads each +// node on the path exactly once, and indexing there cost it time and half again +// its allocations for nothing. So width is one of two conditions; see keyIndex +// for the other. const minIndexedPairs = 16 // keyIndex returns n's expansion as a key map, building it on first use, or nil @@ -564,8 +575,18 @@ const minIndexedPairs = 16 // way for a view to answer two things. It inherits #404 rather than widening it, // and the fix landing there fixes this with it. // -// It builds unconditionally rather than checking v.keys first: every caller -// reads the built index itself before reaching here, so a hit never arrives. +// The second condition is reuse, and it is what the first read records rather +// than predicts. A mapping arrives here with no entry at all the first time and +// leaves with a nil one; only a read that finds that marker builds the map. So an +// index exists exactly where a walk came back, which is the only place it can be +// repaid — and a caller that touches every node once, as the resource-boundary +// walk does, allocates nothing but the markers. +// +// A nil entry cannot be mistaken for an empty index: an empty mapping has no +// pairs, and no mapping below minIndexedPairs is ever stored. +// +// A built index is never returned from here — every caller reads v.keys itself +// before reaching this — so the marker is the only entry this has to interpret. func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { if len(pairs) < minIndexedPairs { return nil @@ -573,14 +594,18 @@ func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { if _, memoized := v.pairs[n]; !memoized { return nil } + if _, seen := v.keys[n]; !seen { + if v.keys == nil { + v.keys = map[*yaml.Node]map[string]*yaml.Node{} + } + v.keys[n] = nil // read once; the next read is what earns an index + return nil + } index := make(map[string]*yaml.Node, len(pairs)) for _, p := range pairs { index[p.Key] = p.Val } - if v.keys == nil { - v.keys = map[*yaml.Node]map[string]*yaml.Node{} - } v.keys[n] = index return index } diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index 7419e695..e85e606a 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -593,7 +593,7 @@ func TestChildByToken_IndexAgreesWithTheScanItReplaces(t *testing.T) { assert.Same(t, p.Val, v.ChildByToken(tc.n, p.Key), "%s: key %q", read, p.Key) } assert.Nil(t, v.ChildByToken(tc.n, "absent"), "%s: an unwritten key names nothing", read) - require.Contains(t, v.keys, tc.n, "%s: through the index, not the fallback scan", read) + require.NotNil(t, v.keys[tc.n], "%s: through the index, not the fallback scan", read) } }) } @@ -617,7 +617,10 @@ func TestKeyIndex_IsBoundedByThePairsMemoRatherThanCharged(t *testing.T) { require.Equal(t, minIndexedPairs, v.cachedPairs, "the expansion is charged") require.Same(t, n.Content[1], v.ChildByToken(n, "k0")) - require.Contains(t, v.keys, n, "and indexed") + require.Nil(t, v.keys[n], "one read records the mapping without indexing it") + require.Same(t, n.Content[3], v.ChildByToken(n, "k1"), "a second read is what builds one") + + require.NotNil(t, v.keys[n], "and indexed") assert.Equal(t, minIndexedPairs, v.cachedPairs, "the index charges the pair budget nothing") assert.Len(t, v.keys[n], len(pairs), "one entry per pair, so the memo bounds it") } @@ -645,7 +648,7 @@ func TestKeyIndex_DeclinesWhatThePairsMemoDidNotKeep(t *testing.T) { assert.Same(t, n.Content[1], v.ChildByToken(n, "k0"), "the scan still answers") assert.Nil(t, v.ChildByToken(n, "absent")) - assert.Empty(t, v.keys, "and nothing was indexed") + assert.Empty(t, v.keys, "and nothing was indexed, not even a marker") }) t.Run("a merge cycle expands to nothing, so nothing is indexed", func(t *testing.T) { @@ -686,7 +689,9 @@ func TestPureRefTarget_ReadsTheIndexWhereTheWalkBuiltOne(t *testing.T) { for _, n := range []*yaml.Node{withRef, without} { require.NotNil(t, v.ChildByToken(n, "k0"), "the walk descends it") - require.Contains(t, v.keys, n, "so the walk indexed it") + require.Nil(t, v.keys[n], "one descent only marks it") + require.NotNil(t, v.ChildByToken(n, "k1"), "and the walk comes back") + require.NotNil(t, v.keys[n], "which is what earns the index") } target, ok := v.PureRefTarget(withRef) From 2b7649cd6e8e4ffe570a7894381b8d57a5f5f4e2 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 18:09:27 +0300 Subject: [PATCH 4/5] docs(compilers/openapi): state the marker bound and rewrap a comment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The reuse markers keyIndex writes are bounded, and more tightly than the indexes they precede: one exists only for a mapping whose pairs the memo kept, and each of those charged at least minIndexedPairs, so v.keys holds at most maxCachedPairs/minIndexedPairs entries. Measured at saturation the bound is exact — 131,072 entries, with 5,000 further mappings adding none — which the bounded-everything rule wants written down rather than derived by a reader. Also drops a memory claim the count bound does not support, and rewraps the line that carried it: a map entry costs more than a Pair, so the ceiling with indexes in play is a multiple of the pairs figure, not that figure. --- .../openapi/internal/nodeview/nodeview.go | 25 ++++++++++++------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/compilers/openapi/internal/nodeview/nodeview.go b/compilers/openapi/internal/nodeview/nodeview.go index b50a25a9..021178ab 100644 --- a/compilers/openapi/internal/nodeview/nodeview.go +++ b/compilers/openapi/internal/nodeview/nodeview.go @@ -44,16 +44,17 @@ const maxPointerSegments = 1024 const MergeDepthLimit = 64 // maxCachedPairs bounds total expanded pairs one View retains, on the order of -// 50 MB at 2²¹ for the pairs themselves — more with the key indexes below, whose -// entries cost more apiece than a Pair does. It complements MergeDepthLimit: that bound caps one mapping's -// expansion depth, this one caps a document with many merged mappings. Past the -// budget the view still answers correctly — it just stops memoizing, trading a -// cache hit for a recomputation. +// 50 MB at 2²¹ for the pairs themselves. It complements MergeDepthLimit: that +// bound caps one mapping's expansion depth, this one caps a document with many +// merged mappings. Past the budget the view still answers correctly — it just +// stops memoizing, trading a cache hit for a recomputation. // -// It bounds the key index beside the pairs, without being charged for it twice: -// an index exists only for a mapping whose pairs this retained and holds one -// entry per pair, so what every index holds together is bounded by what this -// already caps. See keyIndex. +// It bounds the key indexes beside the pairs rather than being charged twice for +// them: an index exists only for a mapping whose pairs this retained and holds +// one entry per pair, so what every index holds together is bounded by what this +// already caps. The bound is a count, and a map entry costs more than a Pair, so +// the memory ceiling with indexes in play is some multiple of the figure above +// rather than that figure. See keyIndex. const maxCachedPairs = 1 << 21 // DocumentRoot returns the effective root node to scan: the content of a @@ -585,6 +586,12 @@ const minIndexedPairs = 16 // A nil entry cannot be mistaken for an empty index: an empty mapping has no // pairs, and no mapping below minIndexedPairs is ever stored. // +// The markers are bounded by the same budget the indexes are, and more tightly: +// one is written only for a mapping whose pairs the memo kept, and every such +// mapping charged at least minIndexedPairs to it, so v.keys holds at most +// maxCachedPairs/minIndexedPairs entries however many mappings a document has. +// Measured at saturation, that bound is exact. +// // A built index is never returned from here — every caller reads v.keys itself // before reaching this — so the marker is the only entry this has to interpret. func (v *View) keyIndex(n *yaml.Node, pairs []Pair) map[string]*yaml.Node { From 96eeddb7deb64b7acababda2fccf9e713851308e Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Thu, 13 Aug 2026 18:17:24 +0300 Subject: [PATCH 5/5] test(compilers/openapi): assert the walk reaches the index it relies on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BenchmarkPointerPath_IntoAWideMapping reads a flat per-component cost as the index being reached, and nothing runs it in CI — so the claim its comment makes was checked by nobody. The half of it that needs no stopwatch is now a test: a walk resolving many pointers through one view must leave an index on the mapping every one of them descends, and if a gate stops admitting that mapping the walk returns to scanning in silence. It bites on all three ways that can happen — the width gate raised past the mapping, the reuse marker never promoted, and the index never built. Also wraps the table rows and comment lines this branch had left longer than anything else in the files, and closes a paragraph break the narrow-width note ran into. --- .../nodeview/nodeview_internal_test.go | 52 +++++++++++++++++-- .../nodeview/pointerpath_bench_test.go | 6 +++ 2 files changed, 53 insertions(+), 5 deletions(-) diff --git a/compilers/openapi/internal/nodeview/nodeview_internal_test.go b/compilers/openapi/internal/nodeview/nodeview_internal_test.go index e85e606a..40c82350 100644 --- a/compilers/openapi/internal/nodeview/nodeview_internal_test.go +++ b/compilers/openapi/internal/nodeview/nodeview_internal_test.go @@ -551,7 +551,8 @@ func TestPointerPath_SegmentCapStopsTheWalk(t *testing.T) { func wideMap(extra ...*yaml.Node) *yaml.Node { var content []*yaml.Node for i := range minIndexedPairs { - content = append(content, ynode.Scalar(fmt.Sprintf("k%d", i)), ynode.Scalar(fmt.Sprintf("v%d", i))) + content = append(content, + ynode.Scalar(fmt.Sprintf("k%d", i)), ynode.Scalar(fmt.Sprintf("v%d", i))) } return ynode.Map(append(content, extra...)...) } @@ -576,17 +577,27 @@ func TestChildByToken_IndexAgreesWithTheScanItReplaces(t *testing.T) { n *yaml.Node }{ {name: "explicit keys", n: wideMap()}, - {name: "merged keys", n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("c"), ynode.Scalar("3"))}, - {name: "explicit beats merged", n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("a"), ynode.Scalar("9"))}, + { + name: "merged keys", + n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("c"), ynode.Scalar("3")), + }, + { + name: "explicit beats merged", + n: wideMap(ynode.Merge(), ynode.Alias(base), ynode.Scalar("a"), ynode.Scalar("9")), + }, {name: "an aliased value", n: wideMap(ynode.Scalar("a"), ynode.Alias(ynode.Scalar("1")))}, - {name: "a key written twice", n: wideMap(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("a"), ynode.Scalar("2"))}, + { + name: "a key written twice", + n: wideMap(ynode.Scalar("a"), ynode.Scalar("1"), ynode.Scalar("a"), ynode.Scalar("2")), + }, } for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { t.Parallel() v := New() pairs := v.MappingPairs(tc.n) - require.GreaterOrEqual(t, len(pairs), minIndexedPairs, "the fixture must be wide enough to index") + require.GreaterOrEqual(t, len(pairs), minIndexedPairs, + "the fixture must be wide enough to index") for _, read := range []string{"builds the index", "reads it back"} { for _, p := range pairs { @@ -673,6 +684,37 @@ func TestKeyIndex_DeclinesWhatThePairsMemoDidNotKeep(t *testing.T) { }) } +// TestPointerPath_ReachesTheIndexOnAWideMapping settles by assertion what the +// benchmark beside it can only show with a stopwatch. +// +// BenchmarkPointerPath_IntoAWideMapping reads a flat per-component cost as the +// index being reached, and nothing runs it in CI. What that cost depends on is +// not a timing question at all: a walk resolving many pointers through one view +// must leave an index on the mapping every one of them descends. If a gate ever +// stops admitting that mapping — a width raised, a reuse marker never promoted — +// the walk silently returns to scanning and only a benchmark nobody runs would +// show it. +// +// The pointers deliberately name distinct components, because it is the mapping +// they share that has to be indexed, not the entries they end at. +func TestPointerPath_ReachesTheIndexOnAWideMapping(t *testing.T) { + t.Parallel() + const width = minIndexedPairs + root := componentsDoc(width) + v := New() + + for i := range width { + _, complete := v.PointerPath(root, fmt.Sprintf("/components/schemas/S%d", i)) + require.True(t, complete, "every pointer resolves") + } + + schemas := v.ChildByToken(v.ChildByToken(root, "components"), "schemas") + require.NotNil(t, schemas, "the fixture has the mapping the walk descends") + require.NotNil(t, v.keys[schemas], + "the mapping every pointer descends is indexed, which a flat per-component cost rests on") + assert.Len(t, v.keys[schemas], width, "one entry per component") +} + // TestPureRefTarget_ReadsTheIndexWhereTheWalkBuiltOne pins the sibling half of // the walk to the same index. // diff --git a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go index ccd53100..151a0000 100644 --- a/compilers/openapi/internal/nodeview/pointerpath_bench_test.go +++ b/compilers/openapi/internal/nodeview/pointerpath_bench_test.go @@ -38,6 +38,12 @@ func componentsDoc(n int) *yaml.Node { // work of a single resolution, so the *per-component* cost is what to read: // divide by n and compare across widths. It should stay flat, and a run where it // grows with n is the index no longer being reached. +// +// Nothing runs this in CI, so that reading is a human's. The half of it that can +// be settled without a stopwatch is settled without one: +// TestPointerPath_ReachesTheIndexOnAWideMapping asserts the walk leaves an index +// on the mapping it descends, which is the condition a flat cost depends on. +// // The narrow widths are here because they are what a real document is mostly // made of, and because they are the case an index loses: below minIndexedPairs // the walk scans, and a run where these regress is that gate having stopped