Skip to content

fix(compilers/openapi): serve a merge memo only where it fits #453

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
fuad-daoud wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(compilers/openapi): serve a merge memo only where it fits #453

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
137 changes: 91 additions & 46 deletions compilers/openapi/internal/nodeview/nodeview.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,17 +90,19 @@ type Pair struct {
//
// It memoizes each mapping's expansion for the scan's lifetime: without that, a
// merge chain costs O(n) per expansion and O(n) expansions per walk, going
// cubic in chain length — a hang where the bug being fixed was a crash. A
// cached expansion is always the depth-0 expansion, independent of the path
// 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.
// cubic in chain length — a hang where the bug being fixed was a crash. The
// memo is a pure cache: a read answers exactly what a fresh view would answer
// at the same depth, whatever was read before it, which is what makes one view
// safe to share across independent walks (see expansion and View.serves).
// 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
pairs map[*yaml.Node]expansion
keys map[*yaml.Node]map[string]*yaml.Node
cachedPairs int
inFlight map[*yaml.Node]bool
Expand All @@ -119,11 +121,28 @@ 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{},
pairs: map[*yaml.Node]expansion{},
inFlight: map[*yaml.Node]bool{},
}
}

// expansion is one mapping's effective pairs together with what the memo needs
// to know to serve them again: how deep the expansion reached, and whether it
// reached everything.
//
// height is the longest chain of `<<` merges beneath the node — 0 for a mapping
// that merges nothing — and it is what makes a memo entry safe to share. The
// expansion of a node depends on the depth it is reached at, because the depth
// bound truncates from the entry point down: a chain that fits inside the bound
// from one node may not from a node above it. So an entry computed from one
// read is only the answer for another read when the whole chain still fits
// (GitHub #404).
type expansion struct {
pairs []Pair
height int
complete bool
}

// MappingPairs returns the effective pairs of a mapping node, following
// speakeasy's precedence: an explicit key beats one from a merge regardless of
// where the `<<` appears, an earlier merge source beats a later one on a
Expand All @@ -134,14 +153,16 @@ func New() *View {
// directly; a non-mapping node (including nil) yields no pairs. The returned
// slice is the view's own memo — callers must treat it as read-only.
func (v *View) MappingPairs(n *yaml.Node) []Pair {
pairs, _ := v.expand(Deref(n), 0)
return pairs
return v.expand(Deref(n), 0).pairs
}

// expand returns n's effective pairs and whether the expansion is complete —
// false if a merge cycle was broken or MergeDepthLimit was reached. Only a
// complete expansion is memoized: caching an incomplete one could make one
// traversal order silently lose a $ref another would find.
// traversal order silently lose a $ref another would find. And a memoized
// expansion is served only to a read it is the right answer for, which serves
// decides — the memo must not let one traversal order see past a bound another
// would stop at.
//
// Truncation is not contagious — only the node that hit the bound is refused,
// every other mapping still expands in full — because truncation only ever
Expand All @@ -157,29 +178,45 @@ func (v *View) MappingPairs(n *yaml.Node) []Pair {
//
// The in-flight (merge-cycle) case needs no bound of its own: it requires an
// alias to an ancestor, which anchorCycle already refuses before refCycles runs.
func (v *View) expand(n *yaml.Node, depth int) ([]Pair, bool) {
func (v *View) expand(n *yaml.Node, depth int) expansion {
if n == nil || n.Kind != yaml.MappingNode {
return nil, true
return expansion{complete: true}
}
if cached, ok := v.pairs[n]; ok {
return cached, true
if cached, ok := v.pairs[n]; ok && v.serves(cached, depth) {
return cached
}
if v.inFlight[n] {
return nil, false
return expansion{}
}
if depth > MergeDepthLimit {
v.exhausted = true
return nil, false
return expansion{}
}

v.inFlight[n] = true
pairs, complete := v.expandContent(n, depth)
e := v.expandContent(n, depth)
delete(v.inFlight, n)

if complete || v.isEntryPoint(depth) {
v.memoize(n, pairs)
if e.complete || v.isEntryPoint(depth) {
v.memoize(n, e)
}
return pairs, complete
return e
}

// serves reports whether a memoized expansion is the answer a fresh view would
// give a read at this depth, which is the only condition under which the memo
// may answer instead of expanding.
//
// A complete entry expanded its whole chain, so it is the answer wherever that
// chain still fits under the bound; from deeper than that a fresh read would
// truncate, and the memo must not hide the truncation. An incomplete entry was
// kept only because it was an entry point, and an entry point is the one read
// it can stand in for.
func (v *View) serves(e expansion, depth int) bool {
if !e.complete {
return v.isEntryPoint(depth)
}
return depth+e.height <= MergeDepthLimit
}

// isEntryPoint reports whether an expansion that just finished at this depth was
Expand All @@ -193,29 +230,33 @@ func (v *View) isEntryPoint(depth int) bool {
// Declining to cache costs a recomputation and nothing else — the cache is pure
// memoization, so a miss recomputes exactly the same pairs — which makes the
// budget a memory bound the scan can enforce without touching what it reports.
func (v *View) memoize(n *yaml.Node, pairs []Pair) {
if v.cachedPairs+len(pairs) > maxCachedPairs {
func (v *View) memoize(n *yaml.Node, e expansion) {
if v.cachedPairs+len(e.pairs) > maxCachedPairs {
return
}
v.pairs[n] = pairs
v.cachedPairs += len(pairs)
v.pairs[n] = e
v.cachedPairs += len(e.pairs)
}

// expandContent splits a mapping's raw content into the pairs it declares itself
// and the pairs its `<<` keys merge in, then applies the two precedence rules
// that govern them. They point in opposite directions, so they cannot share one
// pass: a repeated explicit key resolves to its last value, while a merged key
// yields to an explicit one and to any earlier merge source.
func (v *View) expandContent(n *yaml.Node, depth int) ([]Pair, bool) {
//
// The height it records is one more than the tallest merge source's, so a
// mapping that merges nothing has height 0.
func (v *View) expandContent(n *yaml.Node, depth int) expansion {
var explicit, merged []Pair
complete := true
e := expansion{complete: true}

for i := 0; i+1 < len(n.Content); i += 2 {
raw, val := n.Content[i], Deref(n.Content[i+1])
if IsMergeKey(raw) {
got, ok := v.mergeSource(val, depth+1)
merged = append(merged, got...)
complete = complete && ok
got := v.mergeSource(val, depth+1)
merged = append(merged, got.pairs...)
e.complete = e.complete && got.complete
e.height = max(e.height, got.height+1)
continue
}
key := Deref(raw)
Expand All @@ -225,7 +266,8 @@ func (v *View) expandContent(n *yaml.Node, depth int) ([]Pair, bool) {
explicit = append(explicit, Pair{Key: key.Value, Val: val})
}

return appendUnseen(dedupeLastWins(explicit), merged), complete
e.pairs = appendUnseen(dedupeLastWins(explicit), merged)
return e
}

// dedupeLastWins keeps the last pair for each key, at that last occurrence's
Expand Down Expand Up @@ -271,18 +313,23 @@ func appendUnseen(base, add []Pair) []Pair {
// mergeSource expands one `<<` value into the pairs it contributes: a mapping is
// a single merge source, a sequence is several with an earlier element taking
// precedence over a later one on a shared key.
func (v *View) mergeSource(val *yaml.Node, depth int) ([]Pair, bool) {
//
// The height of a sequence is its tallest element's: the elements are
// alternatives at one level, not links in a chain.
func (v *View) mergeSource(val *yaml.Node, depth int) expansion {
if val == nil || val.Kind != yaml.SequenceNode {
return v.expand(val, depth)
}
var out []Pair
complete := true
e := expansion{complete: true}
for _, item := range val.Content {
got, ok := v.expand(Deref(item), depth)
out = append(out, got...)
complete = complete && ok
got := v.expand(Deref(item), depth)
out = append(out, got.pairs...)
e.complete = e.complete && got.complete
e.height = max(e.height, got.height)
}
return dedupeFirstWins(out), complete
e.pairs = dedupeFirstWins(out)
return e
}

// IsMergeKey reports whether a raw mapping key node is a `<<` merge key,
Expand Down Expand Up @@ -565,16 +612,14 @@ const minIndexedPairs = 16
// 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.
// hang.
//
// A built index is read without asking serves, and may be: every read of it
// comes from a pointer walk, which enters at depth 0, and at depth 0 the memo
// entry it projects is always the answer — a complete entry fits under the bound
// from wherever it was computed, and an incomplete one was kept only as an entry
// point. The index holds no state of its own, so it can be neither more nor less
// correct than that entry.
//
// 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
Expand Down
104 changes: 102 additions & 2 deletions compilers/openapi/internal/nodeview/nodeview_internal_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,12 +310,13 @@ func TestNodeView_TruncationIsPerNode(t *testing.T) {
func TestNodeView_MemoizeRespectsPairBudget(t *testing.T) {
t.Parallel()
pairs := []Pair{{Key: "a", Val: ynode.Scalar("1")}, {Key: "b", Val: ynode.Scalar("2")}}
e := expansion{pairs: pairs, complete: true}

t.Run("within budget: retained and counted", func(t *testing.T) {
t.Parallel()
v := New()
n := ynode.Map()
v.memoize(n, pairs)
v.memoize(n, e)
assert.Contains(t, v.pairs, n)
assert.Equal(t, len(pairs), v.cachedPairs)
})
Expand All @@ -325,7 +326,7 @@ func TestNodeView_MemoizeRespectsPairBudget(t *testing.T) {
v := New()
v.cachedPairs = maxCachedPairs - 1
n := ynode.Map()
v.memoize(n, pairs)
v.memoize(n, e)
assert.NotContains(t, v.pairs, n, "an entry that would overrun the budget is not kept")
assert.Equal(t, maxCachedPairs-1, v.cachedPairs, "and does not count against it")
})
Expand Down Expand Up @@ -755,3 +756,102 @@ func TestPureRefTarget_ReadsTheIndexWhereTheWalkBuiltOne(t *testing.T) {
assert.Equal(t, "/components/schemas/Planted", target,
"the index is what answered, not a rescan of the pairs beneath it")
}

// TestNodeView_SharedViewAnswersAsAFreshOne pins that a memo hit never changes
// an answer (GitHub #404). The expansion is a function of the node and the depth
// it is reached at, and an entry memoized from a shallow read must not be
// served to a read that reaches the same node too deep to expand it in full —
// otherwise which schema a walk happened to read first decides what the view
// reports, and a view cannot be shared across walks.
func TestNodeView_SharedViewAnswersAsAFreshOne(t *testing.T) {
t.Parallel()

// head -> ... -> leaf, two links past the bound: a read from head truncates.
// inner sits ten links down, where a read from it fits inside the bound.
head := ynode.MergeChain(MergeDepthLimit + 2)
inner := head
for range 10 {
inner = Deref(inner.Content[1])
}

fresh := New()
wantHead := pairMap(fresh.MappingPairs(head))
require.Empty(t, wantHead, "the chain from head is past the bound, so nothing survives")
require.True(t, fresh.Exhausted())

t.Run("a shallow memo is not served to a read that is too deep for it", func(t *testing.T) {
t.Parallel()
v := New()
assert.Equal(t, map[string]string{"leaf": "v"}, pairMap(v.MappingPairs(inner)),
"from inner the chain fits inside the bound")
require.Contains(t, v.pairs, inner, "and is memoized")
assert.False(t, v.Exhausted())

assert.Equal(t, wantHead, pairMap(v.MappingPairs(head)),
"reading head afterwards still truncates where a fresh view does")
assert.True(t, v.Exhausted(), "and still reports it")
})

t.Run("a truncating read does not spoil a later read beneath it", func(t *testing.T) {
t.Parallel()
v := New()
require.Empty(t, v.MappingPairs(head))
assert.Equal(t, map[string]string{"leaf": "v"}, pairMap(v.MappingPairs(inner)),
"inner is read at depth 0 and expands in full, as on a fresh view")
})

t.Run("a memo that fits the reader's budget is served", func(t *testing.T) {
t.Parallel()
v := New()
_ = v.MappingPairs(inner)
retained := v.cachedPairs

// Two links above inner: inner is reached at depth 2, and its own chain
// fits under the bound from there, so the memo answers the read.
assert.Equal(t, map[string]string{"leaf": "v"}, pairMap(v.MappingPairs(linksAbove(inner, 2))))
assert.False(t, v.Exhausted())
assert.Equal(t, retained+2, v.cachedPairs,
"only the two new mappings were memoized; nothing beneath inner was recomputed")
})

// inner's chain is MergeDepthLimit-8 links, so from 8 links above it the whole
// chain is exactly the bound and fits; from 9 it is one past and truncates.
// Both readings must agree with a fresh view's, and the fitting one must be
// the memo answering: refusing it recomputes the same pairs, so only the memo
// count can see a bound drawn one short, where the answer sees one drawn one
// long. Together they fix the bound's meaning.
t.Run("the budget is exact at the bound", func(t *testing.T) {
t.Parallel()
for _, tc := range []struct {
links int
want map[string]string
}{
{8, map[string]string{"leaf": "v"}},
{9, map[string]string{}},
} {
above := linksAbove(inner, tc.links)
fresh := New()
require.Equal(t, tc.want, pairMap(fresh.MappingPairs(above)), "links=%d: fresh", tc.links)

shared := New()
_ = shared.MappingPairs(inner)
retained := shared.cachedPairs
assert.Equal(t, tc.want, pairMap(shared.MappingPairs(above)), "links=%d: after inner", tc.links)
assert.Equal(t, fresh.Exhausted(), shared.Exhausted(), "links=%d: exhausted", tc.links)
if len(tc.want) == 0 {
continue
}
assert.Equal(t, retained+tc.links, shared.cachedPairs,
"links=%d: the memo answered for inner; only the links above it are new", tc.links)
}
})
}

// linksAbove returns a mapping that reaches n through the given number of `<<`
// merges, one mapping per link.
func linksAbove(n *yaml.Node, links int) *yaml.Node {
for range links {
n = ynode.Map(ynode.Merge(), ynode.Alias(n))
}
return n
}
12 changes: 6 additions & 6 deletions compilers/openapi/internal/schema/schema.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1743,12 +1743,12 @@ func componentSchemaAt(c lowering.Ctx, pointer string) *oas3.Schema {
// An incomplete walk needs no separate arm: the path holds the nodes it did
// reach, and a boundary above a pointer that falls off the tree still binds.
//
// The view is built per call and deliberately not shared. nodeview memoizes a
// mapping's merge expansion, and a node first expanded shallowly is served from
// that memo to a later walk that reaches it deeper than MergeDepthLimit would
// allow — so a view outliving one walk makes this answer depend on which schema
// lowered first, which invariant #7 forbids. A per-call view costs one expansion
// per path node and is order-invariant.
// The view is built per call. It was once unsafe to share — a memo entry filled
// by a shallow read answered a later, deeper read that a fresh view would have
// truncated, so a view outliving one walk made this answer depend on which
// schema lowered first (GitHub #404). The memo now records the depth an entry
// is good for, so sharing a view across calls is a cost question rather than a
// correctness one; GitHub #338 carries it.
//
// Known gap: a path node whose own merge chain exceeds MergeDepthLimit expands
// to nothing, so an $id written there is invisible and this reports no boundary
Expand Down
Loading
Loading