From 467af7262cac6d9621ee30ecb2d2b713cddb99c4 Mon Sep 17 00:00:00 2001 From: Kevin Doherty Date: Fri, 18 Oct 2024 12:25:55 -0700 Subject: [PATCH] Track setting access count Summary: Setting access information can be used to: * Flag unused settings and dead code paths that are eligible for cleanup * Implement change safety tooling which can leverage this granular (per-setting) consumption information * Figure out which settings are only accessed once during startup and should probably be marked `Immutable` instead of `Mutable` * Figure out which settings are "hot" * `DistributedCounter` benchmarks (could consider committing although they're somewhat redundant with SettingsBenchmarks) https://www.internalfb.com/intern/diff/view-version/248548896/ Reviewed By: yfeldblum Differential Revision: D64188812 fbshipit-source-id: 3d5f25ce6334c6baf1cc7a7f2a44dc8f188e05f9 --- folly/settings/BUCK | 1 + folly/settings/Settings.h | 92 +++++++++++------ folly/settings/detail/SettingsImpl.h | 34 +++--- folly/settings/test/BUCK | 6 +- folly/settings/test/SettingsBenchmarks.cpp | 24 ++--- folly/settings/test/SettingsTest.cpp | 114 +++++++++++++++++++++ 6 files changed, 209 insertions(+), 62 deletions(-) diff --git a/folly/settings/BUCK b/folly/settings/BUCK index bae8fc6946b..28115978e63 100644 --- a/folly/settings/BUCK +++ b/folly/settings/BUCK @@ -25,6 +25,7 @@ cpp_library( "//folly:shared_mutex", "//folly:thread_local", "//folly:utility", + "//folly/concurrency:singleton_relaxed_counter", "//folly/container:f14_hash", "//folly/lang:aligned", ], diff --git a/folly/settings/Settings.h b/folly/settings/Settings.h index 4415c2192b5..011d7c1d365 100644 --- a/folly/settings/Settings.h +++ b/folly/settings/Settings.h @@ -22,6 +22,7 @@ #include #include #include +#include #include #include @@ -35,10 +36,12 @@ namespace detail { * @param TrivialPtr location of the small type storage. Optimization * for better inlining. */ -template * TrivialPtr> +template * TrivialPtr, typename Tag> class SettingWrapper { + using AccessCounter = typename SettingCore::AccessCounter; + public: - using CallbackHandle = typename SettingCore::CallbackHandle; + using CallbackHandle = typename SettingCore::CallbackHandle; /** * Returns the setting's current value. @@ -50,9 +53,13 @@ class SettingWrapper { * here after some amount of time (on the order of minutes). */ std::conditional_t, T, const T&> operator*() const { + AccessCounter::add(1); return core_.getWithHint(*TrivialPtr); } - const T* operator->() const { return &core_.getSlow().value; } + const T* operator->() const { + AccessCounter::add(1); + return &core_.getSlow().value; + } /** * Returns the setting's current value. Equivalent to dereference operator @@ -94,7 +101,8 @@ class SettingWrapper { * @returns a handle object which automatically removes the callback from * processing once destroyd */ - CallbackHandle addCallback(typename SettingCore::UpdateCallback callback) { + CallbackHandle addCallback( + typename SettingCore::UpdateCallback callback) { return core_.addCallback(std::move(callback)); } @@ -111,15 +119,20 @@ class SettingWrapper { */ StringPiece updateReason() const { return core_.getSlow().updateReason; } + /** + * Returns the number of times this setting has been accessed. + */ + uint64_t accessCount() const { return AccessCounter::count(); } + /** * Returns the setting's update reason in the snapshot. */ StringPiece updateReason(const Snapshot& snapshot) const; - explicit SettingWrapper(SettingCore& core) : core_(core) {} + explicit SettingWrapper(SettingCore& core) : core_(core) {} private: - SettingCore& core_; + SettingCore& core_; friend class folly::settings::Snapshot; }; @@ -140,11 +153,15 @@ class SettingWrapper { */ #define FOLLY_DETAIL_SETTINGS_DEFINE_LOCAL_FUNC__( \ _project, _name, _Type, _overloadType) \ - extern ::std::atomic<::folly::settings::detail::SettingCore<_Type>*> \ + struct FOLLY_SETTINGS_TAG__##_project##_##_name; \ + extern ::std::atomic<::folly::settings::detail::SettingCore< \ + _Type, \ + FOLLY_SETTINGS_TAG__##_project##_##_name>*> \ FOLLY_SETTINGS_CACHE__##_project##_##_name; \ extern ::std::atomic FOLLY_SETTINGS_TRIVIAL__##_project##_##_name; \ - ::folly::settings::detail::SettingCore<_Type>& \ - FOLLY_SETTINGS_FUNC__##_project##_##_name(); \ + ::folly::settings::detail:: \ + SettingCore<_Type, FOLLY_SETTINGS_TAG__##_project##_##_name>& \ + FOLLY_SETTINGS_FUNC__##_project##_##_name(); \ FOLLY_ALWAYS_INLINE auto FOLLY_SETTINGS_LOCAL_FUNC__##_project##_##_name( \ _overloadType) { \ auto* folly_detail_settings_value = \ @@ -156,13 +173,16 @@ class SettingWrapper { FOLLY_SETTINGS_CACHE__##_project##_##_name.store( \ folly_detail_settings_value, ::std::memory_order_release); \ } \ - return ::folly::settings::detail:: \ - SettingWrapper<_Type, &FOLLY_SETTINGS_TRIVIAL__##_project##_##_name>( \ - *folly_detail_settings_value); \ + return ::folly::settings::detail::SettingWrapper< \ + _Type, \ + &FOLLY_SETTINGS_TRIVIAL__##_project##_##_name, \ + FOLLY_SETTINGS_TAG__##_project##_##_name>( \ + *folly_detail_settings_value); \ } \ /* This is here just to force a semicolon */ \ - ::folly::settings::detail::SettingCore<_Type>& \ - FOLLY_SETTINGS_FUNC__##_project##_##_name() + ::folly::settings::detail:: \ + SettingCore<_Type, FOLLY_SETTINGS_TAG__##_project##_##_name>& \ + FOLLY_SETTINGS_FUNC__##_project##_##_name() } // namespace detail @@ -193,11 +213,13 @@ class SettingWrapper { * @param _desc setting documentation */ #define FOLLY_SETTING_DEFINE(_project, _name, _Type, _def, _mut, _cli, _desc) \ + struct FOLLY_SETTINGS_TAG__##_project##_##_name {}; \ /* Fastpath optimization, see notes in FOLLY_SETTINGS_DEFINE_LOCAL_FUNC__. \ Aggregate all off these together in a single section for better TLB \ and cache locality. */ \ __attribute__((__section__(".folly.settings.cache"))) ::std::atomic< \ - ::folly::settings::detail::SettingCore<_Type>*> \ + ::folly::settings::detail:: \ + SettingCore<_Type, FOLLY_SETTINGS_TAG__##_project##_##_name>*> \ FOLLY_SETTINGS_CACHE__##_project##_##_name; \ /* Location for the small value cache (if _Type is small and trivial). \ Intentionally located right after the pointer cache above to take \ @@ -206,10 +228,12 @@ class SettingWrapper { __section__(".folly.settings.cache"))) ::std::atomic \ FOLLY_SETTINGS_TRIVIAL__##_project##_##_name; \ /* Meyers singleton to avoid SIOF */ \ - FOLLY_NOINLINE ::folly::settings::detail::SettingCore<_Type>& \ - FOLLY_SETTINGS_FUNC__##_project##_##_name() { \ - static ::folly::Indestructible< \ - ::folly::settings::detail::SettingCore<_Type>> \ + FOLLY_NOINLINE ::folly::settings::detail:: \ + SettingCore<_Type, FOLLY_SETTINGS_TAG__##_project##_##_name>& \ + FOLLY_SETTINGS_FUNC__##_project##_##_name() { \ + static ::folly::Indestructible<::folly::settings::detail::SettingCore< \ + _Type, \ + FOLLY_SETTINGS_TAG__##_project##_##_name>> \ setting( \ ::folly::settings::SettingMetadata{ \ #_project, \ @@ -264,7 +288,7 @@ namespace detail { * Like SettingWrapper, but checks against any values saved/updated in a * snapshot. */ -template +template class SnapshotSettingWrapper { public: /** @@ -286,10 +310,10 @@ class SnapshotSettingWrapper { private: Snapshot& snapshot_; - SettingCore& core_; + SettingCore& core_; friend class folly::settings::Snapshot; - SnapshotSettingWrapper(Snapshot& snapshot, SettingCore& core) + SnapshotSettingWrapper(Snapshot& snapshot, SettingCore& core) : snapshot_(snapshot), core_(core) {} }; @@ -328,10 +352,10 @@ class Snapshot final : public detail::SnapshotBase { /** * Wraps a global FOLLY_SETTING(a, b) and returns a snapshot-local wrapper. */ - template * P> - detail::SnapshotSettingWrapper operator()( - detail::SettingWrapper&& setting) { - return detail::SnapshotSettingWrapper(*this, setting.core_); + template * P, typename Tag> + detail::SnapshotSettingWrapper operator()( + detail::SettingWrapper&& setting) { + return detail::SnapshotSettingWrapper(*this, setting.core_); } /** @@ -397,27 +421,27 @@ class Snapshot final : public detail::SnapshotBase { FunctionRef func) const override; private: - template + template friend class detail::SnapshotSettingWrapper; - template * TrivialPtr> + template * TrivialPtr, typename Tag> friend class detail::SettingWrapper; }; namespace detail { -template -inline const T& SnapshotSettingWrapper::operator*() const { +template +inline const T& SnapshotSettingWrapper::operator*() const { return snapshot_.get(core_).value; } -template * TrivialPtr> +template * TrivialPtr, typename Tag> inline std::conditional_t, T, const T&> -SettingWrapper::value(const Snapshot& snapshot) const { +SettingWrapper::value(const Snapshot& snapshot) const { return snapshot.get(core_).value; } -template * TrivialPtr> -StringPiece SettingWrapper::updateReason( +template * TrivialPtr, typename Tag> +StringPiece SettingWrapper::updateReason( const Snapshot& snapshot) const { return snapshot.get(core_).updateReason; } diff --git a/folly/settings/detail/SettingsImpl.h b/folly/settings/detail/SettingsImpl.h index 4306d3702c9..3f1d0b9007a 100644 --- a/folly/settings/detail/SettingsImpl.h +++ b/folly/settings/detail/SettingsImpl.h @@ -30,6 +30,7 @@ #include #include #include +#include #include #include #include @@ -72,6 +73,7 @@ class SettingCoreBase { virtual SetResult resetToDefault(SnapshotBase* snapshot) = 0; virtual void forceResetToDefault(SnapshotBase* snapshot) = 0; virtual const SettingMetadata& meta() const = 0; + virtual uint64_t accessCount() const = 0; virtual ~SettingCoreBase() {} /** @@ -87,7 +89,7 @@ void registerSetting(SettingCoreBase& core); */ SettingCoreBase::Version nextGlobalVersion(); -template +template class SettingCore; /** @@ -108,11 +110,11 @@ class BoxedValue { * Stores a value that can be both retrieved later and optionally * applied globally */ - template - BoxedValue(const T& value, StringPiece reason, SettingCore& core) + template + BoxedValue(const T& value, StringPiece reason, SettingCore& core) : value_(std::make_shared>(reason.str(), value)), core_{&core}, - publish_{doPublish} {} + publish_{doPublish} {} /** * Returns the reference to the stored value @@ -134,10 +136,10 @@ class BoxedValue { private: using PublishFun = void(BoxedValue&, const FrozenSettingProjects&); - template + template static void doPublish( BoxedValue& boxed, const FrozenSettingProjects& frozenProjects) { - auto& core = *static_cast*>(boxed.core_); + auto& core = *static_cast*>(boxed.core_); if (core.meta().mutability == Mutability::Immutable && frozenProjects.contains(core.meta().project)) { return; @@ -189,6 +191,7 @@ class SnapshotBase { const SettingMetadata& meta() const { return core_.meta(); } std::pair valueAndReason() const; const std::string& fullName() const { return fullName_; } + uint64_t accessCount() const { return core_.accessCount(); } private: const std::string& fullName_; @@ -254,7 +257,7 @@ class SnapshotBase { std::unordered_map snapshotValues_; - template + template friend class SettingCore; SnapshotBase(); @@ -264,8 +267,8 @@ class SnapshotBase { SnapshotBase(SnapshotBase&&) = delete; SnapshotBase& operator=(SnapshotBase&&) = delete; - template - const SettingContents& get(const detail::SettingCore& core) const { + template + const SettingContents& get(const detail::SettingCore& core) const { auto it = snapshotValues_.find(core.getKey()); if (it != snapshotValues_.end()) { return it->second.template unbox(); @@ -277,16 +280,17 @@ class SnapshotBase { return core.getSlow(); } - template - void set(detail::SettingCore& core, const T& t, StringPiece reason) { + template + void set(detail::SettingCore& core, const T& t, StringPiece reason) { snapshotValues_[core.getKey()] = detail::BoxedValue(t, reason, core); } }; -template +template class SettingCore : public SettingCoreBase { public: using Contents = SettingContents; + using AccessCounter = SingletonRelaxedCounter; SetResult setFromString( StringPiece newValue, @@ -329,6 +333,8 @@ class SettingCore : public SettingCoreBase { const SettingMetadata& meta() const override { return meta_; } + uint64_t accessCount() const override { return AccessCounter::count(); } + /** * @param trivialStorage must refer to the same location * as the internal trivialStorage_. This hint will @@ -363,7 +369,7 @@ class SettingCore : public SettingCoreBase { class CallbackHandle { public: CallbackHandle( - std::shared_ptr callback, SettingCore& setting) + std::shared_ptr callback, SettingCore& setting) : callback_(std::move(callback)), setting_(setting) {} ~CallbackHandle() { if (callback_) { @@ -378,7 +384,7 @@ class SettingCore : public SettingCoreBase { private: std::shared_ptr callback_; - SettingCore& setting_; + SettingCore& setting_; }; CallbackHandle addCallback(UpdateCallback callback) { auto callbackPtr = copy_to_shared_ptr(std::move(callback)); diff --git a/folly/settings/test/BUCK b/folly/settings/test/BUCK index a496df04d28..87603a04e55 100644 --- a/folly/settings/test/BUCK +++ b/folly/settings/test/BUCK @@ -22,13 +22,15 @@ cpp_unittest( "SettingsTest.cpp", ], deps = [ + "fbsource//third-party/fmt:fmt", + ":a", + ":b", "//folly:format", "//folly:string", "//folly/portability:gmock", "//folly/portability:gtest", "//folly/settings:settings", - "//folly/settings/test:a", - "//folly/settings/test:b", + "//folly/synchronization/test:barrier", ], ) diff --git a/folly/settings/test/SettingsBenchmarks.cpp b/folly/settings/test/SettingsBenchmarks.cpp index 9f7a79bec1d..0f6c9d0028a 100644 --- a/folly/settings/test/SettingsBenchmarks.cpp +++ b/folly/settings/test/SettingsBenchmarks.cpp @@ -28,20 +28,20 @@ buck run @mode/opt folly/settings/test:settings_bench -- --bm_min_iters=10000000 ============================================================================ [...]/settings/test/SettingsBenchmarks.cpp relative time/iter iters/s ============================================================================ -trivial_access 380.90fs 2.63T -non_trivial_access 1.01ns 987.97M +trivial_access 290.59ps 3.44G +non_trivial_access 1.27ns 787.19M ---------------------------------------------------------------------------- -trival_access_parallel(1thr) 138.92ps 7.20G -trival_access_parallel(8thr) 311.42ps 3.21G -trival_access_parallel(24thr) 451.58ps 2.21G -trival_access_parallel(48thr) 793.19ps 1.26G -trival_access_parallel(72thr) 749.04ps 1.34G +trival_access_parallel(1thr) 482.59ps 2.07G +trival_access_parallel(8thr) 530.65ps 1.88G +trival_access_parallel(24thr) 816.65ps 1.22G +trival_access_parallel(48thr) 1.10ns 911.76M +trival_access_parallel(72thr) 1.32ns 756.95M ---------------------------------------------------------------------------- -non_trival_access_parallel(1thr) 1.22ns 819.36M -non_trival_access_parallel(8thr) 1.73ns 577.11M -non_trival_access_parallel(24thr) 2.05ns 488.47M -non_trival_access_parallel(48thr) 3.20ns 312.18M -non_trival_access_parallel(72thr) 3.73ns 267.76M +non_trival_access_parallel(1thr) 1.53ns 651.83M +non_trival_access_parallel(8thr) 1.60ns 623.54M +non_trival_access_parallel(24thr) 2.36ns 423.37M +non_trival_access_parallel(48thr) 3.09ns 323.19M +non_trival_access_parallel(72thr) 3.77ns 265.19M */ FOLLY_SETTING_DEFINE( diff --git a/folly/settings/test/SettingsTest.cpp b/folly/settings/test/SettingsTest.cpp index e48c4c89278..353d9f6e2a5 100644 --- a/folly/settings/test/SettingsTest.cpp +++ b/folly/settings/test/SettingsTest.cpp @@ -16,10 +16,16 @@ #include +#include +#include + +#include + #include #include #include #include +#include #include #include @@ -117,6 +123,22 @@ void toAppend(const UserDefinedWithMeta& t, String* out) { out->append(t.value_); } +struct TrivialUserDefinedType { + int value() const { return value_; } + + int value_; +}; +template +void toAppend(const TrivialUserDefinedType& t, String* out) { + out->append(fmt::to_string(t.value_)); +} +folly::Expected convertTo( + const folly::settings::SettingValueAndMetadata& src, + TrivialUserDefinedType& out) { + out.value_ = folly::to(src.value); + return folly::unit; +} + FOLLY_SETTING_DEFINE( follytest, user_defined, @@ -133,6 +155,14 @@ FOLLY_SETTING_DEFINE( folly::settings::Mutability::Mutable, folly::settings::CommandLine::AcceptOverrides, "User defined type constructed from string and metadata"); +FOLLY_SETTING_DEFINE( + follytest, + trivial_user_defined, + TrivialUserDefinedType, + TrivialUserDefinedType{123}, + folly::settings::Mutability::Mutable, + folly::settings::CommandLine::AcceptOverrides, + "Trivial user defined type"); FOLLY_SETTING_DEFINE( follytest, immutable_setting, @@ -336,6 +366,8 @@ TEST(Settings, basic) { EXPECT_EQ(meta.typeStr, "UserDefinedType"); } else if (meta.typeId == typeid(some_ns::UserDefinedWithMeta)) { EXPECT_EQ(meta.typeStr, "UserDefinedWithMeta"); + } else if (meta.typeId == typeid(some_ns::TrivialUserDefinedType)) { + EXPECT_EQ(meta.typeStr, "TrivialUserDefinedType"); } else { FAIL() << "Unexpected type: " << meta.typeStr; } @@ -358,6 +390,7 @@ TEST(Settings, basic) { follytest/public_flag_to_a/int/456/Public flag to a/300/from_string follytest/public_flag_to_b/std::string/"basdf"/Public flag to b/basdf/default follytest/some_flag/std::string/"default"/Description/default/default + follytest/trivial_user_defined/TrivialUserDefinedType/TrivialUserDefinedType{123}/Trivial user defined type/123/default follytest/unused/std::string/"unused_default"/Not used, but should still be in the list/unused_default/default follytest/user_defined/UserDefinedType/"b"/User defined type constructed from string/b_out/default follytest/user_defined_with_meta/UserDefinedWithMeta/{"default"}/User defined type constructed from string and metadata/default/default @@ -751,3 +784,84 @@ TEST(Settings, userDefinedConversionWithMetadata) { some_ns::FOLLY_SETTING(follytest, user_defined_with_meta)->value_, "follytest_user_defined_with_meta->new"); } + +TEST(Settings, accessCount) { + { + folly::settings::Snapshot sn; + sn.forEachSetting( + [](auto setting) { EXPECT_EQ(setting.accessCount(), 0); }); + } + + // Check updateReason does not count as an access + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, multi_token_type).accessCount(), 0); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, multi_token_type).updateReason(), + "default"); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, multi_token_type).accessCount(), 0); + + { + // Check accessing a setting in a snapshot does not count as an access + EXPECT_EQ(some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), 0); + folly::settings::Snapshot sn; + EXPECT_EQ(*sn(some_ns::FOLLY_SETTING(follytest, some_flag)), "default"); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, some_flag).value(sn), "default"); + EXPECT_EQ(some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), 0); + } + + // Check trivial setting access + EXPECT_EQ(*some_ns::FOLLY_SETTING(follytest, multi_token_type), 123); + EXPECT_EQ(*some_ns::FOLLY_SETTING(follytest, multi_token_type), 123); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, multi_token_type).accessCount(), 2); + + // Check non-trivial setting access + EXPECT_EQ(some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), 0); + EXPECT_EQ(*some_ns::FOLLY_SETTING(follytest, some_flag), "default"); + EXPECT_EQ(some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), 1); + + // Check trival access via -> + static_assert( + folly::settings::detail::IsSmallPOD); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, trivial_user_defined).accessCount(), 0); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, trivial_user_defined)->value(), 123); + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, trivial_user_defined).accessCount(), 1); + + folly::settings::Snapshot sn; + sn.forEachSetting([](const auto& setting) { + EXPECT_EQ( + setting.accessCount() > 0, + setting.fullName() == "follytest_multi_token_type" || + setting.fullName() == "follytest_some_flag" || + setting.fullName() == "follytest_trivial_user_defined") + << setting.fullName(); + }); +} + +TEST(Settings, concurrentAccessCount) { + EXPECT_EQ(some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), 0); + const size_t numThreads = 16; + const size_t numAccessesPerThread = 10'000; + std::vector threads; + folly::test::Barrier barrier(numThreads + 1); + for (size_t i = 0; i < numThreads; ++i) { + threads.emplace_back([&]() { + barrier.wait(); + for (size_t j = 0; j < numAccessesPerThread; ++j) { + EXPECT_EQ(*some_ns::FOLLY_SETTING(follytest, some_flag), "default"); + } + }); + } + barrier.wait(); + for (auto& thread : threads) { + thread.join(); + } + EXPECT_EQ( + some_ns::FOLLY_SETTING(follytest, some_flag).accessCount(), + numThreads * numAccessesPerThread); +}