-
-
Notifications
You must be signed in to change notification settings - Fork 413
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
Decouple Context
from ByteCompiler
#3829
Conversation
Test262 conformance changes
|
a46a04f
to
9608fc5
Compare
d69070a
to
3896d78
Compare
3896d78
to
4b1fc4b
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.
A couple things I missed.
Sort of general question: did you happen to benchmark this? Was sort of curious what impact the new global
opcodes have.
Also, would it make sense for the global opcodes to occur in the instantiation functions like in the spec vs. opcodes?
/// | ||
/// [spec]: https://tc39.es/ecma262/#sec-globaldeclarationinstantiation | ||
#[cfg(feature = "annex-b")] | ||
pub(crate) fn global_declaration_instantiation_context( |
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.
Thought: Potentially move these out of bytecompiler
to script
Now that these are separated, is there any need for these functions to still live in this module (especially if the bytecompiler is eventually moved to it's own crate).
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.
Yup, we can put them in separate locations, the only reason I left them there was because the context and non-context variants are close to each other.
The opcodes execute only once in the execution of a script, when I checked for rust benchmarks as well as quickjs benchmarks I didn't notice any regressions.
The problem with this is that some functions need to already be compiled and instantiated to be put into the global object, that's why it's delayed when we execute the bytecode so we can separate them. It might be possible by having some post The main benefit to having opcodes for them (besides the complexity reduction) is reproducibility of |
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.
Nice work. :)
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.
Given the above, I'm fine with merging this. Nice work on this! 😄
This PR removes the
&mut Context
field from theByteCompiler
making it possible to put the bytecompiler in its own crate (there is still work to moveJsString
and other parts to their own crate so they can be used it both crates, but this is a step in the right direction).This also fixes the some failingeval
test262 tests.EDIT: Oops, Tested with an older version of test262 result, so no bonus passing tests 😅
TODO: