From: Joel Rosdahl Date: Thu, 10 Sep 2020 17:17:13 +0000 (+0200) Subject: Introduce Counters::get/set/increment methods, replacing operator[] X-Git-Tag: v4.0~108 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=b552f99f4fad10ddc5ff447054ed1ad1b91a17da;p=thirdparty%2Fccache.git Introduce Counters::get/set/increment methods, replacing operator[] The primary motivation for this is that it’s now possible to detect and block underflow when incrementing a value in the unlikely but possible event that a, say, cache_size_kibibyte is decremented below zero. --- diff --git a/src/Counters.cpp b/src/Counters.cpp index a680fd773..01db7356c 100644 --- a/src/Counters.cpp +++ b/src/Counters.cpp @@ -26,25 +26,35 @@ Counters::Counters() : m_counters(static_cast(Statistic::END)) { } -// clang-format off -uint64_t& -Counters::operator[](Statistic index) -// clang-format on +uint64_t +Counters::get(Statistic statistic) const { - const size_t i = static_cast(index); + assert(static_cast(statistic) < static_cast(Statistic::END)); + const size_t i = static_cast(statistic); + return i < m_counters.size() ? m_counters[i] : 0; +} + +void +Counters::set(Statistic statistic, uint64_t value) +{ + const auto i = static_cast(statistic); if (i >= m_counters.size()) { m_counters.resize(i + 1); } - return m_counters.at(i); + m_counters[i] = value; } -// clang-format off -uint64_t -Counters::operator[](Statistic index) const -// clang-format on +void +Counters::increment(Statistic statistic, int64_t value) { - const size_t i = static_cast(index); - return i < m_counters.size() ? m_counters.at(i) : 0; + const auto i = static_cast(statistic); + if (i >= m_counters.size()) { + m_counters.resize(i + 1); + } + auto& counter = m_counters[i]; + counter = + std::max(static_cast(0), static_cast(counter + value)); + } } size_t diff --git a/src/Counters.hpp b/src/Counters.hpp index cdd6c1515..cc8177b9c 100644 --- a/src/Counters.hpp +++ b/src/Counters.hpp @@ -31,8 +31,9 @@ class Counters public: Counters(); - uint64_t& operator[](Statistic index); - uint64_t operator[](Statistic index) const; + uint64_t get(Statistic statistic) const; + void set(Statistic statistic, uint64_t value); + void increment(Statistic statistic, int64_t value = 1); size_t size() const; diff --git a/src/Result.cpp b/src/Result.cpp index ca3c06689..96174f3ed 100644 --- a/src/Result.cpp +++ b/src/Result.cpp @@ -411,8 +411,8 @@ Result::Writer::write_raw_file_entry(const std::string& path, const auto size_delta = (new_stat.size_on_disk() - old_stat.size_on_disk()) / 1024; const auto files_delta = (new_stat ? 1 : 0) - (old_stat ? 1 : 0); - m_ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta; - m_ctx.counter_updates[Statistic::files_in_cache] += files_delta; + m_ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta); + m_ctx.counter_updates.increment(Statistic::files_in_cache, files_delta); } } // namespace Result diff --git a/src/ccache.cpp b/src/ccache.cpp index 4ef4a1c7d..2f6b2d7ce 100644 --- a/src/ccache.cpp +++ b/src/ccache.cpp @@ -757,12 +757,12 @@ update_manifest_file(Context& ctx) const int64_t files_delta = !old_st && st ? 1 : 0; if (ctx.stats_file() == ctx.manifest_stats_file()) { - ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta; - ctx.counter_updates[Statistic::files_in_cache] += files_delta; + ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta); + ctx.counter_updates.increment(Statistic::files_in_cache, files_delta); } else { Counters counters; - counters[Statistic::cache_size_kibibyte] += size_delta; - counters[Statistic::files_in_cache] += files_delta; + counters.increment(Statistic::cache_size_kibibyte, size_delta); + counters.increment(Statistic::files_in_cache, files_delta); stats_flush_to_file(ctx.config, ctx.manifest_stats_file(), counters); } } @@ -977,8 +977,9 @@ to_cache(Context& ctx, } const auto size_delta = (new_dest_stat.size_on_disk() - orig_dest_stat.size_on_disk()) / 1024; - ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta; - ctx.counter_updates[Statistic::files_in_cache] += orig_dest_stat ? 0 : 1; + ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta); + ctx.counter_updates.increment(Statistic::files_in_cache, + orig_dest_stat ? 0 : 1); MTR_END("file", "file_put"); @@ -1953,11 +1954,11 @@ cache_compilation(int argc, const char* const* argv) try { Statistic statistic = do_cache_compilation(*ctx, argv); - ctx->counter_updates[statistic] += 1; + ctx->counter_updates.increment(statistic); return EXIT_SUCCESS; } catch (const Failure& e) { if (e.statistic() != Statistic::none) { - ctx->counter_updates[e.statistic()] += 1; + ctx->counter_updates.increment(e.statistic()); } if (e.exit_code()) { diff --git a/src/compress.cpp b/src/compress.cpp index 5ed62aee2..961faab76 100644 --- a/src/compress.cpp +++ b/src/compress.cpp @@ -201,10 +201,10 @@ recompress_file(Context& ctx, const size_t size_delta = (new_stat.size_on_disk() - old_stat.size_on_disk()) / 1024; if (ctx.stats_file() == stats_file) { - ctx.counter_updates[Statistic::cache_size_kibibyte] += size_delta; + ctx.counter_updates.increment(Statistic::cache_size_kibibyte, size_delta); } else { Counters counters; - counters[Statistic::cache_size_kibibyte] += size_delta; + counters.increment(Statistic::cache_size_kibibyte, size_delta); stats_flush_to_file(ctx.config, stats_file, counters); } diff --git a/src/stats.cpp b/src/stats.cpp index 53aa5438c..70350f828 100644 --- a/src/stats.cpp +++ b/src/stats.cpp @@ -161,7 +161,7 @@ parse_stats(Counters& counters, const std::string& buf) if (p2 == p) { break; } - counters[static_cast(i)] += val; + counters.increment(static_cast(i), val); i++; p = p2; } @@ -173,7 +173,7 @@ stats_write(const std::string& path, const Counters& counters) { AtomicFile file(path, AtomicFile::Mode::text); for (size_t i = 0; i < counters.size(); ++i) { - file.write(fmt::format("{}\n", counters[static_cast(i)])); + file.write(fmt::format("{}\n", counters.get(static_cast(i)))); } try { file.commit(); @@ -188,10 +188,10 @@ stats_write(const std::string& path, const Counters& counters) static double stats_hit_rate(const Counters& counters) { - uint64_t direct = counters[Statistic::direct_cache_hit]; - uint64_t preprocessed = counters[Statistic::preprocessed_cache_hit]; + uint64_t direct = counters.get(Statistic::direct_cache_hit); + uint64_t preprocessed = counters.get(Statistic::preprocessed_cache_hit); uint64_t hit = direct + preprocessed; - uint64_t miss = counters[Statistic::cache_miss]; + uint64_t miss = counters.get(Statistic::cache_miss); uint64_t total = hit + miss; return total > 0 ? (100.0 * hit) / total : 0.0; } @@ -213,17 +213,17 @@ stats_collect(const Config& config, Counters& counters, time_t* last_updated) fname = fmt::format("{}/{:x}/stats", config.cache_dir(), dir); } - counters[Statistic::stats_zeroed_timestamp] = 0; // Don't add + counters.set(Statistic::stats_zeroed_timestamp, 0); // Don't add stats_read(fname, counters); zero_timestamp = - std::max(counters[Statistic::stats_zeroed_timestamp], zero_timestamp); + std::max(counters.get(Statistic::stats_zeroed_timestamp), zero_timestamp); auto st = Stat::stat(fname); if (st && st.mtime() > *last_updated) { *last_updated = st.mtime(); } } - counters[Statistic::stats_zeroed_timestamp] = zero_timestamp; + counters.set(Statistic::stats_zeroed_timestamp, zero_timestamp); } // Read in the stats from one directory and add to the counters. @@ -256,7 +256,7 @@ stats_flush_to_file(const Config& config, if (!config.log_file().empty() || config.debug()) { for (auto& field : k_statistics_fields) { - if (updates[field.statistic] != 0 && !(field.flags & FLAG_NOZERO)) { + if (updates.get(field.statistic) != 0 && !(field.flags & FLAG_NOZERO)) { log("Result: {}", field.message); } } @@ -276,7 +276,8 @@ stats_flush_to_file(const Config& config, stats_read(sfile, counters); for (size_t i = 0; i < static_cast(Statistic::END); ++i) { - counters[static_cast(i)] += updates[static_cast(i)]; + counters.increment(static_cast(i), + updates.get(static_cast(i))); } stats_write(sfile, counters); } @@ -285,19 +286,19 @@ stats_flush_to_file(const Config& config, bool need_cleanup = false; if (config.max_files() != 0 - && counters[Statistic::files_in_cache] > config.max_files() / 16) { + && counters.get(Statistic::files_in_cache) > config.max_files() / 16) { log("Need to clean up {} since it holds {} files (limit: {} files)", subdir, - counters[Statistic::files_in_cache], + counters.get(Statistic::files_in_cache), config.max_files() / 16); need_cleanup = true; } if (config.max_size() != 0 - && counters[Statistic::cache_size_kibibyte] + && counters.get(Statistic::cache_size_kibibyte) > config.max_size() / 1024 / 16) { log("Need to clean up {} since it holds {} KiB (limit: {} KiB)", subdir, - counters[Statistic::cache_size_kibibyte], + counters.get(Statistic::cache_size_kibibyte), config.max_size() / 1024 / 16); need_cleanup = true; } @@ -349,16 +350,16 @@ stats_summary(const Context& ctx) if (k_statistics_fields[i].flags & FLAG_NEVER) { continue; } - if (counters[statistic] == 0 + if (counters.get(statistic) == 0 && !(k_statistics_fields[i].flags & FLAG_ALWAYS)) { continue; } std::string value; if (k_statistics_fields[i].format) { - value = k_statistics_fields[i].format(counters[statistic]); + value = k_statistics_fields[i].format(counters.get(statistic)); } else { - value = fmt::format("{:8}", counters[statistic]); + value = fmt::format("{:8}", counters.get(statistic)); } if (!value.empty()) { fmt::print("{:31} {}\n", k_statistics_fields[i].message, value); @@ -394,7 +395,7 @@ stats_print(const Config& config) if (!(k_statistics_fields[i].flags & FLAG_NEVER)) { fmt::print("{}\t{}\n", k_statistics_fields[i].id, - counters[k_statistics_fields[i].statistic]); + counters.get(k_statistics_fields[i].statistic)); } } } @@ -420,10 +421,10 @@ stats_zero(const Context& ctx) stats_read(fname, counters); for (size_t i = 0; k_statistics_fields[i].message; i++) { if (!(k_statistics_fields[i].flags & FLAG_NOZERO)) { - counters[k_statistics_fields[i].statistic] = 0; + counters.set(k_statistics_fields[i].statistic, 0); } } - counters[Statistic::stats_zeroed_timestamp] = timestamp; + counters.set(Statistic::stats_zeroed_timestamp, timestamp); stats_write(fname, counters); } } @@ -441,8 +442,8 @@ stats_get_obsolete_limits(const std::string& dir, Counters counters; std::string sname = dir + "/stats"; stats_read(sname, counters); - *maxfiles = counters[Statistic::obsolete_max_files]; - *maxsize = counters[Statistic::obsolete_max_size] * 1024; + *maxfiles = counters.get(Statistic::obsolete_max_files); + *maxsize = counters.get(Statistic::obsolete_max_size) * 1024; } // Set the per-directory sizes. @@ -454,8 +455,8 @@ stats_set_sizes(const std::string& dir, uint64_t num_files, uint64_t total_size) Lockfile lock(statsfile); if (lock.acquired()) { stats_read(statsfile, counters); - counters[Statistic::files_in_cache] = num_files; - counters[Statistic::cache_size_kibibyte] = total_size / 1024; + counters.set(Statistic::files_in_cache, num_files); + counters.set(Statistic::cache_size_kibibyte, total_size / 1024); stats_write(statsfile, counters); } } @@ -469,7 +470,7 @@ stats_add_cleanup(const std::string& dir, uint64_t count) Lockfile lock(statsfile); if (lock.acquired()) { stats_read(statsfile, counters); - counters[Statistic::cleanups_performed] += count; + counters.increment(Statistic::cleanups_performed, count); stats_write(statsfile, counters); } }