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

perf(core): don't access isolate slots for JsRuntimeState #16376

Merged
merged 9 commits into from
Oct 21, 2022
Merged

perf(core): don't access isolate slots for JsRuntimeState #16376

merged 9 commits into from
Oct 21, 2022

Conversation

littledivy
Copy link
Member

@littledivy littledivy commented Oct 21, 2022 •
edited
Loading

example writeFile benchmark:

# before
time 188 ms rate 53191
time 168 ms rate 59523
time 167 ms rate 59880
time 166 ms rate 60240
time 168 ms rate 59523
time 173 ms rate 57803
time 183 ms rate 54644

# after
time 157 ms rate 63694
time 152 ms rate 65789
time 151 ms rate 66225
time 151 ms rate 66225
time 152 ms rate 65789

@littledivy littledivy marked this pull request as ready for review October 21, 2022 04:06
aapoalas
aapoalas reviewed Oct 21, 2022
View reviewed changes
core/runtime.rs Outdated Show resolved Hide resolved
aapoalas
aapoalas reviewed Oct 21, 2022
View reviewed changes
ops/lib.rs Outdated Show resolved Hide resolved
aapoalas
aapoalas reviewed Oct 21, 2022
View reviewed changes
Copy link
Collaborator

@aapoalas aapoalas left a comment

Choose a reason for hiding this comment

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

Does this have any effect on startup time? Not that all the RC copying should be that impactful.

andreubotella
andreubotella reviewed Oct 21, 2022
View reviewed changes
core/runtime.rs
}
}

pub(crate) fn event_loop_pending_state_from_isolate(
Copy link
Contributor

@andreubotella andreubotella Oct 21, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It'd be great to somehow get rid of this duplication, especially since when we reland realms this code will be more complicated since it needs to check the realm state and module map from every realm. For example, the logic here could be moved to an EventLoopPendingState::new(&mut JsRuntimeState, &ModuleMap).

@littledivy littledivy merged commit d461a78 into denoland:main Oct 21, 2022
@andreubotella andreubotella mentioned this pull request Dec 13, 2022
Merged
andreubotella added a commit to andreubotella/deno that referenced this pull request Dec 13, 2022
@andreubotella
chore(core): Deduplicate event_loop_pending_state
b20f8b8 
PR denoland#16376 duplicated the definition of
`JsRuntime::event_loop_pending_state` into a new method
`JsRuntime::event_loop_pending_state_from_isolate`, which just changes
how the state and module map are borrowed, as an optimization to avoid
accessing isolate slots. This change deduplicates the logic of these
two methods into `EventLoopPendingState::new`, which takes borrows of
the arguments, making this much more future-proof.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aapoalas aapoalas aapoalas left review comments

@andreubotella andreubotella andreubotella left review comments

@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.
4 participants
@littledivy @andreubotella @bartlomieju @aapoalas