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

[Adrian McCarthy via Phabricator via llvm-commits]

amccarth added a comment.

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.



================
Comment at: llvm/unittests/Support/Path.cpp:1377
+  StringRef Content("hello there");
+  ASSERT_NO_ERROR(fs::resize_file_before_mapping_readwrite(FD, Content.size()));
+
----------------
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.


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

https://reviews.llvm.org/D95494