[PATCH] D95494: Support: Add mapped_file_region::sync(), equivalent to msync

[Duncan P. N. Exon Smith via Phabricator via llvm-commits]

dexonsmith added a comment.

In D95494#2561067 <https://reviews.llvm.org/D95494#2561067>, @amccarth wrote:

> In D95494#2534572 <https://reviews.llvm.org/D95494#2534572>, @rnk wrote:
>
>> @amccarth, can you confirm that these are the right APIs to use here? Do we need both FlushViewOfFile and FlushFileBuffers?
>
> Yes, those are the right APIs.  The first initiates the flush asynchronously.  The second blocks until the flush is complete.  That's necessary if it's important to match the behavior of the corresponding Posix implementation, which passes the MS_SYNC flag.
>
> The only difference I see is that that Posix version will mark the last write time on the file to be updated, but the Windows version _may_ not.  I doubt that's important, but, if it is, then the Windows version would also have to call SetFileTime.  I don't know how often LLVM code will sync, so I would avoid an extra operation if it's not necessary.
>
> On Windows, you rarely need to use FlushViewOfFile because (1) the system will keep multiple _views_ coherent across threads and processes and (2) everything will be flushed to disk when the view is closed.  FlushViewOfFile is useful when you need to ensure the bits on disk are updated because you expect another thread or process to access the file through something other than another view into the same file mapping.

Ah, interesting. The use case I have in mind is optionally keeping a just-written-to `mapped_file_region` alive as a file-backed memory buffer. Effectively, allowing a caller to use `f2` instead of `f1`:

  std::unique_ptr<MemoryBuffer> f1(int FD, SmallVectorImpl<char> &&Data) {
    raw_fd_ostream file(FD, ...);
    file << Data;
    return std::make_unique<SmallVectorMemoryBuffer>(std::move(Data));
  }
  std::unique_ptr<MemoryBuffer> f2(int FD, SmallVectorImpl<char> &&Data) {
    auto MFR = std::make_unique<mapped_file_region>(FD, ...);
    memcpy(MFR.data(), ...)
    SmallVector<char, 0>().swap(Data);
    return std::make_unique<MappedFileRegionMemoryBuffer>(std::move(MFR));
  }

I think we'd just want it to sync as much say destruction a file stream or destruction a mapped_file_region.

What do you recommend? (I'll also document clearly what the guarantees are we end up with...)



================
Comment at: llvm/unittests/Support/Path.cpp:1377
+  StringRef Content("hello there");
+  ASSERT_NO_ERROR(fs::resize_file_before_mapping_readwrite(FD, Content.size()));
+
----------------
amccarth wrote:
> I'm mildly concerned that there are assertions that will prevent the temporary file from being deleted if/when the test fails.  Accumulation of orphaned temp files can slow down tests, eat up disk quotas, etc.
> 
> There is a `fs::TempFile` class that will delete the file when it goes out of scope.  Or, perhaps most of these `ASSERT_`s could be `EXPECT_`s.
Ah, thanks; I was just copy/pasting from an adjacent test, but I can at least clean mine up.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D95494/new/

https://reviews.llvm.org/D95494