[ThinLTO] Add a couple of more knobs to C API to control cache size.

[kromanova]

Summary

Exposing a couple of C LTO API functions to control the size and amount of files in cache directory. These functions have been supported in C++ LTO API for a long time.

[kromanova]

Hello,
Will it possible to review this patch also? Thank you so much in advance.
Does someone knows who in Apple is responsible for ThinLTO? It might be helpful to add this person as a reviewer.
Katya.

[tejohnson]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

169

extraneous new line

189

In CachePruning.h, a value of 0 for this field disables size based pruning. Why do something different here (which seems to go in the opposite direction from D42267)?

tools/llvm-lto/llvm-lto.cpp

163

line too long

166

line too long

166

default here is off by a factor of 10 from the default in CachePruning.h.

[kromanova]

Addressed code review comments.

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

189

In D42267 a value 0 for PruningInterval actually forces the scan to occur, so its meaning is somewhat different.

189

When writing the patch, I was hesitating for a whether to ignore value 0 here (for a time being) or use it for initializing CacheOptions.Policy.* (as a part of this patch).

I decided to ignore a value 0 for now and that's why :

the value 0 for two previously existing pruning control functions is currently ignored (while I believe it should be changed to have the same effect as in C++ AP!)

• setCacheEntryExpiration
• setMaxCacheSizeRelativeToAvailableSpace

I added these two knobs in this patch:

• setCacheMaxSizeBytes
• setCacheMaxSizeFiles

and decided to keep for now consistent with setCacheEntryExpiration and setMaxCacheSizeRelativeToAvailableSpace, since all of these 4 functions are related.

I could have changed all of these functions and accept value 0 as a part of this patch, but a lot of tests needs to be written, so I thought it's better to do it as a separate commit.

tools/llvm-lto/llvm-lto.cpp

166

Done.

However, I noticed that most of the functions from line 68 6o 215 in llvm-lto.cpp are formatted very inconsistently. I'd like to make the formatting uniformed. It could be done as a part of this or (preferably) different commit. If you think this "tidying up" needs to be done, which function should I use as a good example?
I personally prefer the formatting similar to the one used below, but I'm open to any other. as long as it's consistent. Or I could just run this part of the file through clang-format. Let me know.

static cl::list<std::string> ExportedSymbols(
    "exported-symbol",
    cl::desc("List of symbols to export from the resulting object file"),
    cl::ZeroOrMore);

166

Ah, good catch! Thanks. Can't count my zeros.

[kromanova]

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

189

Also, before making a subsequent commit to accept (instead of ignoring) 0 values for setCache* functions, I'd like to get ld64 developers option that they are OK with this. Who is responsible for ld64 or LTO project in Apple?

[kromanova]

Please let me know if more changes needed to this patch.

Based on the discussion in this code review, I'm planning to make two simple follow-up patches:
(1) formatting in llvm-lto.cpp (let me know an example of preferred formatting that I could use as a template or I could use plain clang-format).
(2) accepting 0 values for setCacheEntryExpiration, setMaxCacheSizeRelativeToAvailableSpace, setCacheMaxSizeBytes, setCacheMaxSizeFiles.

[kromanova]

Adding Apple's developers to the list of subscribers, since this patch is related to C LTO API .

[kromanova]

Teresa said that Peter Collingbourne might be the code owner for C LTO API, so I'm including him as a reviewer for this code review as well. This review was sitting for a while. Peter, could you please have a look and approve if everything looks OK to you.

[pcc]

I have no strong feelings about what happens in the legacy API, so probably someone at Apple should take a look first.

[kromanova]

Peter suggested that someone from Apple should have a look. I added Steven Wu as a reviewer.
Steven, could you (or someone else responsible for ld64 or ThinLTO) review and approve?

[steven_wu]

The API change is fine for ld64. But you need to do the following when you update LTO C API:

• You need to declare the interface in include/llvm-c/lto.h
• You need to bump LTO_API_VERSION in that header and document the new API is available since the new LTO_API_VERSION.
• You need to update tools/lto/lto.exports. This is the export list used by ld64 and only the interface listed in that file will be exported on darwin platform.

[kromanova]

Updated this patch to address Steven Wu's comments:

• Declared new interfaces in include/llvm-c/lto.h;
• Bumped LTO_API_VERSION;
• Documented the new APIs available since the new LTO_API_VERSION;
• Updated tools/lto/lto.exports (hopefully the order of the functions is not important, because I re-group them slightly).

[steven_wu]

LGTM. And yes, order doesn't matter.

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

public:
 
   /// Cache policy: expiration (in seconds) for an entry.
   /// A value of 0 will be ignored.
   void setCacheEntryExpiration(unsigned Expiration) {
     if (Expiration)
       CacheOptions.Policy.Expiration = std::chrono::seconds(Expiration);
   }
 
   /**
    * Sets the maximum cache size that can be persistent across build, in terms
    * of percentage of the available space on the disk. Set to 100 to indicate
    * no limit, 50 to indicate that the cache size will not be left over
    * half the available space. A value over 100 will be reduced to 100, and a
    * value of 0 will be ignored.
    *
    *
    * The formula looks like:
    *  AvailableSpace = FreeSpace + ExistingCacheSize
    *  NewCacheSize = AvailableSpace * P/100
    *
    */
   void setMaxCacheSizeRelativeToAvailableSpace(unsigned Percentage) {
     if (Percentage)
       CacheOptions.Policy.MaxSizePercentageOfAvailableSpace = Percentage;
   }
 
+  /// Cache policy: the maximum size for the cache directory in bytes. A value
+  /// over the amount of available space on the disk will be reduced to the
+  /// amount of available space. A value of 0 will be ignored.
+  void setCacheMaxSizeBytes(unsigned MaxSizeBytes) {
+    if (MaxSizeBytes)
+      CacheOptions.Policy.MaxSizeBytes = MaxSizeBytes;
+  }
+
+  /// Cache policy: the maximum number of files in the cache directory. A value
+  /// of 0 will be ignored.
+  void setCacheMaxSizeFiles(unsigned MaxSizeFiles) {
+    if (MaxSizeFiles)
+      CacheOptions.Policy.MaxSizeFiles = MaxSizeFiles;
+  }
+
   /**@}*/
 
   /// Set the path to a directory where to save temporaries at various stages of
   /// the processing.
   void setSaveTempsDir(std::string Path) { SaveTempsDir = std::move(Path); }
 
   /// Set the path to a directory where to save generated object files. This
   /// path can be used by a linker to request on-disk files instead of in-memory

test/ThinLTO/X86/cache.ll

 ; Verify that specifying a negative number for the pruning interval
 ; effectively disables the pruning
 ; RUN: rm -Rf %t.cache && mkdir %t.cache
 ; RUN: touch -t 197001011200 %t.cache/llvmcache-foo
 ; RUN: touch -t 197001011200 %t.cache/llvmcache.timestamp
 ; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-pruning-interval -1
 ; RUN: ls %t.cache/llvmcache-foo
 
+; Verify that specifying max size for the cache directory prunes it to this
+; size, removing the largest files first.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; Create cache files with different sizes.
+; Only 8B, 16B and 76B files should stay after pruning.
+; RUN: %python -c "print(' ' * 1023)" > %t.cache/llvmcache-foo-1024
+; RUN: %python -c "print(' ' * 15)" > %t.cache/llvmcache-foo-16
+; RUN: %python -c "print(' ' * 7)" > %t.cache/llvmcache-foo-8
+; RUN: %python -c "print(' ' * 75)" > %t.cache/llvmcache-foo-76
+; RUN: %python -c "print(' ' * 76)" > %t.cache/llvmcache-foo-77
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-max-size-bytes 100
+; RUN: ls %t.cache/llvmcache-foo-16
+; RUN: ls %t.cache/llvmcache-foo-8
+; RUN: ls %t.cache/llvmcache-foo-76
+; RUN: not ls %t.cache/llvmcache-foo-1024
+; RUN: not ls %t.cache/llvmcache-foo-77
+
+; Verify that specifying max number of files in the cache directory prunes
+; it to this amount, removing the largest files first.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; Create cache files with different sizes.
+; Only 8B and 16B files should stay after pruning.
+; RUN: %python -c "print(' ' * 1023)" > %t.cache/llvmcache-foo-1024
+; RUN: %python -c "print(' ' * 15)" > %t.cache/llvmcache-foo-16
+; RUN: %python -c "print(' ' * 7)" > %t.cache/llvmcache-foo-8
+; RUN: %python -c "print(' ' * 75)" > %t.cache/llvmcache-foo-76
+; RUN: %python -c "print(' ' * 76)" > %t.cache/llvmcache-foo-77
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache --thinlto-cache-max-size-files 2
+; RUN: ls %t.cache/llvmcache-foo-16
+; RUN: ls %t.cache/llvmcache-foo-8
+; RUN: not ls %t.cache/llvmcache-foo-76
+; RUN: not ls %t.cache/llvmcache-foo-1024
+; RUN: not ls %t.cache/llvmcache-foo-77
+
 target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-apple-macosx10.11.0"
 
 define void @globalfunc() #0 {
 entry:
   ret void
 }

tools/llvm-lto/llvm-lto.cpp

cl::desc("For the module ID for the file to process, useful to "
              "match what is in the index."));
 
 static cl::opt<std::string>
     ThinLTOCacheDir("thinlto-cache-dir", cl::desc("Enable ThinLTO caching."));
 
 static cl::opt<int>
     ThinLTOCachePruningInterval("thinlto-cache-pruning-interval", cl::desc("Set ThinLTO cache pruning interval."));
 
+static cl::opt<int>
+    ThinLTOCacheMaxSizeBytes("thinlto-cache-max-size-bytes",
+    cl::desc("Set ThinLTO cache pruning directory maximum size in bytes."));
+
+static cl::opt<int>
+    ThinLTOCacheMaxSizeFiles("thinlto-cache-max-size-files", cl::init(1000000),
+    cl::desc("Set ThinLTO cache pruning directory maximum number of files."));
+
 static cl::opt<std::string> ThinLTOSaveTempsPrefix(
     "thinlto-save-temps",
     cl::desc("Save ThinLTO temp files using filenames created by adding "
              "suffixes to the given file path prefix."));
 
 static cl::opt<std::string> ThinLTOGeneratedObjectsDir(
     "thinlto-save-objects",
     cl::desc("Save ThinLTO generated object files using filenames created in "

 public:
   ThinLTOCodeGenerator ThinGenerator;
 
   ThinLTOProcessing(const TargetOptions &Options) {
     ThinGenerator.setCodePICModel(getRelocModel());
     ThinGenerator.setTargetOptions(Options);
     ThinGenerator.setCacheDir(ThinLTOCacheDir);
     ThinGenerator.setCachePruningInterval(ThinLTOCachePruningInterval);
+    ThinGenerator.setCacheMaxSizeFiles(ThinLTOCacheMaxSizeFiles);
+    ThinGenerator.setCacheMaxSizeBytes(ThinLTOCacheMaxSizeBytes);
     ThinGenerator.setFreestanding(EnableFreestanding);
 
     // Add all the exported symbols to the table of symbols to preserve.
     for (unsigned i = 0; i < ExportedSymbols.size(); ++i)
       ThinGenerator.preserveSymbol(ExportedSymbols[i]);
   }
 
   void run() {

tools/lto/lto.cpp

void thinlto_codegen_set_cache_entry_expiration(thinlto_code_gen_t cg,
   return unwrap(cg)->setCacheEntryExpiration(expiration);
 }
 
 void thinlto_codegen_set_final_cache_size_relative_to_available_space(
     thinlto_code_gen_t cg, unsigned Percentage) {
   return unwrap(cg)->setMaxCacheSizeRelativeToAvailableSpace(Percentage);
 }
 
+void thinlto_codegen_set_cache_size_bytes(
+    thinlto_code_gen_t cg, unsigned MaxSizeBytes) {
+  return unwrap(cg)->setCacheMaxSizeBytes(MaxSizeBytes);
+}
+
+void thinlto_codegen_set_cache_size_files(
+    thinlto_code_gen_t cg, unsigned MaxSizeFiles) {
+  return unwrap(cg)->setCacheMaxSizeFiles(MaxSizeFiles);
+}
+
 void thinlto_codegen_set_savetemps_dir(thinlto_code_gen_t cg,
                                        const char *save_temps_dir) {
   return unwrap(cg)->setSaveTempsDir(save_temps_dir);
 }
 
 void thinlto_set_generated_objects_dir(thinlto_code_gen_t cg,
                                        const char *save_temps_dir) {
   unwrap(cg)->setGeneratedObjectsDirectory(save_temps_dir);