Change the timestamp of llvmcache-foo file to meet the thinLTO prune policy

[LuoYuanke]

Summary

The case will randomly fail if we test it with command "
while llvm-lit test/tools/gold/X86/cache.ll ; do true; done". It is because the llvmcache-foo file is younger than llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6. But due to timestamp precision reason their timestamp is the same. Given the same timestamp, the file prune policy is to remove bigger size file first, so mostly foo file is removed for its bigger size. And the files size is under threshold after deleting foo file. That's what test case expect.

However sometimes, the precision is enough to measure that timestamp of llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6 are smaller than foo, so llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6 are deleted first. Since the files size is still above the file size threshold after deleting the 2 files, the foo file is also deleted. And then the test case fails, because it expect only one file should be deleted instead of 3.

The fix is to change the timestamp of llvmcache-foo file to meet the thinLTO prune policy.

[LuoYuanke]

The case will randomly fail if we test it with command "
while llvm-lit test/tools/gold/X86/cache.ll ; do true; done". It is because the llvmcache-foo file is younger than llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6. But due to timestamp precision reason their timestamp is the same. Given the same timestamp, the file prune policy is to remove bigger size file first, so mostly foo file is removed for its bigger size. And the files size is under threshold after deleting foo file. That's what test case expect.

However sometimes, the precision is enough to measure that timestamp of llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6 are smaller than foo, so llvmcache-349F039B8EB076D412007D82778442BED3148C4E and llvmcache-A8107945C65C2B2BBEE8E61AA604C311D60D58D6 are deleted first. Since the files size is still above the file size threshold after deleting the 2 files, the foo file is also deleted. And then the test case fails, because it expect only one file should be deleted instead of 3.

The fix is to change the timestamp of llvmcache-foo file to meet the thinLTO prune policy

[tejohnson]

Can you upload the patch with context? See the following for instructions:
https://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface

[LuoYuanke]

Add context for the patch.

[tejohnson]

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

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.

[andrew.w.kaylor]

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

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.

--

[LuoYuanke]

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

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;
}

};

64 ↗

(On Diff #168612)

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.

[tejohnson]

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

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.

[LuoYuanke]

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

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?

[tejohnson]

LGTM

test/tools/gold/X86/cache.ll

64 ↗

(On Diff #168612)

Adding @ruiu regarding whether a separate lld patch is needed or if you can just commit the same test fix to lld - I'm guessing that is ok since it is essentially the same test.

In any case, go ahead and commit this one.

[ruiu]

If you are using git monorepo, you can easily combine two or more patches into one on this review site, and when you submit by "git llvm push", the command will automatically make a commit to each backing SVN repository. If you prefer, you can make a separate patch for review though. It's up to you. As long as you get an LGTM, you can commit it to lld.

llvm/trunk/test/tools/gold/X86/cache.ll

 ; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold%shlibext \
 ; RUN:     --plugin-opt=thinlto \
 ; RUN:     --plugin-opt=cache-dir=%t.cache \
 ; RUN:     --plugin-opt=cache-policy=cache_size_bytes=128k:prune_interval=0s \
 ; RUN:     -o %t3.o %t2.o %t.o
 ; RUN: ls %t.cache | count 5
 
 
+; Increase the age of llvmcache-foo
+; RUN: touch -r %t.cache/llvmcache-foo -d '-2 minutes' %t.cache/llvmcache-foo
+
 ; This should remove it.
 ; RUN: %gold -m elf_x86_64 -plugin %llvmshlibdir/LLVMgold%shlibext \
 ; RUN:     --plugin-opt=thinlto \
 ; RUN:     --plugin-opt=save-temps \
 ; 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
 ; RUN: ls %t.cache | count 4