Skip to content
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

Canonicalize path in DiskInterface::Stat for windows #1420

Closed
wants to merge 1 commit into from
Closed

Canonicalize path in DiskInterface::Stat for windows #1420

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 17 additions & 7 deletions src/disk_interface.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then let me remove such tests.
#1423

ostringstream err_stream;
err_stream << "Stat(" << path << "): Failed to canonicalize path " << *err;
*err = err_stream.str();
return -1;
}

// MSDN: "Naming Files, Paths, and Namespaces"
// http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
if (!path.empty() && path[0] != '\\' && path.size() > MAX_PATH) {
if (!canonicalized_path.empty() && canonicalized_path[0] != '\\' &&
canonicalized_path.size() > MAX_PATH) {
ostringstream err_stream;
err_stream << "Stat(" << path << "): Filename longer than " << MAX_PATH
<< " characters";
err_stream << "Stat(" << canonicalized_path << "): Filename longer than "
<< MAX_PATH << " characters";
*err = err_stream.str();
return -1;
}
if (!use_cache_)
return StatSingleFile(path, err);
return StatSingleFile(canonicalized_path, err);

string dir = DirName(path);
string base(path.substr(dir.size() ? dir.size() + 1 : 0));
string dir = DirName(canonicalized_path);
string base(canonicalized_path.substr(dir.size() ? dir.size() + 1 : 0));
if (base == "..") {
// StatAllFilesInDir does not report any information for base = "..".
base = ".";
dir = path;
dir = canonicalized_path;
}

transform(dir.begin(), dir.end(), dir.begin(), ::tolower);
Expand Down