-
Notifications
You must be signed in to change notification settings - Fork 21
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Interpreter improvements #3054
Interpreter improvements #3054
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fairly complicated, and you understand this part of the codebase much better than I do, so I'll trust you on this one.
The output seems much better! (speedup, error messages, etc). Seems solid!
I'd like to better understand this part of the codebase later on, but I wouldn't wait on that for merging this. (Conflicts could be a pain, especially for large changes)
This implements the following items from #1932:
Performance benefits are around 20-25% for pure Squiggle code.
I expected more, but seems like we might be limited by
frTypes
performance.Note: I'm using "compiler" for AST -> Expression tranformation, and "interpreter" for Expression -> Value transformation below.
Good things about this PR besides performance:
FrameStack
andStackTrace
, which allowed me to get rid of that shift-by-one issue that I had to deal with in Error locations and stacktraces #1172 (comment)Somewhat worrisome things about this PR:
Expression
type) is now more condensed, and could be harder to debug; stringified IRs only contain stack references, I don't store names there anymore. We still carry AST values in the IR, but my guess is that we should stop doing that over time, and move to separately stored source maps.Things to do before merge: