[PATCH] D52452: Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy

[LuoYuanke via Phabricator via llvm-commits]

LuoYuanke added inline comments.


================
Comment at: test/tools/gold/X86/cache.ll:64
 ; RUN:     --plugin-opt=cache-dir=%t.cache \
 ; RUN:     --plugin-opt=cache-policy=cache_size_bytes=32k:prune_interval=0s \
 ; RUN:     -o %t4.o %t2.o %t.o
----------------
tejohnson wrote:
> LuoYuanke wrote:
> > LuoYuanke wrote:
> > > andrew.w.kaylor wrote:
> > > > tejohnson wrote:
> > > > > The default prune_after is 1 week, so I'm not sure why we are pruning any of the files here based on whether the timestamps of files just created are the same or different. The llvmcache-foo file as well as the two created by the link should all have very recent time stamps.
> > > > > 
> > > > > Did you confirm it is being removed due to age via the debug messages (i.e. --plugin-opt=-debug-only=cache-pruning)? What does it think the age is?
> > > > > 
> > > > > Also, the same test exists for lld: tools/lld/test/ELF/lto/cache.ll, so whatever change is needed here would presumably be needed there too. But I'd like to understand the answer to my question above first.
> > > > Here's the output I got when the test failed:
> > > > 
> > > > ```
> > > > Exit Code: 1
> > > > 
> > > > Command Output (stderr):
> > > > --
> > > > Occupancy: 0% target is: 75%, 32768 bytes
> > > >  - Remove /export/iusers/akaylor/llvm/test/tools/gold/X86/Output/cache.ll.tmp.cache/llvmcache-03F746885AFFCCA0CAE821DEF570C61729F7B86C (size 600), new occupancy is 66017%
> > > >  - Remove /export/iusers/akaylor/llvm/test/tools/gold/X86/Output/cache.ll.tmp.cache/llvmcache-1EF80E4A284B68475881854D2B20EE892E0A839C (size 480), new occupancy is 65537%
> > > >  - Remove /export/iusers/akaylor/llvm/test/tools/gold/X86/Output/cache.ll.tmp.cache/llvmcache-foo (size 65537), new occupancy is 0%
> > > > Expected 4 lines, got 2.
> > > > 
> > > > --
> > > > ```
> > > It is because the file size of llvmcache-foo is 64k, and there is option “--plugin-opt=cache-policy=cache_size_bytes=32k” in line 61 of the cache.ll. 64k is above the threshold of 32k. So plugin of ld.gold is to prune some files to free disk space. And the free algorithm is first based on timestamp and second based on file size. Below is the code. 
> > > Yes, I use --plugin-opt=-debug and also add some print to dump the timestamp information, so I confirm it is being removed due to the age. This issue is easy to expose if we add “sleep 1” before creating llvmcache-foo.
> > > 
> > > struct FileInfo {
> > >   sys::TimePoint<> Time;
> > >   uint64_t Size;
> > >   std::string Path;
> > > 
> > >   /// Used to determine which files to prune first. Also used to determine
> > >   /// set membership, so must take into account all fields.
> > >   bool operator<(const FileInfo &Other) const {
> > >     if (Time < Other.Time)
> > >       return true;
> > >     else if (Other.Time < Time)
> > >       return false;
> > >     if (Other.Size < Size)
> > >       return true;
> > >     else if (Size < Other.Size)
> > >       return false;
> > >     return Path < Other.Path;
> > >   }
> > > };
> > > 
> > Yes, that the same output on my machine. When it fails, ld.gold first remove llvmcache-03F746885AFFCCA0CAE821DEF570C61729F7B86C  and llvmcache-1EF80E4A284B68475881854D2B20EE892E0A839C , then it remove llvmcache-foo. It think the test case expected only llvmcache-foo should be removed.
> Oh got it - the files are put into a set ordered this way, so they are processed in timestamp order unless the timestamps are the same.
> 
> In that case this change seems fine to me, but can you please also change the corresponding lld test (tools/lld/test/ELF/lto/cache.ll)? Also, please add a comment here and there explaining that we need to make sure the large file to remove has the oldest time stamp so that it is processed and removed first.
I will add comments to explain that we need to make sure the large file to remove has the oldest time stamp so that it is processed and removed first.

 One question about lld. lld is an independent repo from llvm. Should I create another Differential to review lld test code?


https://reviews.llvm.org/D52452