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

perf: optimize ScopeData structure #1370

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

littledivy
Copy link
Member

18.6 ns per run 53.7 million ops/sec → new_ # main
16.4 ns per run 61.2 million ops/sec → new_ # patch

Copy link
Contributor

@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.

Looks good to me. Did you measure the performance effect of the enum zombie bool change in isolation? Or does it affect the struct size?

Nice to get rid of the Cell: No idea why it can be removed now but nice that it can! <3

@littledivy
Copy link
Member Author

Did I just bring back the Rust compiler bug we had earlier? 👀 #1229

https://github.com/denoland/rusty_v8/actions/runs/7057713260/job/19212435294?pr=1370#step:11:7270

enum ScopeStatus {
Free,
Current { zombie: bool },
Shadowed { zombie: bool },
CurrentZombie,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we #[cfg(debug_assertions)] here, the compiler may have more opportunities to optimize out the zombie code in release mode.

Comment on lines +1283 to 1286
match self_mut.status {
ScopeStatus::CurrentZombie | ScopeStatus::CurrentNoZombie => self_mut,
_ => unreachable!(),
}
Copy link
Contributor

Choose a reason for hiding this comment

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

While we're in here, can we convert this match/unreachable pattern to a debug_assert?

@@ -1811,7 +1817,6 @@ pub(crate) mod data {

#[derive(Debug)]
enum ScopeTypeSpecificData {
None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does Rust use the niche with Option? Is it the same size as ScopeTypeSpecificData?

@mmastrac
Copy link
Contributor

Testing w/ASAN and opt-level=1. It's possible this is good to go and was just miscompiling.

@@ -1295,13 +1295,13 @@ pub(crate) mod data {
previous: Option<NonNull<ScopeData>>,
next: Option<Box<ScopeData>>,
// The 'status' field is also always valid (but does change).
status: Cell<ScopeStatus>,
Copy link
Member

Choose a reason for hiding this comment

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

Are you sure removing this Cell is safe?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants