chore: Make materializer stackless for obj and arr#620
chore: Make materializer stackless for obj and arr#620stephenamar-db merged 1 commit intodatabricks:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors Materializer.apply0 to avoid recursive JVM stack growth when materializing deeply nested objects/arrays, aiming to reduce StackOverflow risk by using an explicit stack.
Changes:
- Replaced recursive object/array materialization with an iterative, stack-based algorithm.
- Added
materializeLeafhelper and stack frame types for object/array traversal. - Added documentation about a supposed TCO boundary and a defensive TailCall check.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
He-Pin
force-pushed
the
materializer
branch
9 times, most recently
from
43bf62b to
40dd542
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
sjsonnet/src/sjsonnet/Materializer.scala:22
Materializernow introduces a new abstract methodstorePos(v: Val). BecauseMaterializeris a public abstract class, adding an abstract member is a source/binary breaking change for any downstream code that subclasses it. Consider makingstorePos(v: Val)a concrete (non-abstract) helper that delegates tostorePos(v.pos)(or a default no-op) so existing subclasses continue to compile unchanged.
abstract class Materializer {
def storePos(pos: Position): Unit
def storePos(v: Val): Unit
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@CertainLach FYI |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
He-Pin
force-pushed
the
materializer
branch
2 times, most recently
from
6e666e7 to
b282d75
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
He-Pin
force-pushed
the
materializer
branch
2 times, most recently
from
08b9f9a to
fc24bbc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
|
I think the current shape is good now. |
Motivation:
This can reduce some stack usage of the jvm native frame
refs:google/jsonnet#1142
on main
with this