Page MenuHomePhabricator

Update thin-lto cache file atimes when on windows
ClosedPublic

Authored by gbreynoo on May 23 2018, 8:50 AM.

Details

Summary

Thin-lto cache file atimes are used for expiration based pruning, and since Vista, access file times are not updated by Windows by default:
https://blogs.technet.microsoft.com/filecab/2006/11/07/disabling-last-access-time-in-windows-vista-to-improve-ntfs-performance/

This means on Windows, cache files are currently being pruned from creation time. This change manually updates cache files that are accessed by Thin-lto, when on Windows.

Diff Detail

Repository
rL LLVM

Event Timeline

gbreynoo created this revision.May 23 2018, 8:50 AM
dmajor added a subscriber: dmajor.Jun 1 2018, 9:02 AM
dmajor added a comment.Jun 5 2018, 2:19 PM

I don't know this code well enough to be a proper reviewer, but I want to offer moral support for this change. Not having atime updates on NTFS can be frustrating at times.

include/llvm/Support/FileSystem.h
854 ↗(On Diff #148217)

Needs a @param

lib/Support/Windows/Path.inc
1099 ↗(On Diff #148217)

This needs a CloseHandle here, right?

gbreynoo updated this revision to Diff 151193.Jun 13 2018, 10:29 AM
gbreynoo added a reviewer: zturner.

Updated to a recent revision and made changes suggested by dmajor

pcc added inline comments.Jun 13 2018, 10:41 AM
lib/LTO/Caching.cpp
37 ↗(On Diff #151193)

I'd rather not add another mysterious bool parameter to getFile for something that only one client needs. I'd prefer if the atime update were made more explicit. Specifically, I think we should do this:

  • add an OpenFlag that means "update atime"
  • change this code to use openFileForRead passing the new OpenFlag to obtain an FD, and then pass the FD to MemoryBuffer::getOpenFile().
gbreynoo updated this revision to Diff 151734.Jun 18 2018, 9:29 AM

Updated to recent revision and made changes suggested by pcc

pcc added a comment.Jun 25 2018, 11:23 AM

You should be able to avoid adding the bool argument to openNativeFileInternal as well.

include/llvm/Support/FileSystem.h
726 ↗(On Diff #151734)

Nit: add space between /// and the start of the comment.

lib/LTO/Caching.cpp
36 ↗(On Diff #151734)

I think this is leaking the FD; you need to close it after creating the map. Same in ThinLTOCodeGenerator.cpp.

lib/Support/Windows/Path.inc
1083 ↗(On Diff #151734)

Move this into nativeAccess.

1133 ↗(On Diff #151734)

Move the call to updateAccessTime here (or better yet just inline the code).

gbreynoo updated this revision to Diff 153454.Jun 29 2018, 2:47 AM

Updated to recent revision and made changes suggested by pcc

pcc accepted this revision.Jun 29 2018, 10:11 AM

LGTM

lib/Support/Windows/Path.inc
1117 ↗(On Diff #153454)

Close the file handle here.

This revision is now accepted and ready to land.Jun 29 2018, 10:11 AM
gbreynoo updated this revision to Diff 153720.Jul 2 2018, 8:10 AM

Updated to recent revision and made change suggested by pcc

This revision was automatically updated to reflect the committed changes.