diff options
author | 2018-06-25 01:10:26 -0700 | |
---|---|---|
committer | 2018-06-25 01:12:27 -0700 | |
commit | da91730760ac9078183c236cb9ab95f38a5fd3b8 (patch) | |
tree | bc83357596371d3d3b284d5775492f806e38ffb3 /src/main/cpp/util | |
parent | 88d1caeef07533429468515f63b3d4e2cb9a7a80 (diff) |
Windows,Bazel client: check embedded tools faster
The Bazel client on Windows is now 50% faster to
check the embedded tools than it was before.
Results:
- Linux: 20 ms -> 6 ms
- Windows: 294 ms -> 133 ms
Measurements were done with n=10 runs and a hot
server, using blaze::GetMillisecondsMonotonic().
Previously the client performed the same tasks
multiple times while trying to determine if a path
was a good extracted binary. (E.g. converted the
path to Windows format multiple times, checked if
it was a directory twice, opened the path twice.)
Now the client performes these tasks only once,
e.g. it converts path once and stats only once.
See https://github.com/bazelbuild/bazel/issues/5444
Closes #5445.
PiperOrigin-RevId: 201913758
Diffstat (limited to 'src/main/cpp/util')
-rw-r--r-- | src/main/cpp/util/file_platform.h | 13 | ||||
-rw-r--r-- | src/main/cpp/util/file_posix.cc | 8 | ||||
-rw-r--r-- | src/main/cpp/util/file_windows.cc | 62 |
3 files changed, 47 insertions, 36 deletions
diff --git a/src/main/cpp/util/file_platform.h b/src/main/cpp/util/file_platform.h index 861ca9e997..d7538fd86c 100644 --- a/src/main/cpp/util/file_platform.h +++ b/src/main/cpp/util/file_platform.h @@ -31,10 +31,15 @@ class IFileMtime { public: virtual ~IFileMtime() {} - // Queries the mtime of `path` to see whether it's in the distant future. - // Returns true if querying succeeded and stores the result in `result`. - // Returns false if querying failed. - virtual bool GetIfInDistantFuture(const std::string &path, bool *result) = 0; + // Checks if `path` is a file/directory in the embedded tools directory that + // was not tampered with. + // Returns true if `path` is a directory or directory symlink, or if `path` is + // a file with an mtime in the distant future. + // Returns false otherwise, or if querying the information failed. + // TODO(laszlocsomor): move this function, and with it the whole IFileMtime + // class into blaze_util_<platform>.cc, because it is Bazel-specific logic, + // not generic file-handling logic. + virtual bool IsUntampered(const std::string &path) = 0; // Sets the mtime of file under `path` to the current time. // Returns true if the mtime was changed successfully. diff --git a/src/main/cpp/util/file_posix.cc b/src/main/cpp/util/file_posix.cc index df05b54a45..f9e96451dc 100644 --- a/src/main/cpp/util/file_posix.cc +++ b/src/main/cpp/util/file_posix.cc @@ -329,7 +329,7 @@ class PosixFileMtime : public IFileMtime { : near_future_(GetFuture(9)), distant_future_({GetFuture(10), GetFuture(10)}) {} - bool GetIfInDistantFuture(const string &path, bool *result) override; + bool IsUntampered(const string &path) override; bool SetToNow(const string &path) override; bool SetToDistantFuture(const string &path) override; @@ -344,18 +344,18 @@ class PosixFileMtime : public IFileMtime { static time_t GetFuture(unsigned int years); }; -bool PosixFileMtime::GetIfInDistantFuture(const string &path, bool *result) { +bool PosixFileMtime::IsUntampered(const string &path) { struct stat buf; if (stat(path.c_str(), &buf)) { return false; } + // Compare the mtime with `near_future_`, not with `GetNow()` or // `distant_future_`. // This way we don't need to call GetNow() every time we want to compare and // we also don't need to worry about potentially unreliable time equality // check (in case it uses floats or something crazy). - *result = (buf.st_mtime > near_future_); - return true; + return S_ISDIR(buf.st_mode) || (buf.st_mtime > near_future_); } bool PosixFileMtime::SetToNow(const string &path) { diff --git a/src/main/cpp/util/file_windows.cc b/src/main/cpp/util/file_windows.cc index 8a18403c91..d81ada1b14 100644 --- a/src/main/cpp/util/file_windows.cc +++ b/src/main/cpp/util/file_windows.cc @@ -109,7 +109,7 @@ class WindowsFileMtime : public IFileMtime { WindowsFileMtime() : near_future_(GetFuture(9)), distant_future_(GetFuture(10)) {} - bool GetIfInDistantFuture(const string& path, bool* result) override; + bool IsUntampered(const string& path) override; bool SetToNow(const string& path) override; bool SetToDistantFuture(const string& path) override; @@ -124,53 +124,59 @@ class WindowsFileMtime : public IFileMtime { static bool Set(const string& path, FILETIME time); }; -bool WindowsFileMtime::GetIfInDistantFuture(const string& path, bool* result) { - if (path.empty()) { +bool WindowsFileMtime::IsUntampered(const string& path) { + if (path.empty() || IsDevNull(path.c_str())) { return false; } - if (IsDevNull(path.c_str())) { - *result = false; - return true; - } + wstring wpath; string error; if (!AsAbsoluteWindowsPath(path, &wpath, &error)) { BAZEL_DIE(blaze_exit_code::LOCAL_ENVIRONMENTAL_ERROR) - << "WindowsFileMtime::GetIfInDistantFuture(" << path + << "WindowsFileMtime::IsUntampered(" << path << "): AsAbsoluteWindowsPath failed: " << error; } - AutoHandle handle(::CreateFileW( + // Get attributes, to check if the file exists. (It may still be a dangling + // junction.) + DWORD attrs = GetFileAttributesW(wpath.c_str()); + if (attrs == INVALID_FILE_ATTRIBUTES) { + return false; + } + + bool is_directory = attrs & FILE_ATTRIBUTE_DIRECTORY; + AutoHandle handle(CreateFileW( /* lpFileName */ wpath.c_str(), /* dwDesiredAccess */ GENERIC_READ, /* dwShareMode */ FILE_SHARE_READ, /* lpSecurityAttributes */ NULL, /* dwCreationDisposition */ OPEN_EXISTING, /* dwFlagsAndAttributes */ - IsDirectoryW(wpath) - ? (FILE_FLAG_OPEN_REPARSE_POINT | FILE_FLAG_BACKUP_SEMANTICS) - : FILE_ATTRIBUTE_NORMAL, + // Per CreateFile's documentation on MSDN, opening directories requires + // the FILE_FLAG_BACKUP_SEMANTICS flag. + is_directory ? FILE_FLAG_BACKUP_SEMANTICS : FILE_ATTRIBUTE_NORMAL, /* hTemplateFile */ NULL)); + if (!handle.IsValid()) { return false; } - FILETIME mtime; - if (!::GetFileTime( - /* hFile */ handle, - /* lpCreationTime */ NULL, - /* lpLastAccessTime */ NULL, - /* lpLastWriteTime */ &mtime)) { - return false; - } - // Compare the mtime with `near_future_`, not with `GetNow()` or - // `distant_future_`. - // This way we don't need to call GetNow() every time we want to compare (and - // thus convert a SYSTEMTIME to FILETIME), and we also don't need to worry - // about potentially unreliable FILETIME equality check (in case it uses - // floats or something crazy). - *result = CompareFileTime(&near_future_, &mtime) == -1; - return true; + if (is_directory) { + return true; + } else { + BY_HANDLE_FILE_INFORMATION info; + if (!GetFileInformationByHandle(handle, &info)) { + return false; + } + + // Compare the mtime with `near_future_`, not with `GetNow()` or + // `distant_future_`. + // This way we don't need to call GetNow() every time we want to compare + // (and thus convert a SYSTEMTIME to FILETIME), and we also don't need to + // worry about potentially unreliable FILETIME equality check (in case it + // uses floats or something crazy). + return CompareFileTime(&near_future_, &info.ftLastWriteTime) == -1; + } } bool WindowsFileMtime::SetToNow(const string& path) { |