-
Notifications
You must be signed in to change notification settings - Fork 68
perf: parallelize Remapping::get_many #314
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
Conversation
async = ["dep:tokio", "dep:futures-util"] | ||
checksum = ["foundry-compilers-core/hasher"] | ||
walkdir = ["dep:walkdir", "foundry-compilers-core/walkdir"] | ||
walkdir = ["rayon", "foundry-compilers-core/walkdir"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find
module and related functions are gated by "walkdir", don't wanna change feature gate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
if entry.path_is_symlink() { | ||
if let Ok(target) = utils::canonicalize(entry.path()) { | ||
if !visited_symlink_dirs.insert(target.clone()) { | ||
if file_type.is_symlink() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this path is untested. foundry-rs/foundry#7820
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed here 8dfa926
b6f22db
to
a1cf3c2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems correct amigo
very cursed piece of code 👍
async = ["dep:tokio", "dep:futures-util"] | ||
checksum = ["foundry-compilers-core/hasher"] | ||
walkdir = ["dep:walkdir", "foundry-compilers-core/walkdir"] | ||
walkdir = ["rayon", "foundry-compilers-core/walkdir"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense
// iterate over all dirs that are children of the root | ||
for dir in walkdir::WalkDir::new(dir) | ||
.sort_by_file_name() | ||
.follow_links(true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont need to follow symlinks explicitly here, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not, lib/x can be a symlink, it should be the same logic as the inner fn
No logic changes except: 2nd commit removes walkdir and doesn't sort by file name anymore because order doesn't matter when we run it in parallel