fix: preserve byte renderer fallback context#844
Merged
stephenamar-db merged 1 commit intoMay 12, 2026
Conversation
Motivation: ByteRenderer's direct materialization fallback could restart stackless materialization with a fresh MaterializeContext, losing the objects already being rendered on the active path. That delayed recursive-object detection and could duplicate partial output before reporting the cycle. Modification: Thread the existing MaterializeContext into the stackless fallback for deep arrays and objects, and widen materializeStackless to package-private so ByteRenderer can reuse it. Add a regression test that distinguishes the fixed behavior from the old duplicate-render path. Result: Deep ByteRenderer fallback keeps cycle tracking and materialization settings consistent with the surrounding render pass while preserving the existing recursive error behavior. References: Source material split from render optimization review in PR databricks#776.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
ByteRenderer's direct materialization path falls back to the generic stackless materializer for very deep arrays/objects. That fallback previously started via
Materializer.apply0, which creates a freshMaterializeContextand drops the objects already being materialized on the active ByteRenderer path.Key Design Decision:
Reuse the active
MaterializeContextwhen switching from ByteRenderer's direct recursive path to stackless materialization. This keepsvisitedObjects, sort behavior, assertion logic, and materialization limits consistent across the fallback boundary.Modification:
Materializer.materializeStackless(v, this, ctx).materializeStacklesstoprivate[sjsonnet]so ByteRenderer can reuse it without exposing it publicly.Benchmark Results:
N/A — correctness-only split from render optimization review material. No performance claim is made for this PR.
Analysis:
The old fallback still eventually reported the same recursive materialization error, but it did extra work with a fresh context first. The regression test uses a recursive object with a large string field and a low fallback threshold: the fixed path flushes one copy of the large field before detecting the cycle, while the old path re-rendered the field again before failing.
Validation:
./mill --no-server --ticker false --color false -j 1 'sjsonnet.jvm[3.3.7]'.test.testOnly sjsonnet.RendererTestspassed../mill --no-server --ticker false --color false -j 1 'sjsonnet.jvm[3.3.7]'.testpassed../mill --no-server --ticker false --color false -j 1 __.testpassed../mill --no-server --ticker false --color false -j 1 __.checkFormatpassed.References:
Result:
ByteRenderer fallback now preserves recursive-object detection context across the direct-to-stackless transition without changing external APIs or successful rendering semantics.