Skip to content

fix: avoid exponential blowup when extendedObject becomes a DAG#874

Open
igorwwwwwwwwwwwwwwwwwwww wants to merge 1 commit into
google:masterfrom
igorwwwwwwwwwwwwwwwwwwww:fix-extended-object-dag-blowup
Open

fix: avoid exponential blowup when extendedObject becomes a DAG#874
igorwwwwwwwwwwwwwwwwwwww wants to merge 1 commit into
google:masterfrom
igorwwwwwwwwwwwwwwwwwwww:fix-extended-object-dag-blowup

Conversation

@igorwwwwwwwwwwwwwwwwwwww
Copy link
Copy Markdown

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.

Fixes #785.

Fixes google#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Pathological performance with O(n) objects

1 participant