Support: Extract fs::resize_file_before_mapping_readwrite from FileOutputBuffer

[dexonsmith]

Summary

Add a variant of fs::resize_file for use immediately before opening a
file with mapped_file_region::readwrite. On Windows, _chsize
(ftruncate) is slow, but CreateFileMapping (mmap) automatically
extends the file so the call to fs::resize_file can be skipped.

This optimization was added to FileOutputBuffer in
da9bc2e56d5a5c6332a9def1a0065eb399182b93; this commit just extracts the
logic out and adds a unit test.

[dexonsmith]

Note that I have no way of testing this myself on Windows; I'll be checking https://buildkite.com/llvm-project/premerge-checks/builds/23492.

[dexonsmith]

Fix a copy/paste bug in the test (only observable on Windows).

[dexonsmith]

In D95490#2524082, @dexonsmith wrote:

Note that I have no way of testing this myself on Windows; I'll be checking https://buildkite.com/llvm-project/premerge-checks/builds/23492.

This failed with:

C:\ws\w1\llvm-project\premerge-checks\llvm\unittests\Support\Path.cpp(1307): error:       Expected: Status.getSize()

      Which is: 4096

To be equal to: 123U

      Which is: 123

which is the bug I fixed in the update https://reviews.llvm.org/differential/diff/319433/:

In D95490#2524095, @dexonsmith wrote:

Fix a copy/paste bug in the test (only observable on Windows).

Anyone know how to trigger a rebuild now that the patch is fixed?

[rnk]

If I understand correctly, uploading a new patch triggered new build and test results, and now the reported size is 4096:
https://reviews.llvm.org/harbormaster/unit/view/310832/

So I think everything is working, it's just that mapping the file region on Windows seems to have a side effect of rounding the file size up to a multiple of the page size. So, the solution might be to change 123 to 4096 and hope for the best.

Code looks good to me, though.

[dexonsmith]

In D95490#2534499, @rnk wrote:

If I understand correctly, uploading a new patch triggered new build and test results, and now the reported size is 4096:
https://reviews.llvm.org/harbormaster/unit/view/310832/

Navigating from there leads me to https://reviews.llvm.org/B86778, which links to the old patch with the unit tests bug (https://reviews.llvm.org/D95490?id=319428). I don't see a build for the (slightly) new(er) patch with the fix (https://reviews.llvm.org/D95490?id=319433). Do you see one somewhere?

[dexonsmith]

Actually use resize_fiel_before_mapping_readwrite in FileOutputBuffer (just noticed I'd only copied the code, not extracted it).

(Maybe this will trigger a new harbourmaster build?)

[dexonsmith]

ping / rebase

[rnk]

lgtm, sorry for the delay

llvm/include/llvm/Support/FileSystem.h

 /// Resize path to size. File is resized as if by POSIX truncate().
 ///
 /// @param FD Input file descriptor.
 /// @param Size Size to resize to.
 /// @returns errc::success if \a path has been resized to \a size, otherwise a
 ///          platform-specific error_code.
 std::error_code resize_file(int FD, uint64_t Size);
 
+/// Resize \p FD to \p Size before mapping \a mapped_file_region::readwrite. On
+/// non-Windows, this calls \a resize_file(). On Windows, this is a no-op,
+/// since the subsequent mapping (via \c CreateFileMapping) automatically
+/// extends the file.
+inline std::error_code resize_file_before_mapping_readwrite(int FD,
+                                                            uint64_t Size) {
+#ifdef _WIN32
+  (void)FD;
+  (void)Size;
+  return std::error_code();
+#else
+  return resize_file(FD, Size);
+#endif
+}
+
 /// Compute an MD5 hash of a file's contents.
 ///
 /// @param FD Input file descriptor.
 /// @returns An MD5Result with the hash computed, if successful, otherwise a
 ///          std::error_code.
 ErrorOr<MD5::MD5Result> md5_contents(int FD);
 
 /// Version of compute_md5 that doesn't require an open file descriptor.

llvm/lib/Support/FileOutputBuffer.cpp

 static Expected<std::unique_ptr<FileOutputBuffer>>
 createOnDiskBuffer(StringRef Path, size_t Size, unsigned Mode) {
   Expected<fs::TempFile> FileOrErr =
       fs::TempFile::create(Path + ".tmp%%%%%%%", Mode);
   if (!FileOrErr)
     return FileOrErr.takeError();
   fs::TempFile File = std::move(*FileOrErr);
 
-#ifndef _WIN32
-  // On Windows, CreateFileMapping (the mmap function on Windows)
-  // automatically extends the underlying file. We don't need to
-  // extend the file beforehand. _chsize (ftruncate on Windows) is
-  // pretty slow just like it writes specified amount of bytes,
-  // so we should avoid calling that function.
-  if (auto EC = fs::resize_file(File.FD, Size)) {
+  if (auto EC = fs::resize_file_before_mapping_readwrite(File.FD, Size)) {
     consumeError(File.discard());
     return errorCodeToError(EC);
   }
-#endif
 
   // Mmap it.
   std::error_code EC;
   auto MappedFile = std::make_unique<fs::mapped_file_region>(
       fs::convertFDToNativeFile(File.FD), fs::mapped_file_region::readwrite,
       Size, 0, EC);
 
   // mmap(2) can fail if the underlying filesystem does not support it.

llvm/unittests/Support/Path.cpp

TEST_F(FileSystemTest, Resize) {
   ASSERT_NO_ERROR(fs::resize_file(FD, 123));
   fs::file_status Status;
   ASSERT_NO_ERROR(fs::status(FD, Status));
   ASSERT_EQ(Status.getSize(), 123U);
   ::close(FD);
   ASSERT_NO_ERROR(fs::remove(TempPath));
 }
 
+TEST_F(FileSystemTest, ResizeBeforeMapping) {
+  // Create a temp file.
+  int FD;
+  SmallString<64> TempPath;
+  ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));
+  ASSERT_NO_ERROR(fs::resize_file_before_mapping_readwrite(FD, 123));
+
+  // Map in temp file. On Windows, fs::resize_file_before_mapping_readwrite is
+  // a no-op and the mapping itself will resize the file.
+  std::error_code EC;
+  {
+    fs::mapped_file_region mfr(fs::convertFDToNativeFile(FD),
+                               fs::mapped_file_region::readwrite, 123, 0, EC);
+    ASSERT_NO_ERROR(EC);
+    // Unmap temp file
+  }
+
+  // Check the size.
+  fs::file_status Status;
+  ASSERT_NO_ERROR(fs::status(FD, Status));
+  ASSERT_EQ(Status.getSize(), 123U);
+  ::close(FD);
+  ASSERT_NO_ERROR(fs::remove(TempPath));
+}
+
 TEST_F(FileSystemTest, MD5) {
   int FD;
   SmallString<64> TempPath;
   ASSERT_NO_ERROR(fs::createTemporaryFile("prefix", "temp", FD, TempPath));
   StringRef Data("abcdefghijklmnopqrstuvwxyz");
   ASSERT_EQ(write(FD, Data.data(), Data.size()), static_cast<ssize_t>(Data.size()));
   lseek(FD, 0, SEEK_SET);
   auto Hash = fs::md5_contents(FD);
   ::close(FD);
   ASSERT_NO_ERROR(Hash.getError());
 
   EXPECT_STREQ("c3fcd3d76192e4007dfb496cca67e13b", Hash->digest().c_str());
 }
 
 TEST_F(FileSystemTest, FileMapping) {
   // Create a temp file.
   int FileDescriptor;
   SmallString<64> TempPath;
   ASSERT_NO_ERROR(
       fs::createTemporaryFile("prefix", "temp", FileDescriptor, TempPath));
   unsigned Size = 4096;
-  ASSERT_NO_ERROR(fs::resize_file(FileDescriptor, Size));
+  ASSERT_NO_ERROR(
+      fs::resize_file_before_mapping_readwrite(FileDescriptor, Size));
 
   // Map in temp file and add some content
   std::error_code EC;
   StringRef Val("hello there");
   {
     fs::mapped_file_region mfr(fs::convertFDToNativeFile(FileDescriptor),
                                fs::mapped_file_region::readwrite, Size, 0, EC);
     ASSERT_NO_ERROR(EC);