-
Notifications
You must be signed in to change notification settings - Fork 88
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.
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
base: master
Are you sure you want to change the base?
Conversation
27ece42 to
79928b0
Compare
| // filename from `DirEntry` here, we store it proactively (we still | ||
| // have to heap allocate it). |
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.
"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.
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.
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?
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.
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.
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.
Fair enough, once it gets into nightly, I'll benchmark it against a previous nightly to see if there's a big difference.
|
With rust-lang/rust#148084 landed:
Still seems worth landing (~600ms vs ~730ms). |
Alternative to #144.
This reduces the duration to glob
**/*.rsin a rustc checkout on my PC from ~1000ms to ~820ms: