Windows,client: extract embedded binaries faster #5448

laszlocsomor · 2018-06-21T17:23:12Z

When extracting embedded binaries, the client now
caches which directories it has already created
and won't attempt creating them again.

This saves some time on Windows: from 16.3 sec on
average down to 13.2 sec. (n=10 runs, always
starting Bazel with a new --output_user_root and
shutting down afterwards.)

On Linux I see only a marginal speedup, not
significant enough to claim credit for it. :)

See #5444

laszlocsomor · 2018-06-21T17:23:32Z

Based on #5447 -- enough to review the last commit (bee1005).

cvcal · 2018-06-21T18:02:31Z

src/main/cpp/blaze.cc

-    if (!blaze_util::MakeDirectories(blaze_util::Dirname(path), 0777)) {
-      BAZEL_DIE(blaze_exit_code::INTERNAL_ERROR)
-          << "couldn't create '" << path << "': " << GetLastErrorString();
+    if (created_directories_.insert(path).second) {


It's not obvious why this is here, I would add a comment here explaining that you do this to prevent duplicate work for performance reasons. Otherwise, I could imagine someone accidentally removing this without testing the performance implications.

cvcal · 2018-06-21T18:07:07Z

src/test/cpp/util/file_test.cc

-  ASSERT_TRUE(mtime.get()->GetIfInDistantFuture(tempdir, &actual));
-  ASSERT_FALSE(actual);
-
+  // Assert that a directory is always a good embedded embedded binary. (We do


embedded embedded

cvcal · 2018-06-21T18:08:54Z

src/main/cpp/util/file_platform.h

+  // embedded binary that Bazel extracted from a previous run and it hasn't
+  // been tampered with (its mtime is unchanged), or it's a directory.
+  // Returns false otherwise, or if querying the information failed.
+  virtual bool IsValidEmbeddedBinary(const std::string &path) = 0;


Ok, I hear you on not wanting to move this to blaze_util_platform. Maybe you can make this name more general then, though? IsUntamperedFile() keeps it more neutral.

I like that idea! Done.

I also added a TODO with my own name to move the function.

cvcal · 2018-06-21T18:14:35Z

src/test/cpp/util/file_windows_test.cc

@@ -34,6 +34,9 @@
 #error("This test should only be run on Windows")
 #endif  // !defined(_WIN32) && !defined(__CYGWIN__)

+#define _T(x) #x


These two macros look quite odd to me. I also don't see why you want to use LINE as a way to distinguish between targets. You could just give a random number, no? Is there any point to use the line number from the file?

If you do need this, I don't think you need to split it into two definitions, the single one should be enough.

I also don't see why you want to use __LINE__ as a way to distinguish between targets.

Because it is guaranteed to be unique among all other tests. For a while there was no cleanup code that deleted temp files that tests created. Now there is, at least in some tests (the test framework won't do it automatically), but I still use this practice to avoid tests clobbering each other's files.

If you do need this, I don't think you need to split it into two definitions, the single one should be enough.

Indeed! Done.

When extracting embedded binaries, the client now caches which directories it has already created and won't attempt creating them again. This saves some time on Windows: from 16.3 sec on average down to 13.2 sec. (n=10 runs, always starting Bazel with a new --output_user_root and shutting down afterwards.) On Linux I see only a marginal speedup, not significant enough to claim credit for it. :) See bazelbuild#5444

laszlocsomor · 2018-06-22T08:13:11Z

I addressed all comments, rebased this PR on top of master, and removed the diffbase (PR #5445) to make it cleaner.

Sorry for the confusion and for the confusing-looking commit history. Now it should look cleaner.

Add a simple profiler that can measure function call counts and durations, and report statistics by printing to stdout. Motivation: I recently needed a profiler for PR bazelbuild#5445 and PR bazelbuild#5448, so I'm adding the polished code now. Usage: 1. depend on //src/main/cpp/util:profiler 2. use PrintingStopWatch, PrintingStats, and ScopeProfiler objects, as shown in profiler.h's class documentation See bazelbuild#5444 Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6

Add a simple profiler that can measure function call counts and durations, and report statistics by printing to stdout. Motivation: I recently needed a profiler for PR bazelbuild#5445 and PR bazelbuild#5448, so I'm adding the polished code now. Usage: 1. depend on //src/main/cpp/util:profiler 2. use StopWatch, Task, and ScopedTask objects as shown in profiler.h's class documentation See bazelbuild#5444 Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6

Add a simple profiler that can measure function call counts and durations, and report statistics by printing to stderr. Motivation: I recently needed a profiler for PR #5445 and PR #5448, so I'm adding the polished code now. Usage: 1. depend on //src/main/cpp/util:profiler 2. use StopWatch, Task, and ScopedTask objects as shown in profiler.h's class documentation See #5444 Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6 Closes #5461. Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6 PiperOrigin-RevId: 202314319

When extracting embedded binaries, the client now caches which directories it has already created and won't attempt creating them again. This saves some time on Windows: from 16.3 sec on average down to 13.2 sec. (n=10 runs, always starting Bazel with a new --output_user_root and shutting down afterwards.) On Linux I see only a marginal speedup, not significant enough to claim credit for it. :) See bazelbuild#5444 Closes bazelbuild#5448. PiperOrigin-RevId: 201933181

Add a simple profiler that can measure function call counts and durations, and report statistics by printing to stderr. Motivation: I recently needed a profiler for PR bazelbuild#5445 and PR bazelbuild#5448, so I'm adding the polished code now. Usage: 1. depend on //src/main/cpp/util:profiler 2. use StopWatch, Task, and ScopedTask objects as shown in profiler.h's class documentation See bazelbuild#5444 Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6 Closes bazelbuild#5461. Change-Id: I43f0afd124b486c694f451e8455a66ffca8137b6 PiperOrigin-RevId: 202314319

googlebot added the cla: yes label Jun 21, 2018

laszlocsomor requested a review from cvcal June 21, 2018 17:25

cvcal mentioned this pull request Jun 21, 2018

Windows,Bazel client: check embedded tools faster #5445

Closed

cvcal reviewed Jun 21, 2018

View reviewed changes

laszlocsomor force-pushed the client-speedup-2 branch from bee1005 to 1fe97d6 Compare June 22, 2018 08:12

cvcal approved these changes Jun 22, 2018

View reviewed changes

bazel-io closed this in f5043d6 Jun 25, 2018

laszlocsomor mentioned this pull request Jun 25, 2018

Bazel client: add a simple profiler #5461

Closed

laszlocsomor deleted the client-speedup-2 branch June 27, 2018 12:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows,client: extract embedded binaries faster #5448

Windows,client: extract embedded binaries faster #5448

laszlocsomor commented Jun 21, 2018

laszlocsomor commented Jun 21, 2018 •

edited

Loading

cvcal Jun 21, 2018

laszlocsomor Jun 22, 2018

cvcal Jun 21, 2018

laszlocsomor Jun 22, 2018

cvcal Jun 21, 2018

laszlocsomor Jun 22, 2018

laszlocsomor Jun 22, 2018

cvcal Jun 21, 2018

laszlocsomor Jun 22, 2018

laszlocsomor commented Jun 22, 2018

Windows,client: extract embedded binaries faster #5448

Windows,client: extract embedded binaries faster #5448

Conversation

laszlocsomor commented Jun 21, 2018

laszlocsomor commented Jun 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laszlocsomor commented Jun 22, 2018

laszlocsomor commented Jun 21, 2018 •

edited

Loading