Do not tickle TimestampGranularityMonitor for stable-status.txt no-op updates.

When rewriting stable-status.txt, which happens on each build, avoid updating
the file's ctime and mtime if the new contents match what is already in the
file.

This prevents tickling the TimestampGranularityMonitor for what should be a
no-op update, which in turn could cause null/incremental builds to stall for
up to a second.  The problem was magnified on macOS where the default HFS+
file system only has second-level granularity.  (This also affects Linux, but
because current Linux file systems have milli/nanosecond-level granularity,
the wait imposed by TimestampGranularityMonitor is minimal and thus not
generally noticeable.)

--
MOS_MIGRATED_REVID=137983794

3 files changed, 84 insertions, 1 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index 569ce9f839..cc8c96d826 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -160,6 +160,14 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
     }
 
     @Override
+    public void prepare(Path execRoot) throws IOException {
+      // The default implementation of this method deletes all output files; override it to keep
+      // the old stableStatus around. This way we can reuse the existing file (preserving its mtime)
+      // if the contents haven't changed.
+      deleteOutput(volatileStatus);
+    }
+
+    @Override
     public void execute(ActionExecutionContext actionExecutionContext)
         throws ActionExecutionException {
       try {
@@ -181,7 +189,14 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
         stableMap.put(BuildInfo.BUILD_USER, username);
         volatileMap.put(BuildInfo.BUILD_TIMESTAMP, Long.toString(System.currentTimeMillis()));
 
-        FileSystemUtils.writeContent(stableStatus.getPath(), printStatusMap(stableMap));
+        // Only update the stableStatus contents if they are different than what we have on disk.
+        // This is to preserve the old file's mtime so that we do not generate an unnecessary dirty
+        // file on each incremental build.
+        FileSystemUtils.maybeUpdateContent(stableStatus.getPath(), printStatusMap(stableMap));
+
+        // Contrary to the stableStatus, write the contents of volatileStatus unconditionally
+        // because we know it will be different. This output file is marked as "constant metadata"
+        // so its dirtiness will be ignored anyway.
         FileSystemUtils.writeContent(volatileStatus.getPath(), printStatusMap(volatileMap));
       } catch (IOException e) {
         throw new ActionExecutionException(
diff --git a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
index 73a9a5119f..fd5b711e2e 100644
--- a/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/vfs/FileSystemUtils.java
@@ -841,6 +841,38 @@ public class FileSystemUtils {
   }
 
   /**
+   * Updates the contents of the output file if they do not match the given array, thus maintaining
+   * the mtime and ctime in case of no updates. Follows symbolic links.
+   *
+   * <p>If the output file already exists but is unreadable, this tries to overwrite it with the new
+   * contents. In other words: unreadable or missing files are considered to be non-matching.
+   *
+   * @throws IOException if there was an error
+   */
+  public static void maybeUpdateContent(Path outputFile, byte[] newContent) throws IOException {
+    byte[] currentContent;
+    try {
+      currentContent = readContent(outputFile);
+    } catch (IOException e) {
+      // Ignore error per the rationale given in the docstring. Keep in mind that what we are doing
+      // here is for performance reasons only so we should only break if the real action (that is,
+      // the write) fails -- not any of the optimization steps.
+      currentContent = null;
+    }
+
+    if (currentContent == null) {
+      writeContent(outputFile, newContent);
+    } else {
+      if (!Arrays.equals(newContent, currentContent)) {
+        if (!outputFile.isWritable()) {
+          outputFile.delete();
+        }
+        writeContent(outputFile, newContent);
+      }
+    }
+  }
+
+  /**
    * Returns the entirety of the specified input stream and returns it as a char
    * array, decoding characters using ISO-8859-1 (Latin1).
    *
diff --git a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
index f05b1d5edd..f9f4850320 100644
--- a/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/vfs/FileSystemUtilsTest.java
@@ -697,6 +697,42 @@ public class FileSystemUtilsTest {
   }
 
   @Test
+  public void testUpdateContent() throws Exception {
+    Path file = fileSystem.getPath("/test.txt");
+
+    clock.advanceMillis(1000);
+
+    byte[] content = new byte[] { 'a', 'b', 'c', 23, 42 };
+    FileSystemUtils.maybeUpdateContent(file, content);
+    byte[] actual = FileSystemUtils.readContent(file);
+    assertArrayEquals(content, actual);
+    FileStatus stat = file.stat();
+    assertEquals(1000, stat.getLastChangeTime());
+    assertEquals(1000, stat.getLastModifiedTime());
+
+    clock.advanceMillis(1000);
+
+    // Update with same contents; should not write anything.
+    FileSystemUtils.maybeUpdateContent(file, content);
+    assertArrayEquals(content, actual);
+    stat = file.stat();
+    assertEquals(1000, stat.getLastChangeTime());
+    assertEquals(1000, stat.getLastModifiedTime());
+
+    clock.advanceMillis(1000);
+
+    // Update with different contents; file should be rewritten.
+    content[0] = 'b';
+    file.chmod(0400);  // Protect the file to ensure we can rewrite it.
+    FileSystemUtils.maybeUpdateContent(file, content);
+    actual = FileSystemUtils.readContent(file);
+    assertArrayEquals(content, actual);
+    stat = file.stat();
+    assertEquals(3000, stat.getLastChangeTime());
+    assertEquals(3000, stat.getLastModifiedTime());
+  }
+
+  @Test
   public void testGetFileSystem() throws Exception {
     Path mountTable = fileSystem.getPath("/proc/mounts");
     FileSystemUtils.writeIsoLatin1(mountTable,