[ThinLTO] Allow 0 to be a valid value for pruning interval for C LTO API.

[kromanova]

Summary

• Allow 0 to be a valid value pruning interval in C LTO API. Value 0 will cause garbage collector to run. This matches the behavior in C++ LTO API.
• Updated comments accordingly.
• Updated the default value of pruning interval to be 1200 second in llvm-lto; that corresponds to the default values for all other users of C/C++ LTO API. This change is optional and could be reverted if there are some objections.

If someone knows (Mehdi?) the name of the person who works on ThinLTO in Apple, it will be nice to add him as a reviewer, since this is a change in C LTO API.

[bd1976llvm]

tools/llvm-lto/llvm-lto.cpp

161

Under what circumstances does this change have an effect?

I remember that the same default is set in the CachePruningPolicy constructor.

[kromanova]

tools/llvm-lto/llvm-lto.cpp

161

Before my change in llvm-lto.cpp we effectively overwrote the default value of PruningInterval, initializing it with 0.

ThinLTOProcessing(const TargetOptions &Options) {
    ...
    ThinGenerator.setCacheDir(ThinLTOCacheDir);
    ThinGenerator.setCachePruningInterval(ThinLTOCachePruningInterval);  //set PruningInterval to 0
    ThinGenerator.setFreestanding(EnableFreestanding);
    ...
}

All other knobs (e.g. Expiration or MaxSizePercentageOfAvailableSpace) determining the pruning policy are not being set in llvm-lto.cpp,
so they keep the default values assigned by CachePruningPolicy constructor.
With my change, we keep the default value (1200) for PruningInterval.

Why this is needed? I guess for consistency. When I wrote my tests, I compared the default pruning policy behavior with the behavior when PruningInterval == 0.
I was puzzled to notice that the behavior was the same. Only then I noticed that we are intentionally setting the value 0 to PruningInterval in llvm-lto.cpp.
I decided to fix it. However, if this behavior is undesirable, I don't mind to change my test to explicitly pass option for setting PruningInterval value for both of the tests, rather than using a default value.

[kromanova]

Hello,
Will it possible to review this patch? Thank you so much in advance.
Katya.

[tejohnson]

It looks ok to me, but I am not sure how ld64 is using this - are they also setting to 0 by default expecting default behavior?

[kromanova]

In D42267#992799, @tejohnson wrote:

It looks ok to me, but I am not sure how ld64 is using this - are they also setting to 0 by default expecting default behavior?

Unfortunately, I don't know about ld64's usage of pruning policy options. Anyone in Apple knows/cares?

[steven_wu]

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.

[kromanova]

In D42267#999581, @steven_wu wrote:

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.

Well, the main reason for this change was:

(a) compatibility with C++ API. A lot of code in libLTO is common for C API and C++ API. Having the same semantics for the same options in two different APIs will make code simpler and prevents from someone who only cares about C+ API to unintentionally break the C API functionality. The latter is particularly important, since incremental linking is not thoroughly tested.

(b) giving a possibility to a user to run a pruner immediately, vs. "no-op" which could get achieved simply by not passing "pruning-interval" option to the linker, making option value 0 useless.

I understand your compatibility issue, but I don't think it's very serious.

• I doubt it's common for a user to pass a option to the linker, if the same effect could be achieved by not passing any option at all (why would user do extra work?).
• This option will cause the pruner to run, which not necessarily means that the cache files will get removed and incremental build won't happen. Cache files will get removed only if at the same time the files are stale, or their sizes are larger then certain thresholds, which are passed as parameters.
• The pruner runs just 20 minutes earlier than it would have run by default. So, only the builds that are done more frequent than 20 minutes might be affected.
• Only the users who pick up the linker and libLTO from different packages will get affected.

Let me know what you decide. If you want to keep things the old way, I could modify this patch for PS4 only.

So, to summarize, we could add a meaningful value for pruning_interval option, which currently can't get achieved otherwise, and additionally we will offer compatibility with C++API.
The only users that will be affected are the ones who because of whatever reason decided to explicitly specify a default option on the command line rather than skipping it, whose builds are happening more frequently than 20 minutes and who had mixed and matched different releases of ld64 and libLTO.

[steven_wu]

In D42267#999711, @kromanova wrote:

In D42267#999581, @steven_wu wrote:

This indeed affects how ld64 interact with libLTO. If user didn't specify a value, ld64 will always call thinlto_codegen_set_cache_pruning_interval with interval 0. This is currently a no-op so it will use whatever the default value in libLTO. With this change, it will be force pruning.

It is possible for us to make change to future ld64 to use different value but it is not going work very well for compatibility. If old ld64 loaded a new libLTO, it is going to lose incremental build.

Well, the main reason for this change was:

(a) compatibility with C++ API. A lot of code in libLTO is common for C API and C++ API. Having the same semantics for the same options in two different APIs will make code simpler and prevents from someone who only cares about C+ API to unintentionally break the C API functionality. The latter is particularly important, since incremental linking is not thoroughly tested.

(b) giving a possibility to a user to run a pruner immediately, vs. "no-op" which could get achieved simply by not passing "pruning-interval" option to the linker, making option value 0 useless.

I understand your compatibility issue, but I don't think it's very serious.

• I doubt it's common for a user to pass a option to the linker, if the same effect could be achieved by not passing any option at all (why would user do extra work?).
• This option will cause the pruner to run, which not necessarily means that the cache files will get removed and incremental build won't happen. Cache files will get removed only if at the same time the files are stale, or their sizes are larger then certain thresholds, which are passed as parameters.
• The pruner runs just 20 minutes earlier than it would have run by default. So, only the builds that are done more frequent than 20 minutes might be affected.
• Only the users who pick up the linker and libLTO from different packages will get affected.

Let me know what you decide. If you want to keep things the old way, I could modify this patch for PS4 only.

So, to summarize, we could add a meaningful value for pruning_interval option, which currently can't get achieved otherwise, and additionally we will offer compatibility with C++API.
The only users that will be affected are the ones who because of whatever reason decided to explicitly specify a default option on the command line rather than skipping it, whose builds are happening more frequently than 20 minutes and who had mixed and matched different releases of ld64 and libLTO.

I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

I don't really like that we have the default in multiple places depending on which tool you use. So it seems we have default pruning interval in CachePruningPolicy and llvm-lto (and maybe in some linker that uses the C/C++ interfaces as well). That is probably the reason why C API choose to ignore 0 because if you have a cl::opt initialize to zero, it means it will respect the default from CachePruningPolicy.

[kromanova]

I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

So, can I assume that I got an "OK" from the other main stakeholder who is using C LTO API? BTW, I CC-ed you to another small patch https://reviews.llvm.org/D42446, where I added a couple of missing APIs for controlling cache policy. You might want to expose it to ld64 users.

I don't really like that we have the default in multiple places depending on which tool you use. So it seems we have default pruning interval in CachePruningPolicy and llvm-lto (and maybe in some linker that uses the C/C++ interfaces as well). That is probably the reason why C API choose to ignore 0 because if you have a cl::opt initialize to zero, it means it will respect the default from CachePruningPolicy.

I agree. Do you have an idea how to fix this? Feel free to propose a patch.

[steven_wu]

In D42267#999761, @kromanova wrote:

I am definitely not against the change. It would be good to have the number means the same thing in C/C++ API. I think ld64 can be updated in advance before we release 7.0 so we can have a larger compatibility window so I don't think this should be a blocker.

So, can I assume that I got an "OK" from the other main stakeholder who is using C LTO API? BTW, I CC-ed you to another small patch https://reviews.llvm.org/D42446, where I added a couple of missing APIs for controlling cache policy. You might want to expose it to ld64 users.

Adding new APIs to C APIs is safe for us. Thanks for the heads up.

I don't really like that we have the default in multiple places depending on which tool you use. So it seems we have default pruning interval in CachePruningPolicy and llvm-lto (and maybe in some linker that uses the C/C++ interfaces as well). That is probably the reason why C API choose to ignore 0 because if you have a cl::opt initialize to zero, it means it will respect the default from CachePruningPolicy.

I agree. Do you have an idea how to fix this? Feel free to propose a patch.

Unless you want to reserve a number for that (-2?) which is quite confusing, I don't know how to improve this situation. This patch is LGTM now.

[kromanova]

Great! Teresa, please let me know if it's OK to commit or if you want me to change something in this patch.
I got an OK from Steven Wu, Apple's ThinLTO/ld64 developer.

[kromanova]

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

[pcc]

LGTM with a few spelling/grammar nits

include/llvm-c/lto.h

787

nit: remove "be".

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

134

nit: "Setting to 0 will force..."

153

nit: prunning -> pruning

[kromanova]

kromanova committed r325303: Allow 0 to be a valid value pruning interval in C LTO API.

include/llvm-c/lto.h

  * \since LTO_API_VERSION=18
  */
 extern void thinlto_codegen_set_cache_dir(thinlto_code_gen_t cg,
                                           const char *cache_dir);
 
 /**
  * Sets the cache pruning interval (in seconds). A negative value disables the
  * pruning. An unspecified default value will be applied, and a value of 0 will
- * be ignored.
+ * be force prunning to occur.
  *
  * \since LTO_API_VERSION=18
  */
 extern void thinlto_codegen_set_cache_pruning_interval(thinlto_code_gen_t cg,
                                                        int interval);
 
 /**
  * Sets the maximum cache size that can be persistent across build, in terms of

include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

public:
    * These entry points control the ThinLTO cache. The cache is intended to
    * support incremental build, and thus needs to be persistent accross build.
    * The client enabled the cache by supplying a path to an existing directory.
    * The code generator will use this to store objects files that may be reused
    * during a subsequent build.
    * To avoid filling the disk space, a few knobs are provided:
    *  - The pruning interval limit the frequency at which the garbage collector
    *    will try to scan the cache directory to prune it from expired entries.
-   *    Setting to -1 disable the pruning (default).
+   *    Setting to -1 disable the pruning (default). Setting to 0 force pruning
+   *    to occur.
    *  - The pruning expiration time indicates to the garbage collector how old
    *    an entry needs to be to be removed.
    *  - Finally, the garbage collector can be instructed to prune the cache till
    *    the occupied space goes below a threshold.
    * @{
    */
 
   struct CachingOptions {
     std::string Path;                    // Path to the cache, empty to disable.
     CachePruningPolicy Policy;
   };
 
   /// Provide a path to a directory where to store the cached files for
   /// incremental build.
   void setCacheDir(std::string Path) { CacheOptions.Path = std::move(Path); }
 
   /// Cache policy: interval (seconds) between two prunes of the cache. Set to a
-  /// negative value to disable pruning. A value of 0 will be ignored.
+  /// negative value to disable pruning. A value of 0 will force prunning to
+  /// occur.
   void setCachePruningInterval(int Interval) {
-    if (Interval == 0)
-      return;
     if(Interval < 0)
       CacheOptions.Policy.Interval.reset();
     else
       CacheOptions.Policy.Interval = std::chrono::seconds(Interval);
   }
 
   /// Cache policy: expiration (in seconds) for an entry.
   /// A value of 0 will be ignored.

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 the pruner doesn't run and a cache file is not deleted when: 
+; default values for pruning interval and cache expiration are used, 
+; llvmcache.timestamp is current, 
+; cache file is older than default cache expiration value.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: touch -t 197001011200 %t.cache/llvmcache-foo
+; RUN: touch %t.cache/llvmcache.timestamp
+; RUN: llvm-lto -thinlto-action=run -exported-symbol=globalfunc %t2.bc %t.bc -thinlto-cache-dir %t.cache
+; RUN: ls %t.cache/llvmcache-foo
+
+; Verify that the pruner runs and a cache file is deleted when:
+; pruning interval has value 0 (i.e. run garbage collector now)
+; default value for cache expiration is used,
+; llvmcache.timestamp is current,
+; cache file is older than default cache expiration value.
+; RUN: rm -Rf %t.cache && mkdir %t.cache
+; RUN: touch -t 197001011200 %t.cache/llvmcache-foo
+; RUN: touch %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 0
+; RUN: not ls %t.cache/llvmcache-foo
+
 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

static cl::opt<std::string> ThinLTOModuleId(
     "thinlto-module-id",
     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."));
+    ThinLTOCachePruningInterval("thinlto-cache-pruning-interval", 
+    cl::init(1200), cl::desc("Set ThinLTO cache pruning interval."));
 
 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",