Optimize absl/flags registration, default validation, and FlagSaver. 1. Pass `flag_.impl_.filename_ == filename ? nullptr : filename` from `FlagRegistrar` to `RegisterCommandLineFlag` so `FlagImpl` skips calling `GetUsageConfig().normalize_filename` (which acquires `custom_config_guard` twice, copies 10 `std::function` objects, and allocates/frees 2 `std::string`s per `ABSL_FLAG` at startup) when the filename pointer is already set in the `constexpr` constructor. 2. Remove the duplicate `ValidateDefaultValuesOfUnspecifiedFlags()` pass in `ParseAbseilFlagsOnlyImpl`, replace the 13-type `FastTypeId(op)` check in `ShouldValidateFlagValue` with direct `op == &FlagOps<T>` pointer equality, consolidate `DefaultValue()` and `MakeInitValue()` in `FlagImpl::CheckDefaultValueParsingRoundtrip()` under a single lock and allocation, and add lock-free `seq_lock_.IsUnmodified()` fast paths to `FlagImpl::IsModified()` and `IsSpecifiedOnCommandLine()`. 3. Return `nullptr` from `FlagImpl::SaveState()` when `CanSkipInitOnSave()` and restore via `RestoreToInitialState()` (preserving the initial `FlagDefaultSrc` alongside `SET_FLAGS_DEFAULT` values in `DynamicDefault` and matching saved handles linearly in `FlagSaverImpl`), eliminating ~10,000 `FlagState` heap allocations/deletions per `FlagSaver` scope. PiperOrigin-RevId: 989386174
diff --git a/absl/flags/commandlineflag.cc b/absl/flags/commandlineflag.cc index 4140230..3b3b785 100644 --- a/absl/flags/commandlineflag.cc +++ b/absl/flags/commandlineflag.cc
@@ -26,6 +26,7 @@ absl::string_view CommandLineFlag::TypeName() const { return ""; } bool CommandLineFlag::IsRetired() const { return false; } +void CommandLineFlag::RestoreToInitialState() {} bool CommandLineFlag::ParseFrom(absl::string_view value, std::string* error) { return ParseFrom(value, flags_internal::SET_FLAGS_VALUE, flags_internal::kProgrammaticChange, *error);
diff --git a/absl/flags/commandlineflag.h b/absl/flags/commandlineflag.h index 9336d6d..aab7dac 100644 --- a/absl/flags/commandlineflag.h +++ b/absl/flags/commandlineflag.h
@@ -188,6 +188,9 @@ // or nullptr if flag does not support saving and restoring a state. virtual std::unique_ptr<flags_internal::FlagStateInterface> SaveState() = 0; + // Restores a flag that was in its initial unmodified state when saved. + virtual void RestoreToInitialState(); + // Copy-construct a new value of the flag's type in a memory referenced by // the dst based on the current flag's value. virtual void Read(void* absl_nonnull dst) const = 0;
diff --git a/absl/flags/commandlineflag_test.cc b/absl/flags/commandlineflag_test.cc index b6d01b2..ded66fb 100644 --- a/absl/flags/commandlineflag_test.cc +++ b/absl/flags/commandlineflag_test.cc
@@ -37,6 +37,8 @@ // These are only used to test default values. ABSL_FLAG(int, int_flag2, 201, ""); ABSL_FLAG(std::string, string_flag2, "dflt", ""); +ABSL_FLAG(int, int_flag3, 201, ""); +ABSL_FLAG(std::string, string_flag3, "dflt", ""); namespace { @@ -233,4 +235,34 @@ // EXPECT_EQ(err, "ERROR: int_flag is already set to 201"); } +TEST_F(CommandLineFlagTest, TestFlagSaverRestoresUnmodifiedSetFlagsDefault) { + std::string err; + auto* int_handle = absl::FindCommandLineFlag("int_flag3"); + auto* str_handle = absl::FindCommandLineFlag("string_flag3"); + ASSERT_NE(int_handle, nullptr); + ASSERT_NE(str_handle, nullptr); + + { + absl::FlagSaver saver; + EXPECT_TRUE(flags::PrivateHandleAccessor::ParseFrom( + *int_handle, "111", flags::SET_FLAGS_DEFAULT, + flags::kProgrammaticChange, err)); + EXPECT_TRUE(flags::PrivateHandleAccessor::ParseFrom( + *int_handle, "222", flags::SET_FLAGS_DEFAULT, + flags::kProgrammaticChange, err)); + EXPECT_EQ(absl::GetFlag(FLAGS_int_flag3), 222); + + EXPECT_TRUE(flags::PrivateHandleAccessor::ParseFrom( + *str_handle, "abc", flags::SET_FLAGS_DEFAULT, + flags::kProgrammaticChange, err)); + EXPECT_TRUE(flags::PrivateHandleAccessor::ParseFrom( + *str_handle, "def", flags::SET_FLAGS_DEFAULT, + flags::kProgrammaticChange, err)); + EXPECT_EQ(absl::GetFlag(FLAGS_string_flag3), "def"); + } + + EXPECT_EQ(absl::GetFlag(FLAGS_int_flag3), 201); + EXPECT_EQ(absl::GetFlag(FLAGS_string_flag3), "dflt"); +} + } // namespace
diff --git a/absl/flags/internal/flag.cc b/absl/flags/internal/flag.cc index 662b342..9d4a8d9 100644 --- a/absl/flags/internal/flag.cc +++ b/absl/flags/internal/flag.cc
@@ -59,11 +59,17 @@ namespace { // Currently we only validate flag values for user-defined flag types. -bool ShouldValidateFlagValue(FlagFastTypeId flag_type_id) { -#define DONT_VALIDATE(T, _) \ +bool ShouldValidateFlagValue(FlagOpFn op) { +#define DONT_VALIDATE_OP(T, _) \ + if (op == &FlagOps<T>) return false; + ABSL_FLAGS_INTERNAL_SUPPORTED_TYPES(DONT_VALIDATE_OP) +#undef DONT_VALIDATE_OP + + FlagFastTypeId flag_type_id = flags_internal::FastTypeId(op); +#define DONT_VALIDATE_ID(T, _) \ if (flag_type_id == absl::FastTypeId<T>()) return false; - ABSL_FLAGS_INTERNAL_SUPPORTED_TYPES(DONT_VALIDATE) -#undef DONT_VALIDATE + ABSL_FLAGS_INTERNAL_SUPPORTED_TYPES(DONT_VALIDATE_ID) +#undef DONT_VALIDATE_ID return true; } @@ -104,6 +110,11 @@ s_freelist->push_back(p); } +struct DynamicDefault { + void* value; + FlagDefaultSrc initial_default; +}; + } // namespace /////////////////////////////////////////////////////////////////////////////// @@ -129,15 +140,7 @@ on_command_line_(on_command_line), counter_(counter) {} - FlagState(FlagImpl& flag_impl, FlagDefaultSrc default_value) - : flag_impl_(flag_impl), - value_(default_value), - modified_(false), - on_command_line_(false), - counter_(-1) {} - ~FlagState() override { - if (counter_ < 0) return; if (flag_impl_.ValueStorageKind() != FlagValueStorageKind::kHeapAllocated && flag_impl_.ValueStorageKind() != FlagValueStorageKind::kSequenceLocked) return; @@ -161,11 +164,9 @@ union SavedValue { explicit SavedValue(void* v) : heap_allocated(v) {} explicit SavedValue(int64_t v) : one_word(v) {} - explicit SavedValue(FlagDefaultSrc v) : default_value(v) {} void* heap_allocated; int64_t one_word; - FlagDefaultSrc default_value; } value_; bool modified_; bool on_command_line_; @@ -296,7 +297,9 @@ void* res = nullptr; switch (DefaultKind()) { case FlagDefaultKind::kDynamicValue: - res = flags_internal::Clone(op_, default_value_.dynamic_value); + res = flags_internal::Clone( + op_, static_cast<const DynamicDefault*>(default_value_.dynamic_value) + ->value); break; case FlagDefaultKind::kGenFunc: res = flags_internal::Alloc(op_); @@ -374,6 +377,7 @@ } bool FlagImpl::IsSpecifiedOnCommandLine() const { + if (ABSL_PREDICT_TRUE(seq_lock_.IsUnmodified())) return false; absl::MutexLock l(DataGuard()); return on_command_line_; } @@ -464,14 +468,14 @@ case FlagValueStorageKind::kHeapAllocated: // Built-in types have side-effect-free defaults; user-defined types may // have custom construction or validation side effects. - return !ShouldValidateFlagValue(flags_internal::FastTypeId(op_)); + return !ShouldValidateFlagValue(op_); } return false; } std::unique_ptr<FlagStateInterface> FlagImpl::SaveState() { if (ABSL_PREDICT_TRUE(CanSkipInitOnSave())) { - return std::make_unique<FlagState>(*this, default_value_); + return nullptr; } absl::MutexLock l(DataGuard()); @@ -506,11 +510,42 @@ return nullptr; } -bool FlagImpl::RestoreState(const FlagState& flag_state) { - if (ABSL_PREDICT_TRUE(flag_state.counter_ < 0 && seq_lock_.IsUnmodified())) { - return false; +void FlagImpl::RestoreToInitialState() { + if (ABSL_PREDICT_TRUE(seq_lock_.IsUnmodified())) { + return; } + { + absl::MutexLock l(DataGuard()); + if (ModificationCount() == 0) { + return; + } + const FlagDefaultSrc initial_default = + DefaultKind() == FlagDefaultKind::kDynamicValue + ? static_cast<const DynamicDefault*>(default_value_.dynamic_value) + ->initial_default + : default_value_; + switch (ValueStorageKind()) { + case FlagValueStorageKind::kValueAndInitBit: + case FlagValueStorageKind::kOneWordAtomic: + StoreValue(&initial_default, kProgrammaticChange); + break; + case FlagValueStorageKind::kSequenceLocked: + case FlagValueStorageKind::kHeapAllocated: { + std::unique_ptr<void, DynValueDeleter> init_value( + flags_internal::Alloc(op_), DynValueDeleter{op_}); + (*initial_default.gen_func)(init_value.get()); + StoreValue(init_value.get(), kProgrammaticChange); + break; + } + } + modified_ = false; + on_command_line_ = false; + } + ABSL_INTERNAL_LOG(INFO, absl::StrCat("Restore saved value of ", Name(), + " to: ", CurrentValue())); +} +bool FlagImpl::RestoreState(const FlagState& flag_state) { absl::MutexLock l(DataGuard()); if (flag_state.counter_ == ModificationCount()) { return false; @@ -519,22 +554,11 @@ switch (ValueStorageKind()) { case FlagValueStorageKind::kValueAndInitBit: case FlagValueStorageKind::kOneWordAtomic: - StoreValue( - flag_state.counter_ < 0 - ? static_cast<const void*>(&flag_state.value_.default_value) - : static_cast<const void*>(&flag_state.value_.one_word), - kProgrammaticChange); + StoreValue(&flag_state.value_.one_word, kProgrammaticChange); break; case FlagValueStorageKind::kSequenceLocked: case FlagValueStorageKind::kHeapAllocated: - if (flag_state.counter_ < 0) { - std::unique_ptr<void, DynValueDeleter> init_value( - flags_internal::Alloc(op_), DynValueDeleter{op_}); - (*flag_state.value_.default_value.gen_func)(init_value.get()); - StoreValue(init_value.get(), kProgrammaticChange); - } else { - StoreValue(flag_state.value_.heap_allocated, kProgrammaticChange); - } + StoreValue(flag_state.value_.heap_allocated, kProgrammaticChange); break; } @@ -652,7 +676,7 @@ void FlagImpl::Write(const void* src) { absl::MutexLock l(DataGuard()); - if (ShouldValidateFlagValue(flags_internal::FastTypeId(op_))) { + if (ShouldValidateFlagValue(op_)) { std::unique_ptr<void, DynValueDeleter> obj{flags_internal::Clone(op_, src), DynValueDeleter{op_}}; std::string ignored_error; @@ -713,17 +737,22 @@ if (!tentative_value) return false; if (DefaultKind() == FlagDefaultKind::kDynamicValue) { - void* old_value = default_value_.dynamic_value; - default_value_.dynamic_value = tentative_value.release(); + auto* dyn = static_cast<DynamicDefault*>(default_value_.dynamic_value); + void* old_value = dyn->value; + dyn->value = tentative_value.release(); tentative_value.reset(old_value); } else { - default_value_.dynamic_value = tentative_value.release(); + default_value_.dynamic_value = + new DynamicDefault{tentative_value.release(), default_value_}; def_kind_ = static_cast<uint8_t>(FlagDefaultKind::kDynamicValue); } if (!modified_) { // Need to set both default value *and* current, in this case. - StoreValue(default_value_.dynamic_value, source); + StoreValue( + static_cast<const DynamicDefault*>(default_value_.dynamic_value) + ->value, + source); modified_ = false; } break; @@ -734,11 +763,12 @@ } void FlagImpl::CheckDefaultValueParsingRoundtrip() const { - std::string v = DefaultValue(); + if (!ShouldValidateFlagValue(op_)) return; absl::MutexLock lock(DataGuard()); auto dst = MakeInitValue(); + std::string v = flags_internal::Unparse(op_, dst.get()); std::string error; if (!flags_internal::Parse(op_, v, dst.get(), &error)) { ABSL_INTERNAL_LOG(
diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h index 8baff43..f625456 100644 --- a/absl/flags/internal/flag.h +++ b/absl/flags/internal/flag.h
@@ -641,6 +641,8 @@ private: template <typename T> friend class Flag; + template <typename, bool> + friend class FlagRegistrar; friend class FlagState; // Ensures that `data_guard_` is initialized and returns it. @@ -718,6 +720,7 @@ // saving and restoring a state. std::unique_ptr<FlagStateInterface> SaveState() override ABSL_LOCKS_EXCLUDED(DataGuard()); + void RestoreToInitialState() override ABSL_LOCKS_EXCLUDED(DataGuard()); // Restores the flag state to the supplied state object. If there is // nothing to restore returns false. Otherwise returns true. @@ -939,8 +942,10 @@ public: constexpr explicit FlagRegistrar(Flag<T>& flag, const char* filename) : flag_(flag) { - if (do_register) - flags_internal::RegisterCommandLineFlag(flag_.impl_, filename); + if (do_register) { + flags_internal::RegisterCommandLineFlag( + flag_.impl_, flag_.impl_.filename_ == filename ? nullptr : filename); + } } FlagRegistrar OnUpdate(FlagCallbackFunc cb) && {
diff --git a/absl/flags/internal/private_handle_accessor.cc b/absl/flags/internal/private_handle_accessor.cc index 610d1ff..145b1b4 100644 --- a/absl/flags/internal/private_handle_accessor.cc +++ b/absl/flags/internal/private_handle_accessor.cc
@@ -36,6 +36,10 @@ return flag.SaveState(); } +void PrivateHandleAccessor::RestoreToInitialState(CommandLineFlag& flag) { + flag.RestoreToInitialState(); +} + bool PrivateHandleAccessor::IsSpecifiedOnCommandLine( const CommandLineFlag& flag) { return flag.IsSpecifiedOnCommandLine();
diff --git a/absl/flags/internal/private_handle_accessor.h b/absl/flags/internal/private_handle_accessor.h index bf4154c..fedf750 100644 --- a/absl/flags/internal/private_handle_accessor.h +++ b/absl/flags/internal/private_handle_accessor.h
@@ -39,6 +39,9 @@ // Access to CommandLineFlag::SaveState. static std::unique_ptr<FlagStateInterface> SaveState(CommandLineFlag& flag); + // Access to CommandLineFlag::RestoreToInitialState. + static void RestoreToInitialState(CommandLineFlag& flag); + // Access to CommandLineFlag::IsSpecifiedOnCommandLine. static bool IsSpecifiedOnCommandLine(const CommandLineFlag& flag);
diff --git a/absl/flags/parse.cc b/absl/flags/parse.cc index 940bf56..9b9502d 100644 --- a/absl/flags/parse.cc +++ b/absl/flags/parse.cc
@@ -335,13 +335,6 @@ #ifndef NDEBUG flags_internal::ForEachFlag([&](CommandLineFlag& flag) { if (flag.IsRetired()) return; - -#define ABSL_FLAGS_INTERNAL_IGNORE_TYPE(T, _) \ - if (flag.IsOfType<T>()) return; - - ABSL_FLAGS_INTERNAL_SUPPORTED_TYPES(ABSL_FLAGS_INTERNAL_IGNORE_TYPE) -#undef ABSL_FLAGS_INTERNAL_IGNORE_TYPE - flags_internal::PrivateHandleAccessor::CheckDefaultValueParsingRoundtrip( flag); });
diff --git a/absl/flags/reflection.cc b/absl/flags/reflection.cc index 13f210c..d8303db 100644 --- a/absl/flags/reflection.cc +++ b/absl/flags/reflection.cc
@@ -19,6 +19,7 @@ #include <algorithm> #include <atomic> +#include <cstddef> #include <cstdlib> #include <functional> #include <memory> @@ -319,24 +320,52 @@ // It's an error to call this more than once. void SaveFromRegistry() { assert(backup_registry_.empty()); // call only once! + saved_finalized_ = FlagRegistry::GlobalRegistry().finalized_flags_.load( + std::memory_order_acquire); flags_internal::ForEachFlag([&](CommandLineFlag& flag) { if (auto flag_state = flags_internal::PrivateHandleAccessor::SaveState(flag)) { - backup_registry_.emplace_back(std::move(flag_state)); + backup_registry_.emplace_back(&flag, std::move(flag_state)); } }); + if (!saved_finalized_) { + std::sort(backup_registry_.begin(), backup_registry_.end(), + [](const auto& a, const auto& b) { + return std::less<CommandLineFlag*>()(a.first, b.first); + }); + } } // Restores the saved flag states into the flag registry. void RestoreToRegistry() && { - for (const auto& flag_state : backup_registry_) { - std::move(*flag_state).Restore(); - } + size_t index = 0; + flags_internal::ForEachFlag([&](CommandLineFlag& flag) { + if (saved_finalized_) { + if (index < backup_registry_.size() && + backup_registry_[index].first == &flag) { + std::move(*backup_registry_[index++].second).Restore(); + return; + } + } else { + auto it = std::partition_point( + backup_registry_.begin(), backup_registry_.end(), + [&](const auto& entry) { + return std::less<CommandLineFlag*>()(entry.first, &flag); + }); + if (it != backup_registry_.end() && it->first == &flag) { + std::move(*it->second).Restore(); + return; + } + } + flags_internal::PrivateHandleAccessor::RestoreToInitialState(flag); + }); } private: - std::vector<std::unique_ptr<flags_internal::FlagStateInterface>> + std::vector<std::pair<CommandLineFlag*, + std::unique_ptr<flags_internal::FlagStateInterface>>> backup_registry_; + bool saved_finalized_ = false; }; } // namespace flags_internal