Skip to content
This repository has been archived by the owner on Apr 21, 2023. It is now read-only.

Commit

Permalink
cache-cleaning: allow people to turn it off
Browse files Browse the repository at this point in the history
Support 'FileCacheCleanIntervalMs -1' to turn off cache cleaning.

Fixes #1343
  • Loading branch information
jeffkaufman authored and crowell committed Jul 21, 2016
1 parent 59a1982 commit ccea83f
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 7 deletions.
10 changes: 9 additions & 1 deletion pagespeed/kernel/cache/file_cache.cc
Expand Up @@ -94,6 +94,7 @@ FileCache::FileCache(const GoogleString& path, FileSystem* file_system,
message_handler_(handler),
cache_policy_(policy),
mutex_(thread_system->NewMutex()),
next_clean_ms_(INT64_MAX),
path_length_limit_(file_system_->MaxPathLength(path)),
clean_time_path_(path),
clean_lock_path_(path),
Expand All @@ -102,7 +103,9 @@ FileCache::FileCache(const GoogleString& path, FileSystem* file_system,
evictions_(stats->GetVariable(kEvictions)),
bytes_freed_in_cleanup_(stats->GetVariable(kBytesFreedInCleanup)),
write_errors_(stats->GetVariable(kWriteErrors)) {
next_clean_ms_ = policy->timer->NowMs() + policy->clean_interval_ms / 2;
if (policy->cleaning_enabled()) {
next_clean_ms_ = policy->timer->NowMs() + policy->clean_interval_ms / 2;
}
EnsureEndsInSlash(&clean_time_path_);
StrAppend(&clean_time_path_, kCleanTimeName);
EnsureEndsInSlash(&clean_lock_path_);
Expand Down Expand Up @@ -182,6 +185,7 @@ const int64 kEmptyDirCleanAgeSec = 60;
} // namespace

bool FileCache::Clean(int64 target_size_bytes, int64 target_inode_count) {
DCHECK(cache_policy_->cleaning_enabled());
// TODO(jud): this function can delete .lock and .outputlock files, is this
// problematic?
message_handler_->Message(kInfo,
Expand Down Expand Up @@ -305,6 +309,10 @@ void FileCache::CleanWithLocking(int64 next_clean_time_ms) {
}

bool FileCache::ShouldClean(int64* suggested_next_clean_time_ms) {
if (!cache_policy_->cleaning_enabled()) {
return false;
}

bool to_return = false;
const int64 now_ms = cache_policy_->timer->NowMs();
{
Expand Down
5 changes: 5 additions & 0 deletions pagespeed/kernel/cache/file_cache.h
Expand Up @@ -50,6 +50,7 @@ class FileCache : public CacheInterface {
int64 clean_interval_ms;
int64 target_size_bytes;
int64 target_inode_count;
bool cleaning_enabled() { return clean_interval_ms != kDisableCleaning; }
private:
DISALLOW_COPY_AND_ASSIGN(CachePolicy);
};
Expand Down Expand Up @@ -88,6 +89,10 @@ class FileCache : public CacheInterface {
static const char kEvictions[];
static const char kWriteErrors[];

// What to set clean_interval_ms to in order to disable cleaning. This needs
// to be -1, because that's what we have in our public documentation.
static const int kDisableCleaning = -1;

private:
class CacheCleanFunction;
friend class FileCacheTest;
Expand Down
64 changes: 59 additions & 5 deletions pagespeed/kernel/cache/file_cache_test.cc
Expand Up @@ -53,11 +53,7 @@ class FileCacheTest : public CacheTestBase {
kTargetInodeLimit(10),
stats_(thread_system_.get()) {
FileCache::InitStats(&stats_);
cache_.reset(new FileCache(
GTestTempDir(), &file_system_, thread_system_.get(), &worker_,
new FileCache::CachePolicy(&mock_timer_, &hasher_, kCleanIntervalMs,
kTargetSize, kTargetInodeLimit),
&stats_, &message_handler_));
ResetFileCache(kCleanIntervalMs, kTargetSize);
disk_checks_ = stats_.GetVariable(FileCache::kDiskChecks);
cleanups_ = stats_.GetVariable(FileCache::kCleanups);
evictions_ = stats_.GetVariable(FileCache::kEvictions);
Expand All @@ -69,6 +65,14 @@ class FileCacheTest : public CacheTestBase {
file_system_.set_advance_time_on_update(true, &mock_timer_);
}

void ResetFileCache(int64 clean_interval_ms, int64 target_size_bytes) {
cache_.reset(new FileCache(
GTestTempDir(), &file_system_, thread_system_.get(), &worker_,
new FileCache::CachePolicy(&mock_timer_, &hasher_, clean_interval_ms,
target_size_bytes, kTargetInodeLimit),
&stats_, &message_handler_));
}

void CheckCleanTimestamp(int64 min_time_ms) {
GoogleString buffer;
file_system_.ReadFile(cache_->clean_time_path_.c_str(), &buffer,
Expand Down Expand Up @@ -278,4 +282,54 @@ TEST_F(FileCacheTest, CheckClean) {
CheckCleanTimestamp(time_ms);
}

// Test cleaning doesn't mean deleting everything in the cache.
TEST_F(FileCacheTest, CheckPartialClean) {
// Set the cache capacity to be big enough to hold two entries. We clean down
// to something like 75% full, though, so after cleaning only one entry will
// be left.
const int target_size = StrCat("Name1", "Value1",
"Name2", "Value2").length();
ResetFileCache(kCleanIntervalMs, target_size);

CheckPut("Name1", "Value1");
CheckPut("Name2", "Value2");
// Advance time to make sure we clean the old ones first.
mock_timer_.SleepMs(1);
CheckPut("Name3", "Value3");
mock_timer_.SleepMs(kCleanIntervalMs + 1);

// The cache is now oversize. Run clean. It deletes entries 1 and 2.
RunClean();

CheckNotFound("Name1"); // cleaned
CheckNotFound("Name2"); // cleaned
CheckGet("Name3", "Value3");
}

// Test that if we disable cleaning then cleaning doesn't happen. This is the
// same as CheckPartialClean, with cleaning disabled.
TEST_F(FileCacheTest, CheckPartialCleanWithCleaningDisabled) {
// Set the cache capacity to be big enough to hold two entries. We clean down
// to something like 75% full, though, so after cleaning only one entry will
// be left.
const int target_size = StrCat("Name1", "Value1",
"Name2", "Value2").length();
ResetFileCache(FileCache::kDisableCleaning, target_size);

CheckPut("Name1", "Value1");
CheckPut("Name2", "Value2");
// Advance time for consistency with the yes-clean version above.
mock_timer_.SleepMs(1);
CheckPut("Name3", "Value3");
mock_timer_.SleepMs(kCleanIntervalMs + 1);

// The cache is now oversize, but we won't clean because that's turned off.
RunClean();

// Everything is still there.
CheckGet("Name1", "Value1");
CheckGet("Name2", "Value2");
CheckGet("Name3", "Value3");
}

} // namespace net_instaweb
3 changes: 2 additions & 1 deletion pagespeed/system/system_rewrite_options.cc
Expand Up @@ -130,7 +130,8 @@ void SystemRewriteOptions::AddProperties() {
AddSystemProperty(
Timer::kHourMs, &SystemRewriteOptions::file_cache_clean_interval_ms_,
"afcci", RewriteOptions::kFileCacheCleanIntervalMs,
"Set the interval (in ms) for cleaning the file cache", true);
"Set the interval (in ms) for cleaning the file cache, -1 to disable "
"cleaning", true);
AddSystemProperty(100 * 1024 /* 100 megabytes */,
&SystemRewriteOptions::file_cache_clean_size_kb_,
"afc", RewriteOptions::kFileCacheCleanSizeKb,
Expand Down

0 comments on commit ccea83f

Please sign in to comment.