Skip to content

Cache filename for sorting in fill_todo #181

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cache filename for sorting in fill_todo #181

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79928b0
Cache filename for sorting in `fill_todo`
Kobzol Oct 22, 2025
7d385ab
Also use the cached filename for `require_literal_leading_dot` check
Kobzol Oct 25, 2025
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
26 changes: 17 additions & 9 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ doctest!("../README.md");
use std::cmp;
use std::cmp::Ordering;
use std::error::Error;
use std::ffi::OsString;
use std::fmt;
use std::fs;
use std::fs::DirEntry;
Expand Down Expand Up @@ -945,24 +946,31 @@ fn fill_todo(
let dirs = fs::read_dir(path).and_then(|d| {
d.map(|e| {
e.map(|e| {
let path = if curdir {
PathBuf::from(e.path().file_name().unwrap())
// We sort by filename a few lines below. However, it is actually quite
// expensive to extract the filename from PathBuf. Since we already know the
// filename from `DirEntry` here, we store it proactively (we still
// have to heap allocate it).
Comment on lines +951 to +952
Copy link
Member

Choose a reason for hiding this comment

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

"still"? afaict this adds an allocation. So it's surprising that allocating an extra OsString is still cheaper than getting the filename from the path later.

If you keep the whole DirEntry you could call file_name_ref on cfg(unix) to avoid that allocation too.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is a bit surprising, but the benchmark is quite clear on the results. It's one allocation vs potentially tens/hundreds/thousands of iterations of components, which seemingly isn't cheap, perf.-wise.

file_name_ref is unstable, as far as I can see?

Copy link
Member

Choose a reason for hiding this comment

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

file_name_ref is unstable, as far as I can see?

Right, too bad.

Regarding the overhead, maybe wait until my rust PR lands and then benchmark again? The file_name optimization looks like it just might give us that 20% speedup too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, once it gets into nightly, I'll benchmark it against a previous nightly to see if there's a big difference.

// Then we use this cached filename for sorting, and further on work only
// with the full path.
let (path, filename) = if curdir {
(PathBuf::from(e.path().file_name().unwrap()), e.file_name())
} else {
e.path()
(e.path(), e.file_name())
};
PathWrapper::from_dir_entry(path, e)
(PathWrapper::from_dir_entry(path, e), filename)
})
})
.collect::<Result<Vec<_>, _>>()
.collect::<Result<Vec<(PathWrapper, OsString)>, _>>()
});
match dirs {
Ok(mut children) => {
if options.require_literal_leading_dot {
children
.retain(|x| !x.file_name().unwrap().to_str().unwrap().starts_with('.'));
children.retain(|x| {
!x.0.file_name().unwrap().to_str().unwrap().starts_with('.')
});
}
children.sort_by(|p1, p2| p2.file_name().cmp(&p1.file_name()));
todo.extend(children.into_iter().map(|x| Ok((x, idx))));
children.sort_by(|p1, p2| p2.1.cmp(&p1.1));
todo.extend(children.into_iter().map(|x| Ok((x.0, idx))));

// Matching the special directory entries . and .. that
// refer to the current and parent directory respectively
Expand Down