Skip to content

Fix recursive deepJoin and flattenDeepArray traversal hangs#1041

Open
He-Pin wants to merge 1 commit into
databricks:masterfrom
He-Pin:fix/deepjoin-cycle
Open

Fix recursive deepJoin and flattenDeepArray traversal hangs#1041
He-Pin wants to merge 1 commit into
databricks:masterfrom
He-Pin:fix/deepjoin-cycle

Conversation

@He-Pin

@He-Pin He-Pin commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replaces recursive std.deepJoin / std.flattenDeepArray array walking with a shared iterative DeepArrayTraversal.
  • Uses an explicit array-frame stack and identity active-set cycle detection, while keeping the direct backing-array fast path for materialized arrays.
  • Avoids the previous closure-based traversal helper and keeps traversal state local to each call for GC/JIT friendliness.
  • Reports recursive arrays through the existing builtin error framing:
    • [std.deepJoin] max stack frames exceeded
    • [std.flattenDeepArray] max stack frames exceeded
  • Moves renderer long scratch buffers from shared companion state to renderer instances so independent interpreters do not share mutable scratch memory.
  • Restores the agent-facing threading-model note in CLAUDE.md.

Current head: bca8902aa3999d78176f9999f413f731e8a56b0f

Performance Notes

  • No per-element closures or iterator pipelines in the hot traversal loop.
  • ArrFrameStack is an array-backed stack and clears popped slots to avoid retaining traversed arrays.
  • JVM/native use java.util.IdentityHashMap; JS/WASM use a small open-addressed identity set to avoid ArrayDeque/collection overhead in the hot path.
  • Generic array views still work through indexed access, with tests covering a true lazy ConcatView case at the 256-element threshold.

Verification

  • rtk ./mill --no-daemon 'sjsonnet.jvm[3.3.8].checkFormat'
  • rtk ./mill --no-daemon 'sjsonnet.js[2.13.18].checkFormat'
  • rtk ./mill --no-daemon 'sjsonnet.jvm[3.3.8].test' sjsonnet.FileTests
  • rtk ./mill --no-daemon 'sjsonnet.js[2.13.18].test' sjsonnet.FileTests
  • rtk ./mill --no-daemon 'sjsonnet.wasm[2.13.18].test' sjsonnet.FileTests
  • rtk ./mill --no-daemon 'sjsonnet.jvm[3.3.8].test' sjsonnet.StdLibOfficialCompatibilityTests
  • rtk ./mill --no-daemon 'sjsonnet.native[2.13.18].compile'
  • Qoder stdout review of the final diff: no must-fix findings.

@He-Pin He-Pin force-pushed the fix/deepjoin-cycle branch 2 times, most recently from 951e137 to 972f73f Compare June 25, 2026 05:33
@He-Pin He-Pin marked this pull request as draft June 25, 2026 18:09
@He-Pin He-Pin force-pushed the fix/deepjoin-cycle branch from 22cd3c5 to 1779389 Compare June 25, 2026 19:04
@He-Pin He-Pin marked this pull request as ready for review June 25, 2026 19:08
Avoid closure-based traversal helpers in std.deepJoin/std.flattenDeepArray and use an explicit array-frame stack with direct backing-array access where available.

Track active arrays by identity so recursive arrays fail deterministically with the existing max-stack style error instead of overflowing the VM stack.

Use platform-specific identity sets for JVM/native and JS/WASM, and cover true lazy ConcatView traversal with a >=256-element concat case.
@He-Pin He-Pin force-pushed the fix/deepjoin-cycle branch from 1779389 to bca8902 Compare June 25, 2026 20:22
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.

1 participant