[ThinLTO][CachePruning] explicitly disable pruning

[bd1976llvm]

Summary

In https://reviews.llvm.org/rL321077 and https://reviews.llvm.org/D41231 I fixed a regression in the c-api which prevented the pruning from being *effectively* disabled.

However this approach, helpfully recommended by @labath, is cleaner.
It is also nice to remove the weasel words about effectively disabling from the api comments.

[labath]

This looks much cleaner indeed. lgtm, but please wait for signoff from the LTO folks as well.

include/llvm/Support/CachePruning.h

31 ↗

(On Diff #127897)

Maybe document the meaning of llvm::None ?

[bd1976llvm]

Addressed review comments

[tejohnson]

Thanks this looks much nicer. Question below about the handling of the Optional though.

As an aside, it would be great to have similar handling in parseCachePruningPolicy(), which is used by gold-plugin and lld, and which gives an error when -1 is passed as the interval. But that doesn't need to be in this patch, just noting it here.

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

Wouldn't this need to be *Policy.Interval, like in the below <= check?

[bd1976llvm]

In D41497#963176, @tejohnson wrote:

Thanks this looks much nicer. Question below about the handling of the Optional though.

As an aside, it would be great to have similar handling in parseCachePruningPolicy(), which is used by gold-plugin and lld, and which gives an error when -1 is passed as the interval. But that doesn't need to be in this patch, just noting it here.

If possible I would rather do that in another change.

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

The != operator is overloaded so it does the right thing.

[tejohnson]

In D41497#963189, @bd1976llvm wrote:

In D41497#963176, @tejohnson wrote:

Thanks this looks much nicer. Question below about the handling of the Optional though.

As an aside, it would be great to have similar handling in parseCachePruningPolicy(), which is used by gold-plugin and lld, and which gives an error when -1 is passed as the interval. But that doesn't need to be in this patch, just noting it here.

If possible I would rather do that in another change.

Agree

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

Ah ok, see that. It looks like <= has the same overloads - can the * be removed there?

[bd1976llvm]

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

Nope. On some platforms, e.g. windows, the type of TimeStampModTime is nanoseconds. Therefore, none of the Optional <= operator overloads will match.

[tejohnson]

LGTM assuming the answer to my question below is "yes". =)

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

Oh I see. I suppose there are <= overloads to make the comparison between the Interval in std::chrono::seconds and a TimeStampModTime in nanoseconds work as desired?

[bd1976llvm]

lib/Support/CachePruning.cpp

170 ↗

(On Diff #128006)

Indeed. What will happen is that a nanoseconds type will be constructed from the Policy.Interval which is of type seconds (this is where there is potential for overflow) and then the comparison will be between the two seconds types.

llvm/trunk/include/llvm-c/lto.h

  * These entry points control the ThinLTO cache. The cache is intended to
  * support incremental builds, and thus needs to be persistent across builds.
  * The client enables 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 limits the frequency at which the garbage collector
  *    will try to scan the cache directory to prune expired entries.
- *    Setting to a negative number applies the maximum interval.
+ *    Setting to a negative number disables the pruning.
  *  - 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 until
  *    the occupied space goes below a threshold.
  * @{
  */
 
 /**
  * Sets the path to a directory to use as a cache storage for incremental build.
  * Setting this activates caching.
  *
  * \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 sets the
- * maximum possible pruning interval. An unspecified default value will be
- * applied, and a value of 0 will be ignored.
+ * 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.
  *
  * \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

llvm/trunk/include/llvm/LTO/legacy/ThinLTOCodeGenerator.h

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. A
-  /// negative value sets the maximum possible pruning interval. A value
-  /// of 0 will be ignored.
+  /// 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.
   void setCachePruningInterval(int Interval) {
-    static_assert(std::is_same<decltype(CacheOptions.Policy.Interval),
-                               std::chrono::seconds>::value,
-                  "ensure same types to avoid risk of overflow");
-    if (Interval)
-      CacheOptions.Policy.Interval = Interval > 0 ? std::chrono::seconds(Interval)
-                                                  : std::chrono::seconds::max();
+    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.
   void setCacheEntryExpiration(unsigned Expiration) {
     if (Expiration)
       CacheOptions.Policy.Expiration = std::chrono::seconds(Expiration);
   }

llvm/trunk/include/llvm/Support/CachePruning.h

 
 template <typename T> class Expected;
 
 /// Policy for the pruneCache() function. A default constructed
 /// CachePruningPolicy provides a reasonable default policy.
 struct CachePruningPolicy {
   /// The pruning interval. This is intended to be used to avoid scanning the
   /// directory too often. It does not impact the decision of which file to
-  /// prune. A value of 0 forces the scan to occur.
-  std::chrono::seconds Interval = std::chrono::seconds(1200);
+  /// prune. A value of 0 forces the scan to occur. A value of None disables
+  /// pruning.
+  llvm::Optional<std::chrono::seconds> Interval = std::chrono::seconds(1200);
 
   /// The expiration for a file. When a file hasn't been accessed for Expiration
   /// seconds, it is removed from the cache. A value of 0 disables the
   /// expiration-based pruning.
   std::chrono::seconds Expiration = std::chrono::hours(7 * 24); // 1w
 
   /// The maximum size for the cache directory, in terms of percentage of the
   /// available space on the the disk. Set to 100 to indicate no limit, 50 to

llvm/trunk/lib/Support/CachePruning.cpp

if (Policy.Expiration == seconds(0) &&
     // Nothing will be pruned, early exit
     return false;
   }
 
   // Try to stat() the timestamp file.
   SmallString<128> TimestampFile(Path);
   sys::path::append(TimestampFile, "llvmcache.timestamp");
   sys::fs::file_status FileStatus;
-  const auto CurrentTime =
-      time_point_cast<decltype(Policy.Interval)>(system_clock::now());
+  const auto CurrentTime = system_clock::now();
   if (auto EC = sys::fs::status(TimestampFile, FileStatus)) {
     if (EC == errc::no_such_file_or_directory) {
       // If the timestamp file wasn't there, create one now.
       writeTimestampFile(TimestampFile);
     } else {
       // Unknown error?
       return false;
     }
   } else {
+    if (!Policy.Interval)
+      return false;
     if (Policy.Interval != seconds(0)) {
       // Check whether the time stamp is older than our pruning interval.
       // If not, do nothing.
-      const auto TimeStampModTime = time_point_cast<decltype(Policy.Interval)>(
-          FileStatus.getLastModificationTime());
+      const auto TimeStampModTime = FileStatus.getLastModificationTime();
       auto TimeStampAge = CurrentTime - TimeStampModTime;
-      if (TimeStampAge <= Policy.Interval) {
+      if (TimeStampAge <= *Policy.Interval) {
         DEBUG(dbgs() << "Timestamp file too recent ("
                      << duration_cast<seconds>(TimeStampAge).count()
                      << "s old), do not prune.\n");
         return false;
       }
     }
     // Write a new timestamp file so that nobody else attempts to prune.
     // There is a benign race condition here, if two processes happen to