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.

Sign up for GitHub

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

Jump to bottom

feat(ops): op2 support for v8::Global<...> #58

Merged
merged 13 commits into from
Jul 20, 2023
Merged

feat(ops): op2 support for v8::Global<...> #58

merged 13 commits into from
Jul 20, 2023

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Jul 19, 2023 •
edited

This adds support for v8 globals, along with a benchmark. v8::Globals are constructed from a *mut Isolate that is stashed in OpCtx, making this faster than when using a scope.

test bench_op_v8_global                     ... bench:      37,070 ns/iter (+/- 7,077)
test bench_op_v8_global_scope               ... bench:      44,636 ns/iter (+/- 1,668)
test bench_op_v8_local                      ... bench:       3,289 ns/iter (+/- 75)
test bench_op_v8_local_nofast               ... bench:       6,209 ns/iter (+/- 196)
test bench_op_v8_local_scope                ... bench:      12,640 ns/iter (+/- 440)

In addition, string methods that can use the isolate to skip scope creation have been updated.

@mmastrac mmastrac changed the title WIP feat(ops): op2 support for v8::Global<...> feat(ops): op2 support for v8::Global<...> Jul 19, 2023
bartlomieju
bartlomieju approved these changes Jul 20, 2023
View reviewed changes
core/ops_builtin_v8.rs
@@ -38,9 +38,8 @@ pub fn op_unref_op(scope: &mut v8::HandleScope, promise_id: i32) {
#[op2(core)]
pub fn op_set_promise_reject_callback<'a>(
scope: &mut v8::HandleScope<'a>,
cb: v8::Local<'a, v8::Function>,
#[global] cb: v8::Global<v8::Function>,
Copy link
Member

Choose a reason for hiding this comment

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

Very nice :)

core/runtime/ops.rs Outdated Show resolved Hide resolved
mmastrac and others added 2 commits July 19, 2023 19:04
@mmastrac mmastrac enabled auto-merge (squash) July 20, 2023 01:14
@mmastrac mmastrac mentioned this pull request Jul 19, 2023
Closed
18 tasks
@mmastrac mmastrac merged commit 7882311 into denoland:main Jul 20, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@mmastrac @bartlomieju