Roll back skip initializing, locking, and cloning unmodified flags in FlagSaver. PiperOrigin-RevId: 1009139508
diff --git a/absl/flags/internal/flag.cc b/absl/flags/internal/flag.cc index 662b342..0a1dff2 100644 --- a/absl/flags/internal/flag.cc +++ b/absl/flags/internal/flag.cc
@@ -129,15 +129,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 +153,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_; @@ -445,35 +435,7 @@ cb(); } -// Returns true if an unmodified absl::Flag (i.e. `seq_lock_.IsUnmodified()`) -// can skip lazy initialization and state allocation in `SaveState()`, and -// instead reconstructs from the provided default value if modified before -// restoration. -bool FlagImpl::CanSkipInitOnSave() const { - if (!seq_lock_.IsUnmodified()) return false; - switch (ValueStorageKind()) { - case FlagValueStorageKind::kValueAndInitBit: - case FlagValueStorageKind::kOneWordAtomic: - // Only `kOneWord` defaults are inlinable compile-time constants; other - // kinds may have dynamic defaults or side effects. - return static_cast<FlagDefaultKind>(def_kind_) == - FlagDefaultKind::kOneWord; - case FlagValueStorageKind::kSequenceLocked: - // Always uses a default-generator function when unmodified. - return true; - 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 false; -} - std::unique_ptr<FlagStateInterface> FlagImpl::SaveState() { - if (ABSL_PREDICT_TRUE(CanSkipInitOnSave())) { - return std::make_unique<FlagState>(*this, default_value_); - } - absl::MutexLock l(DataGuard()); bool modified = modified_; @@ -507,10 +469,6 @@ } bool FlagImpl::RestoreState(const FlagState& flag_state) { - if (ABSL_PREDICT_TRUE(flag_state.counter_ < 0 && seq_lock_.IsUnmodified())) { - return false; - } - absl::MutexLock l(DataGuard()); if (flag_state.counter_ == ModificationCount()) { return false; @@ -519,22 +477,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; }
diff --git a/absl/flags/internal/flag.h b/absl/flags/internal/flag.h index 8baff43..8ef95ab 100644 --- a/absl/flags/internal/flag.h +++ b/absl/flags/internal/flag.h
@@ -284,8 +284,7 @@ } template <typename ValueT, typename GenT, - std::enable_if_t<std::is_arithmetic_v<ValueT>, int> = ((void)GenT{}, - 0)> + std::enable_if_t<std::is_integral_v<ValueT>, int> = ((void)GenT{}, 0)> constexpr FlagDefaultArg DefaultArg(int) { return {FlagDefaultSrc(GenT{}.value), FlagDefaultKind::kOneWord}; } @@ -711,7 +710,6 @@ ABSL_LOCKS_EXCLUDED(DataGuard()); int64_t ModificationCount() const ABSL_EXCLUSIVE_LOCKS_REQUIRED(DataGuard()); - bool CanSkipInitOnSave() const; // Interfaces to save and restore flags to/from persistent state. // Returns current flag state or nullptr if flag does not support
diff --git a/absl/flags/internal/sequence_lock.h b/absl/flags/internal/sequence_lock.h index 1729d7f..da04ff4 100644 --- a/absl/flags/internal/sequence_lock.h +++ b/absl/flags/internal/sequence_lock.h
@@ -66,12 +66,6 @@ lock_.store(0, std::memory_order_release); } - // Returns true if the flag is either uninitialized or initialized with zero - // modifications. - bool IsUnmodified() const { - return lock_.load(std::memory_order_acquire) <= 0; - } - // Copy "size" bytes of data from "src" to "dst", protected as a read-side // critical section of the sequence lock. //
diff --git a/absl/flags/reflection.cc b/absl/flags/reflection.cc index 13f210c..e7eeaa3 100644 --- a/absl/flags/reflection.cc +++ b/absl/flags/reflection.cc
@@ -191,7 +191,6 @@ if (registry.finalized_flags_.load(std::memory_order_acquire)) { for (const auto& i : registry.flat_flags_) visitor(*i); - return; } FlagRegistryLock frl(registry);
diff --git a/absl/flags/reflection_test.cc b/absl/flags/reflection_test.cc index 0fb902d..99a3525 100644 --- a/absl/flags/reflection_test.cc +++ b/absl/flags/reflection_test.cc
@@ -139,12 +139,6 @@ static int counter = 0; ABSL_FLAG(int, test_flag_13, 200, "").OnUpdate([]() { counter++; }); ABSL_FLAG(CustomUDT, test_flag_14, {}, ""); -static int runtime_bool_default_eval_count = 0; -static bool GetRuntimeBoolDefault() { - ++runtime_bool_default_eval_count; - return true; -} -ABSL_FLAG(bool, test_flag_15, GetRuntimeBoolDefault(), ""); namespace { @@ -276,16 +270,4 @@ EXPECT_EQ(absl::GetFlag(FLAGS_test_flag_08), 9876543); } -TEST_F(ReflectionTest, TestFlagSaverEagerlyInitsRuntimeDefaultIntegralFlag) { - { - absl::FlagSaver s; - const int evals_after_save = runtime_bool_default_eval_count; - EXPECT_GE(evals_after_save, 1); - absl::SetFlag(&FLAGS_test_flag_15, false); - EXPECT_EQ(runtime_bool_default_eval_count, evals_after_save); - EXPECT_FALSE(absl::GetFlag(FLAGS_test_flag_15)); - } - EXPECT_TRUE(absl::GetFlag(FLAGS_test_flag_15)); -} - } // namespace