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

Context independent CodeBlocks #3424

Merged
merged 2 commits into from
Dec 3, 2023
Merged

Context independent CodeBlocks #3424

merged 2 commits into from
Dec 3, 2023

Conversation

HalidOdat
Copy link
Member

Fixes/Closes #2626

This allows us to share CodeBlocks between Context, which makes functions sharable between Contexts.

@github-actions
Copy link

github-actions bot commented Oct 26, 2023

Test262 conformance changes

Test result main count PR count difference
Total 95,609 95,609 0
Passed 76,526 76,526 0
Ignored 18,132 18,132 0
Failed 951 951 0
Panics 0 0 0
Conformance 80.04% 80.04% 0.00%

@codecov
Copy link

codecov bot commented Oct 26, 2023

Codecov Report

Attention: 256 lines in your changes are missing coverage. Please review.

Comparison is base (b2223cb) 44.59% compared to head (98f447b) 44.66%.
Report is 5 commits behind head on main.

Files Patch % Lines
boa_engine/src/module/source.rs 0.00% 67 Missing ⚠️
boa_engine/src/vm/code_block.rs 0.00% 32 Missing ⚠️
boa_engine/src/bytecompiler/declarations.rs 59.45% 30 Missing ⚠️
boa_engine/src/bytecompiler/mod.rs 64.38% 26 Missing ⚠️
boa_engine/src/module/mod.rs 0.00% 19 Missing ⚠️
boa_engine/src/bytecompiler/statement/loop.rs 69.44% 11 Missing ⚠️
...ne/src/object/internal_methods/module_namespace.rs 0.00% 11 Missing ⚠️
boa_engine/src/vm/opcode/set/name.rs 47.05% 9 Missing ⚠️
boa_engine/src/module/synthetic.rs 0.00% 6 Missing ⚠️
boa_examples/src/bin/synthetic.rs 0.00% 6 Missing ⚠️
... and 16 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3424      +/-   ##
==========================================
+ Coverage   44.59%   44.66%   +0.07%     
==========================================
  Files         487      487              
  Lines       50601    50640      +39     
==========================================
+ Hits        22563    22620      +57     
+ Misses      28038    28020      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HalidOdat HalidOdat requested a review from a team October 28, 2023 06:00
@HalidOdat HalidOdat added this to the v0.18.0 milestone Oct 28, 2023
@HalidOdat HalidOdat marked this pull request as ready for review October 28, 2023 06:01
@HalidOdat
Copy link
Member Author

Ideally we would make the interner store JsStrings and when we .resolve_expect() it would return a JsString, this would simplify our compiler logic.

@jedel1043
Copy link
Member

Does this affect performance in any way? Just thinking that copying Identifier was pretty cheap, but cloning a JsString involves incrementing a Cell.

The other alternative would be to have an API similar to V8's, where scripts are "linked" to contexts, and you can unlink them and relink them to other contexts if you need to.

@HalidOdat
Copy link
Member Author

HalidOdat commented Oct 28, 2023

Does this affect performance in any way?

Yes, there is a small perf. increase.

main:

PROGRESS Richards
RESULT Richards 35.0
PROGRESS DeltaBlue
RESULT DeltaBlue 39.8
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 54.5
PROGRESS RayTrace
RESULT RayTrace 135
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 114

PR:

PROGRESS Richards
RESULT Richards 36.2
PROGRESS DeltaBlue
RESULT DeltaBlue 41.9
PROGRESS Encrypt
PROGRESS Decrypt
RESULT Crypto 55.5
PROGRESS RayTrace
RESULT RayTrace 140
PROGRESS Earley
PROGRESS Boyer
RESULT EarleyBoyer 116

NOTE: I didn't run all of them, the RegExp takes a lot of time (+10mins). Not sure when it got fixed, probably in #3338?

Just thinking that copying Identifier was pretty cheap, but cloning a JsString involves incrementing a Cell.

I'm fine with keeping Identifier in AST, we should only change the interners internal string store to be JsString instead of InternedStr<u16>, it would remove the back and forth conversions. This would require either moving JsString to a separate crate (probably the better choice) or in boa_interner crate.

The other alternative would be to have an API similar to V8's, where scripts are "linked" to contexts, and you can unlink them and relink them to other contexts if you need to.

Sounds interesting! Is there any resource that describes this? :)

@jedel1043
Copy link
Member

jedel1043 commented Oct 28, 2023

Sounds interesting! Is there any resource that describes this? :)

I don't think V8 has an example on how to use that API, but Script is split into an UnboundScript (the data that doesn't depend on the current context) and the rest of the context-dependent data. Then, UnboundScript has a method to bind the script to a Context:
https://github.com/v8/v8/blob/2465184635ae2c2237ea13afe3bb395eae7f94bd/include/v8-script.h#L62-L65

I think it's slightly more flexible, since an UnboundScript can be shared between many Contexts, and you can always recover the shared UnboundScript from the Script:
https://github.com/v8/v8/blob/7fdc2728f2114b083ebbd24710a32fe4574c0c57/src/api/api.cc#L2153-L2159

@HalidOdat HalidOdat added bug Something isn't working API labels Oct 30, 2023
@HalidOdat HalidOdat force-pushed the cross-context-codeblocks branch 4 times, most recently from 0107b83 to 7838764 Compare November 2, 2023 18:30
This allows us to share `CodeBlock`s between `Context`, which makes
functions sharable between `Context`s.
@jedel1043
Copy link
Member

@jedel1043 and @raskad will review this.

@jedel1043 jedel1043 added the waiting-on-review Waiting on reviews from the maintainers label Nov 29, 2023
Copy link
Member

@raskad raskad left a comment

Choose a reason for hiding this comment

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

Really nice, I just got one comment.

boa_engine/src/builtins/eval/mod.rs Outdated Show resolved Hide resolved
@HalidOdat HalidOdat requested a review from a team December 2, 2023 23:16
Comment on lines +49 to +64
pub(crate) trait ToJsString {
fn to_js_string(&self, interner: &Interner) -> JsString;
}

impl ToJsString for Sym {
fn to_js_string(&self, interner: &Interner) -> JsString {
js_string!(interner.resolve_expect(*self).utf16())
}
}

impl ToJsString for Identifier {
fn to_js_string(&self, interner: &Interner) -> JsString {
self.sym().to_js_string(interner)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

Note for later: see if we can avoid taking interner as a parameter on this, to make it a bit more similar to the ToString trait.

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.

Looks great!

@jedel1043 jedel1043 added this pull request to the merge queue Dec 3, 2023
Merged via the queue into main with commit d8e8c7a Dec 3, 2023
14 checks passed
@jedel1043 jedel1043 deleted the cross-context-codeblocks branch December 4, 2023 03:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API bug Something isn't working waiting-on-review Waiting on reviews from the maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusing Codeblocks between Contexts is not properly supported
3 participants