Canonicalize path in DiskInterface::Stat for windows #1420
Conversation
atetubou
force-pushed
the
fix_test
branch
2 times, most recently
from
7957ee6 to
9a3dd31
Compare
| @@ -156,24 +156,34 @@ bool DiskInterface::MakeDirs(const string& path) { | |||
| TimeStamp RealDiskInterface::Stat(const string& path, string* err) const { | |||
| METRIC_RECORD("node stat"); | |||
| #ifdef _WIN32 | |||
| uint64_t slash_bits = 0; | |||
| string canonicalized_path = path; | |||
| if (!CanonicalizePath(&canonicalized_path, &slash_bits, err)) { | |||
There was a problem hiding this comment.
Stat() is called frequently, and CanonicalizePath() isn't super fast – have you measured what this does to empty build times on a large-ish project (my go-to project is Chromium; llvm is probably too small to get statistically relevant numbers) with the stat cache disabled (-d nostatcache)?
There was a problem hiding this comment.
I took empty build time of "ninja -C out/Release -d nostatcache base" on chromium repository with this patch and ninja 1.8.2 on Windows 10 3 times.
With ninja 1.8.2
3.3424782s
3.0806936s
2.9474454s
With this patch
2.9908952s
3.1358611s
3.1082572s
This does not show visible performance change. I think stat on windows itself has much higher cost than CanonicalizePath here.
There was a problem hiding this comment.
Yes, any canonicalization that is necessary should happen before a path reaches the call to stat(). In other places we expect two references to the "same" path to have exactly the same string representation, so it's important to canonicalize early.
There was a problem hiding this comment.
Then let me remove such tests.
#1423
|
Thanks for jumping on this! It looks like it might be slow; maybe we should revert the original patch instead? @moroten, opinions? |
|
Closing for now with #1421 applied instead. It sounds like there's more to do, but maybe not this, not sure. |
|
I think #1421 does not make appveyor happy. Depends on disk write timing, it can pass and fail. So we need to do canonicalization yet or disallow non-canonicalized path in Stat. |
Without canonicalization, "subdir/." and "subdir" are treated like below and test takes stat from unintended files.
Dirname("subdir/.") => "subdir"
Basename("subdir/.") => "."
Dirname("subdir") => ""
Basename("subdir") => "subdir"
This is followup of #1365