Skip to content
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

Implement scope analysis and local variables #3988

Merged
merged 7 commits into from
Sep 22, 2024
Merged

Conversation

raskad
Copy link
Member

@raskad raskad commented Sep 7, 2024

This PR implements binding scopes in the AST and uses those to implement function local variables.

This builds on / inlucdes #3942.
This supersedes #3194.

I appreciate that on first sight this seems like a huge change, so let me break down the changes to make the review easier.

Most of the changes in the AST are the addition of Scopes to relevant AST nodes like functions or blocks. Scopes replace CompileTimeEnvironment while moving into the AST. Instead of creating bindings during the bytecode compilation, they are now added after parsing.
The big additions to the AST are in core/ast/src/scope.rs and core/ast/src/scope_analyzer.rs. As expected the Scope is mostly a copy from the CompileTimeEnvironment. The scope analyzer contains new visitor code that creates bindings and looks for bindings that escape their function scope. The escape analysis is needed so that we can switch bindings to local storage or to the function environments as needed.
There are some changes to AST nodes to account for Scopes that had to be added. Also I switched all function bodys from using Script to their dedicated FunctionBody node. That way we can easier work with them in the visitor.

Most changes in the parser should only be to account for changes in AST nodes.

Changes in the engine are the addition of usable registers, lifted from #3942. For the creation of local bindings, I had to create additional opcodes, since currently we have no static way of checking for uninitialized bindings. I added some helper functions in the bytecode compiler to create bindings that are either local or non-local.

One additional note is, that the ...Instantiation functions (GlobalDeclarationInstantiation and friends) are now split between the AST (creation of bindings) and the bytecode compiler (the rest).

There are some significant performance wins that come with this change. I can also imagine some further optimizations based on the static Scope analysis that we can do with this.

Here are my local benchmarks:

Before:

RESULT Richards 72.2
RESULT DeltaBlue 73.6
RESULT Crypto 82.2
RESULT RayTrace 220
RESULT EarleyBoyer 216
RESULT RegExp 60.8
RESULT Splay 242
RESULT NavierStokes 192
SCORE 125

After:

RESULT Richards 89.2
RESULT DeltaBlue 79.7
RESULT Crypto 132
RESULT RayTrace 249
RESULT EarleyBoyer 261
RESULT RegExp 61.3
RESULT Splay 289
RESULT NavierStokes 319
SCORE 156

@raskad raskad added performance Performance related changes and issues parser Issues surrounding the parser execution Issues or PRs related to code execution ast Issue surrounding the abstract syntax tree labels Sep 7, 2024
@raskad raskad added this to the next-release milestone Sep 7, 2024
Copy link

github-actions bot commented Sep 7, 2024

Test262 conformance changes

Test result main count PR count difference
Total 48,494 48,494 0
Passed 43,609 43,609 0
Ignored 1,500 1,500 0
Failed 3,385 3,385 0
Panics 0 0 0
Conformance 89.93% 89.93% 0.00%

@raskad raskad requested a review from a team September 8, 2024 00:11
@jedel1043
Copy link
Member

Impressive work! I'm really liking that we're starting to add some static analysis to the processed code, instead of only relying on runtime optimizations.

About the method of storing the scopes, is adding the scope info directly into the AST the best approach? Because I was thinking that in the future we'd have an architecture similar to rustc's; every node has a NodeId, and we query a global context to access useful information like scopes info, typing info, etc. Can we use something similar to that for the scopes, instead of sprinkling everything with Rc's? Maybe something like assigning an ID to every "scope creator", then doing all the scope analysis after parsing. That will also decouple the parse phase from the analysis phase, and could potentially make it a bit easier in the future to transition to a full NodeId. What do you think?

@raskad
Copy link
Member Author

raskad commented Sep 8, 2024

About the method of storing the scopes, is adding the scope info directly into the AST the best approach? Because I was thinking that in the future we'd have an architecture similar to rustc's; every node has a NodeId, and we query a global context to access useful information like scopes info, typing info, etc. Can we use something similar to that for the scopes, instead of sprinkling everything with Rc's?

That would be pretty similar to a flattened AST just for the metadata, right? I think that should work for the most part. That could also remove the need for the interior mutability in Scope if I'm thinking about it right.

Maybe something like assigning an ID to every "scope creator", then doing all the scope analysis after parsing. That will also decouple the parse phase from the analysis phase, [...]

Just for clarification; the scope analysis is also done after parsing with this approach. You could just not do it if it's not interesting for your AST use case.

[...] and could potentially make it a bit easier in the future to transition to a full NodeId. What do you think?

I definitley would like to give it a try. It could also be a start for a full flat AST. I've not read about it in detail, but I think that might also be a benefit for cache locality and function call depth when working on the AST.
Since from how I imagine it, we would need to have the global context access structure and apis like you mentioned, I think that should probably be in a follow up, as a full throughout feature. It would probably change some of the AST access patterns significantly.
For this PR I would recommend we keep it as is, since the implementation is pretty easy and it pretty much just moves the Rcs from the compiler / runtime up to the AST.

@raskad
Copy link
Member Author

raskad commented Sep 9, 2024

@jedel1043 I looked at your suggestion for using a drop guards for the Registers to avoid calling dealloc (#3942 (comment)). For my usecase here that would be no change, since I only use persistent registers. For normal registers that should work, but we would have to wrap the entires in the RegisterAllocator vec in Cells. Otherwise we would need a mutable borrow and could not have more than one Register at a time.
Do you have any opinions on using that method @HalidOdat? Anything I overlooked?

@HalidOdat
Copy link
Member

HalidOdat commented Sep 9, 2024

@jedel1043 I looked at your suggestion for using a drop guards for the Registers to avoid calling dealloc (#3942 (comment)). [..]

Apologies for not responding to the comment earlier!

[..] Do you have any opinions on using that method @HalidOdat? Anything I overlooked?

Yes, we would need some sort of interior mutability, I think the approach with vec and cell wouldn't work because every register needs to have a reference to the allocator, &RegisterAllocator would trigger a compile error when trying to grow the register vec. Wrapping the allocator Rc<RefCell<RegisterAllocator>> should work.

The mechanism is just for debugging purposes and it ensured that registers have the shortest lifespan since you explicitly have to deallocated it, otherwise it was very easy to miss a register drop and essentially wasting a register. Long lifespan registers use more values on the stack and prevents it from being reused.

We could change the panic into a debug assert, longer lifespan registers don't cause any weird behaviours, the max they'll do is trigger the call stack error earlier in very recursive calls, just less efficient.

EDIT: The double panic could be fixed by checking if we are panic-ing with https://doc.rust-lang.org/std/thread/fn.panicking.html.

@jedel1043
Copy link
Member

@HalidOdat I was more thinking of using &RefCell<RegisterAllocator>, then giving a lifetime to all temporal registers. That would both limit the scope on which every register can live, and also allow mutable modifications of the inner Vec on drops.

@HalidOdat
Copy link
Member

HalidOdat commented Sep 10, 2024

Hmmm... I don't think that's going to work, doing a partial borrow on a struct (ByteCompiler) field is allowed but calling mutable methods on that struct (like .emit(&mut self)) while there is a partial borrow would cause a compile error.

We could split the allocator and pass the &RefCell<RegisterAllocator> everywhere, but this makes the ergonomics somewhat challenging.

Am I missing something?

@jedel1043
Copy link
Member

It would be more like making a struct RegisterAllocator(RefCell<Inner>), then switching the operations to take a &self

@raskad
Copy link
Member Author

raskad commented Sep 10, 2024

I think we should probably postpone the disussion until we get some real experience with non persistent registers via the register VM PR. As @HalidOdat mentioned it is more a static safety feature.
My feeling is that we can probably optimize it way better when we actually know all the use cases. At least that is how I always arrived at removing panics in the past.

@jedel1043
Copy link
Member

Sounds good!

@raskad
Copy link
Member Author

raskad commented Sep 13, 2024

Rebased after #3942 was merged.

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@raskad Amazing work!! This looks pretty much perfect to me! Just some very minor nitpicks :)

TBH I think this is worth doing a release and a blog for it after it's merged, showcasing the performance increase of boa :)

core/engine/src/bytecompiler/mod.rs Outdated Show resolved Hide resolved
core/ast/src/scope_analyzer.rs Show resolved Hide resolved
core/engine/src/vm/opcode/call/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested a review from a team September 19, 2024 20:23
@raskad
Copy link
Member Author

raskad commented Sep 19, 2024

TBH I think this is worth doing a release and a blog for it after it's merged, showcasing the performance increase of boa :)

I will see if I can write up a post :)
I have some further changes based on this, mostly skipping runtime environment allocations when possible, that would be interesting for that post.

cli/src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really impressive work!

@jedel1043 jedel1043 added this pull request to the merge queue Sep 22, 2024
Merged via the queue into main with commit aa09d98 Sep 22, 2024
13 checks passed
@jedel1043 jedel1043 deleted the ast-scopes-local-bindings branch September 22, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ast Issue surrounding the abstract syntax tree execution Issues or PRs related to code execution parser Issues surrounding the parser performance Performance related changes and issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants