From f3bb230cf51e69a14544df120655d216ae5c0095 Mon Sep 17 00:00:00 2001 From: Igor Wiedler Date: Fri, 8 May 2026 17:38:09 +0200 Subject: [PATCH] fix: avoid exponential blowup when extendedObject becomes a DAG MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #785. builtinPlus on objects creates an extendedObject whose left and right are uncachedObject pointers. When the same valueObject is used on both sides of '+' — for example via the common pattern acc { [k]+: std.get(acc, k, default) } — left and right end up pointing at the same uncachedObject, and the graph rooted at the new extendedObject is really a DAG with shared subtrees. The recursive walks on uncachedObject (uncachedObjectFieldsVisibility, checkAssertionsHelper) treated the graph as a tree and visited each shared subtree once per incoming edge. Iterated over n folds this produces 2^n visits, so manifesting the result of e.g. a 30-element foldl with the pattern above took ~40s of CPU. This change: * Memoizes uncachedObjectFieldsVisibility per *extendedObject. The result depends only on the (immutable) structure of the object and is safe to cache. The recursive call's result must be copied before being mutated by the caller, so the simpleObject and restrictedObject branches now also return fresh maps. * Adds a lazily-computed hasAssertions() predicate to extendedObject and uses it to skip checkAssertionsHelper entirely for subtrees that contain no assertions. We cannot memoize the walk itself (assertions at different superDepths see different super bindings), but skipping assertion-free subtrees is sufficient to avoid the DAG-as-tree blowup in the common case while preserving semantics when assertions are present. Performance on the reproducer from the issue (n=30): before: 40.5 s after: 18 ms (~2200x) On a real workload (cold-cache 'make generate' on gitlab-com/runbooks, which uses jsonnet-tool / go-jsonnet to render alerting rules, dashboards and reference architectures): before: 391.7 s wall, 1594.7 s CPU after: 327.1 s wall, 1216.4 s CPU (~17% wall, ~24% CPU) Adds a regression test (testdata/object_plus_dag_sharing.jsonnet) based on the minimal reproducer, scaled to n=50 — infeasible before this change, runs in ~15 ms after. --- testdata/object_plus_dag_sharing.golden | 3 + testdata/object_plus_dag_sharing.jsonnet | 19 ++++++ .../object_plus_dag_sharing.linter.golden | 0 value.go | 67 ++++++++++++++++++- 4 files changed, 86 insertions(+), 3 deletions(-) create mode 100644 testdata/object_plus_dag_sharing.golden create mode 100644 testdata/object_plus_dag_sharing.jsonnet create mode 100644 testdata/object_plus_dag_sharing.linter.golden diff --git a/testdata/object_plus_dag_sharing.golden b/testdata/object_plus_dag_sharing.golden new file mode 100644 index 000000000..ea578661b --- /dev/null +++ b/testdata/object_plus_dag_sharing.golden @@ -0,0 +1,3 @@ +{ + "a": { } +} diff --git a/testdata/object_plus_dag_sharing.jsonnet b/testdata/object_plus_dag_sharing.jsonnet new file mode 100644 index 000000000..ff336f74a --- /dev/null +++ b/testdata/object_plus_dag_sharing.jsonnet @@ -0,0 +1,19 @@ +// Regression test for https://github.com/google/go-jsonnet/issues/785 +// +// The pattern below builds an accumulator where each iteration's `+:` +// causes the resulting field value to reference itself, producing an +// extendedObject DAG (left and right of the `+` end up pointing at the +// same uncachedObject). Walking the DAG as a tree leads to exponential +// behaviour in manifestJSON / checkAssertions; we make sure here that +// it now scales linearly and produces the correct value. +local ouch(values) = + std.foldl( + function(acc, value) + local lookup = std.get(acc, "a", {}); + acc { ["a"]+: lookup }, + values, + {} + ); + +local values = [null for x in std.range(1, 50)]; +ouch(values) diff --git a/testdata/object_plus_dag_sharing.linter.golden b/testdata/object_plus_dag_sharing.linter.golden new file mode 100644 index 000000000..e69de29bb diff --git a/value.go b/value.go index d5a657166..1f496fa3b 100644 --- a/value.go +++ b/value.go @@ -548,6 +548,12 @@ type simpleObject struct { func checkAssertionsHelper(i *interpreter, obj *valueObject, curr uncachedObject, superDepth int) error { switch curr := curr.(type) { case *extendedObject: + // Skip whole subtrees that have no assertions to evaluate. + // This avoids walking exponential DAG-as-tree structures + // (see comment on extendedObject) when there's nothing to do. + if !curr.hasAssertions() { + return nil + } err := checkAssertionsHelper(i, obj, curr.right, superDepth) if err != nil { return err @@ -634,9 +640,50 @@ type unboundField interface { // // This represenation allows us to implement "+" in O(1), // but requires going through the tree and trying subsequent leafs for field access. +// +// Note: when the same valueObject is used on both sides of a `+` +// (e.g. `x + x`, which can arise from `acc { f+: acc.f }` patterns), +// `left` and `right` end up pointing to the same uncachedObject. The graph +// of an extendedObject is therefore really a DAG. Naively walking it as a +// tree leads to exponential behaviour, so the recursive walks below +// (uncachedObjectFieldsVisibility, checkAssertionsHelper) memoize their +// results per *extendedObject pointer. type extendedObject struct { left, right uncachedObject totalInheritanceSize int + + // cachedFieldsVisibility caches the result of + // uncachedObjectFieldsVisibility for this *extendedObject. + // nil until populated. + cachedFieldsVisibility fieldHideMap + + // assertionsScanned and assertionsPresent cache whether the subtree + // rooted at this extendedObject contains any simpleObject with + // assertions. This lets checkAssertionsHelper skip walking subtrees + // that have nothing to check, which is essential when the same + // uncachedObject is reachable via many paths (DAG sharing). + assertionsScanned bool + assertionsPresent bool +} + +func (o *extendedObject) hasAssertions() bool { + if !o.assertionsScanned { + o.assertionsPresent = subtreeHasAssertions(o.left) || subtreeHasAssertions(o.right) + o.assertionsScanned = true + } + return o.assertionsPresent +} + +func subtreeHasAssertions(o uncachedObject) bool { + switch o := o.(type) { + case *extendedObject: + return o.hasAssertions() + case *restrictedObject: + return subtreeHasAssertions(o.obj) + case *simpleObject: + return len(o.asserts) > 0 + } + return false } func (o *extendedObject) inheritanceSize() int { @@ -770,10 +817,20 @@ func objectHasField(sb selfBinding, fieldName string) bool { type fieldHideMap map[string]ast.ObjectFieldHide func uncachedObjectFieldsVisibility(obj uncachedObject) fieldHideMap { - r := make(fieldHideMap) switch obj := obj.(type) { case *extendedObject: - r = uncachedObjectFieldsVisibility(obj.left) + // Memoize on the *extendedObject pointer. The result depends only + // on the (immutable) structure of the object, so it's safe to cache. + // This avoids exponential blowup when the same subtree is shared + // across multiple branches (DAG-as-tree). + if obj.cachedFieldsVisibility != nil { + return obj.cachedFieldsVisibility + } + r := make(fieldHideMap) + leftMap := uncachedObjectFieldsVisibility(obj.left) + for k, v := range leftMap { + r[k] = v + } rightMap := uncachedObjectFieldsVisibility(obj.right) for k, v := range rightMap { if v == ast.ObjectFieldInherit { @@ -784,20 +841,24 @@ func uncachedObjectFieldsVisibility(obj uncachedObject) fieldHideMap { r[k] = v } } + obj.cachedFieldsVisibility = r return r case *restrictedObject: + r := make(fieldHideMap, len(obj.retainedFields)) for k, v := range obj.retainedFields { r[k] = v } return r case *simpleObject: + r := make(fieldHideMap, len(obj.fields)) for fieldName, field := range obj.fields { r[fieldName] = field.hide } + return r } - return r + return make(fieldHideMap) } func objectFieldsVisibility(obj *valueObject) fieldHideMap {