-
-
Notifications
You must be signed in to change notification settings - Fork 406
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
Execution and Node modularization #392
Conversation
Benchmark for a84b7d1Click to view benchmark
|
Benchmark for 3567431Click to view benchmark
|
Benchmark for ab63b70Click to view benchmark
|
Benchmark for f3a168dClick to view benchmark
|
Benchmark for 8849ae3Click to view benchmark
|
Benchmark for 91b0b79Click to view benchmark
|
dd00036
to
d638746
Compare
Benchmark for 5083e07Click to view benchmark
|
Benchmark for ec9295eClick to view benchmark
|
Benchmark for fdcdf09Click to view benchmark
|
Benchmark for 954ad85Click to view benchmark
|
A small update on the changes I made: This has added a bit of extra complexity in the parser, which brings the times up a bit, but they are clearly minor compared to the huge gains in execution. This should improve further as I continue modularizing more nodes. I also removed the I also added a preliminary version of hoisting, but it's not 100% spec compliant yet. |
ccf0820
to
e600c4c
Compare
Some more changes:
Still many things to do, but this modularization is already showing big performance improvements. In the future, I would like to modify the |
Benchmark for b8899e2Click to view benchmark
|
Benchmark for 9db5584Click to view benchmark
|
Benchmark for 157482bClick to view benchmark
|
Benchmark for c4bc22bClick to view benchmark
|
Benchmark for 36278f7Click to view benchmark
|
eb6e6b5
to
462bf9a
Compare
Benchmark for c1897e3Click to view benchmark
|
Benchmark for df82a0fClick to view benchmark
|
Benchmark for 9c87c48Click to view benchmark
|
005f9b1
to
673483e
Compare
Benchmark for fff8e38Click to view benchmark
|
Benchmark for 2c449a5Click to view benchmark
|
@jasonwilliams, @HalidOdat I think this is ready for review, in the sense that there are some nodes left, but the architecture is there. I'm starting to see that future future PRs will make merges very difficult, so I think we can land a preliminary version of this, and then modularize the rest of the nodes (and the tests) in another PR. This PR notably fixes #376 (with a workaround), #336 and #377. It adds a Let me know what you think :) PS: I also modified all static strings to be |
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 looks simply awesome!!! Good work as usual!
With 122 files changed there will be a lot of merge conflicts with future PRs so we should merge this ASAP :)
Will find some time to take a look at this
So we need that to be merged before this then, is there a link to that PR or issue? |
It was already merged, we only need a release in crates.io :D |
Published |
Great! I updated the dependencies, without the Git dependency, so this is ready to be merged! |
0fced5b
to
5f41f3f
Compare
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.
Looks good to me, lets merge and carry on after
The idea behind this work is to divide the
exec
andast::node
modules in smaller code pieces. I also want to solve #376 in the process, and maybe #377.This is the progress as of now:
Node::ArrayDecl
Node::ArrowFunctionDecl
Node::Assign
Node::BinOp
Node::Block
Node::Break
Node::Call
Node::ConditionalOp
Node::Const
Node::ConstDecl
=>Node::ConstDeclList
Node::Continue
Node::DoWhileLoop
Node::FunctionDecl
Node::FunctionExpr
Node::GetConstField
Node::GetField
Node::ForLoop
Node::If
Node::LetDecl
=>Node::LetDeclList
Node::Local
=>Node::Identifier
Node::New
Node::Object
Node::Return
Node::Switch
Node::Spread
Node::StatementList
(no longer a node)Node::Throw
Node::Try
Node::This
(no change)Node::UnaryOp
Node::VarDecl
=>Node::VarDeclList
Node::WhileLoop
I also added the
Executable
trait, and I'm modifying theExecutor
trait in order for external libraries to provide their own executors. We could also just remove theExecutor
trait if we don't want to offer this option.