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: skip visiting if it's out of range #3762

Merged
merged 4 commits into from
Feb 14, 2023
Merged

perf: skip visiting if it's out of range #3762

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6bab504
perf: skip visiting if it's out of range
h-a-n-a Feb 12, 2023
87abfff
perf: update
h-a-n-a Feb 12, 2023
b95c7b6
chore: cr
h-a-n-a Feb 13, 2023
3e5cb84
chore: update
h-a-n-a Feb 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 16 additions & 2 deletions crates/turbopack-ecmascript/src/path_visitor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,29 @@ fn find_range<'a, 'b>(
index: usize,
) -> Option<&'b [(&'a AstPath, &'a dyn VisitorFactory)]> {
// Precondition: visitors is never empty
let start = if visitors.first().unwrap().0[index] >= *kind {
// Fast path: It's likely that the whole range is selected
if visitors.first().unwrap().0[index] > *kind || visitors.last().unwrap().0[index] < *kind {
// Fast path: If ast path of the first visitor is already out of range, then we
// can skip the whole visit.
return None;
}

Copy link
Member

Choose a reason for hiding this comment

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

There could be another fast path for visitors.last().unwrap().0[index] < *kind

Copy link
Member

Choose a reason for hiding this comment

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

I would still keep the old fast path too (after the out of bounds checks you added):

    if visitors.first().unwrap().0[index] >= *kind {
        // Fast path: It's likely that the whole range is selected
        0
    }

It avoids the O(log n) partition_point for that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks great! Done @sokra

let start = if visitors.first().unwrap().0[index] == *kind {
// Fast path: It looks like the whole range is selected
0
} else {
visitors.partition_point(|(path, _)| path[index] < *kind)
};

if start >= visitors.len() {
return None;
}

if visitors[start].0[index] > *kind {
// Fast path: If the starting point is greater than the given kind, it's
// meaningless to visit later.
return None;
}

let end = if visitors.last().unwrap().0[index] == *kind {
// Fast path: It's likely that the whole range is selected
visitors.len()
Expand Down