-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
enhancements for <filesystem>
#3850
Conversation
As Additionally, Lines 5 to 8 in 2261f7e
|
I will update these benchmarks once I have time.
Does this mean that I only need to provide test cases in the benchmark, and call library functions directly, roughly like this? Example#include <filesystem>
#include "benchmark/benchmark.h"
using namespace std;
namespace fs = std::filesystem;
void BM_lexically_normal(benchmark::State& state) {
const fs::path& p(
LR"(C:\Program Files\Azure Data Studio\resources\app\extensions\bat\snippets\batchfile.code-snippets)");
for (auto _ : state) {
benchmark::DoNotOptimize(p.lexically_normal());
}
}
BENCHMARK(BM_lexically_normal);
// more tests...
BENCHMARK_MAIN(); |
Thanks!
Yep, then we can compare the results before and after proposed changes. |
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 should make calls to std::prev
ADL-proof. As you've changed to use vector
now, I believe we can use _New_end[-2]
/_Vec.end()[-2]
.
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.
That is the only thing I see; this is fantastic otherwise. Thanks so much!
…iew..."; try to improve comment
Thanks, looks great! 😻 🗃️ I pushed minor changes, the most significant being an update to |
LR"(X:DriveRelative)"sv, | ||
LR"(\\server\\\share)"sv, | ||
LR"(STL/.github/workflows/../..)"sv, | ||
LR"(C:\Program Files\Azure Data Studio\resources\app\extensions\bat\snippets\batchfile.code-snippets)"sv, |
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.
Is it better to replace these two test cases with more generic ones?
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 fine to me since it's a realistic Microsoft path. If it were from some other program then I'd request a generic meow-style example.
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.
yes, it is a real path in my computer.
for C:\Program Files\Azure Data Studio...
I find a better one:
C:\Program Files\Microsoft Visual Studio\2022\Community\Common7\IDE\VC\Snippets
(it has the same number of segments, and looks shorter than the last test case(/\server/\share/\a...
, so that it's more obvious that the test cases are increasing in segment size).
(I'm not sure whether I can make pushes to ready-to-merge prs)
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.
If you want to add more tests, it's probably better to do it in a new PR to not reset the review process.
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.
Generally, when a PR has been moved to Ready To Merge, changes should only be pushed when they're big enough to be worth it (fixing bugs or major omissions), or they're small enough to be unquestionably safe like comment typo fixes. You should ping the maintainers who approved, on GitHub and/or Discord, so they notice and can take another look.
Basically, we maintainers love doing work, but we want to focus it on necessary work that keeps PRs flowing and the codebase moving forward. We try to minimize avoidable work, so we prefer to avoid scenarios like:
- When a PR that was ready to go is changed in a way that requires further rework.
- If the change was an attempt to fix a real bug, then this is not annoying - we'd rather pull a PR at the last minute than merge a bug. However, if the change wasn't really necessary, then iterating on it again is extra work.
- When changes are pushed after Ready To Merge without maintainers being notified.
- This is a potential process loophole so we don't like it - we require all codebase modifications to be reviewed by another maintainer so last-minute changes that try to sneak in are not cool. (This is applied uniformly; if maintainer X creates a PR, and maintainer Y approves, then X doesn't get to push further changes without Y taking a look. The only exceptions are fixing build breaks and test failures during mirroring, in which case we notify but don't block on getting re-approval.)
- When changes are pushed after a maintainer has self-assigned the PR and commented "I'm mirroring this to the MSVC-internal codebase, please notify me of any changes", and they aren't for a really good reason.
- Resetting this mirroring process is extra extra work and adds a lot of delay.
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.
hm, thanks for clarification 👀
I'm mirroring this to the MSVC-internal repo - please notify me if any further changes are pushed. |
Thanks for improving the performance of my old implementation here! 😻 🚀 🐇 |
path::lexically_normal
(replacinglist
withvector
)<list>
inclusionBenchmark for lexically_normal
Result
The main bottleneck of the original implementation was memory allocation. As the benchmark shows, the new approach works much better for "long" paths(like
cat/./dog/..
or longer). For very-short paths(likeX:///
) the efficiency gain will not be significant.