Page MenuHomePhabricator

Support: Extract fs::resize_file_before_mapping_readwrite from FileOutputBuffer
ClosedPublic

Authored by dexonsmith on Jan 26 2021, 4:15 PM.

Details

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.

Diff Detail

Event Timeline

dexonsmith created this revision.Jan 26 2021, 4:15 PM
dexonsmith requested review of this revision.Jan 26 2021, 4:15 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 26 2021, 4:15 PM

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.

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

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/:

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 added a comment.Feb 1 2021, 11:38 AM

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.

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 set the repository for this revision to rG LLVM Github Monorepo.

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?)

ping / rebase

rnk accepted this revision.Mar 3 2021, 2:34 PM

lgtm, sorry for the delay

This revision is now accepted and ready to land.Mar 3 2021, 2:34 PM