From 3435a47f1a749b676a03dc5301e2b42e46d67716 Mon Sep 17 00:00:00 2001 From: OmarAlJarrah Date: Sun, 9 Aug 2026 07:32:53 +0300 Subject: [PATCH] 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) + } + } + } + }) + } +}