From fe69f71f0a89c6626dbc6233b41e0b55389a5c21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Mon, 5 Feb 2024 06:41:50 -0800 Subject: [PATCH] Improve thread safety of ReactNativeFeatureFlags (#42870) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/42870 This refactors `ReactNativeFeatureFlagsAccessor` in C++ to improve its thread-safety: * It makes all cached feature flags atomic to prevent data corruption when writing them concurrently. * It refactors the list of accessed feature flags to be an array of atomic character pointers instead of a vector. Performance-wise, this is lock-free so it would still be fast enough for our use cases. Semantic-wise, this implementation could lead to feature flags being initialized more than once (if 2 threads happen to access the same feature flag before it has been initialized), but that's ok. The only consequence of this would be accessing the provider twice, but the end state of the accessor is the same (the same value would be cached and the flag would still be marked as accessed). Changelog: [internal] Reviewed By: javache Differential Revision: D53406924 fbshipit-source-id: 1023673c40f9da43a51c5f96354d4c458c9d14d4 --- .../featureflags/ReactNativeFeatureFlags.h | 5 +- .../ReactNativeFeatureFlagsAccessor.cpp | 183 +++++++++++------- .../ReactNativeFeatureFlagsAccessor.h | 25 ++- .../ReactNativeFeatureFlags.h-template.js | 3 +- ...NativeFeatureFlagsAccessor.cpp-template.js | 57 ++++-- ...ctNativeFeatureFlagsAccessor.h-template.js | 13 +- 6 files changed, 176 insertions(+), 110 deletions(-) diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h index 34d88798b5abff..af9c3fa5270e99 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlags.h @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<> + * @generated SignedSource<<252f46e2c95e08d1180f656c870087b7>> */ /** @@ -28,7 +28,8 @@ namespace facebook::react { /** * This class provides access to internal React Native feature flags. * - * All the methods are thread-safe if you handle `override` correctly. + * All the methods are thread-safe (as long as the methods in the overridden + * provider are). */ class ReactNativeFeatureFlags { public: diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.cpp b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.cpp index ab62bb34960605..16c358b2ad12d1 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.cpp +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.cpp @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<7d50752c48503e31eccfe64d66ee2e99>> + * @generated SignedSource<> */ /** @@ -29,143 +29,178 @@ ReactNativeFeatureFlagsAccessor::ReactNativeFeatureFlagsAccessor() : currentProvider_(std::make_unique()) {} bool ReactNativeFeatureFlagsAccessor::commonTestFlag() { - if (!commonTestFlag_.has_value()) { + auto flagValue = commonTestFlag_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "commonTestFlag"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(0, flagName); - commonTestFlag_.emplace(currentProvider_->commonTestFlag()); + flagValue = currentProvider_->commonTestFlag(); + commonTestFlag_ = flagValue; } - return commonTestFlag_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::useModernRuntimeScheduler() { - if (!useModernRuntimeScheduler_.has_value()) { + auto flagValue = useModernRuntimeScheduler_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "useModernRuntimeScheduler"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(1, flagName); - useModernRuntimeScheduler_.emplace(currentProvider_->useModernRuntimeScheduler()); + flagValue = currentProvider_->useModernRuntimeScheduler(); + useModernRuntimeScheduler_ = flagValue; } - return useModernRuntimeScheduler_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::enableMicrotasks() { - if (!enableMicrotasks_.has_value()) { + auto flagValue = enableMicrotasks_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "enableMicrotasks"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(2, flagName); - enableMicrotasks_.emplace(currentProvider_->enableMicrotasks()); + flagValue = currentProvider_->enableMicrotasks(); + enableMicrotasks_ = flagValue; } - return enableMicrotasks_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::batchRenderingUpdatesInEventLoop() { - if (!batchRenderingUpdatesInEventLoop_.has_value()) { + auto flagValue = batchRenderingUpdatesInEventLoop_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "batchRenderingUpdatesInEventLoop"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(3, flagName); - batchRenderingUpdatesInEventLoop_.emplace(currentProvider_->batchRenderingUpdatesInEventLoop()); + flagValue = currentProvider_->batchRenderingUpdatesInEventLoop(); + batchRenderingUpdatesInEventLoop_ = flagValue; } - return batchRenderingUpdatesInEventLoop_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::enableSpannableBuildingUnification() { - if (!enableSpannableBuildingUnification_.has_value()) { + auto flagValue = enableSpannableBuildingUnification_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "enableSpannableBuildingUnification"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(4, flagName); - enableSpannableBuildingUnification_.emplace(currentProvider_->enableSpannableBuildingUnification()); + flagValue = currentProvider_->enableSpannableBuildingUnification(); + enableSpannableBuildingUnification_ = flagValue; } - return enableSpannableBuildingUnification_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::enableCustomDrawOrderFabric() { - if (!enableCustomDrawOrderFabric_.has_value()) { + auto flagValue = enableCustomDrawOrderFabric_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "enableCustomDrawOrderFabric"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(5, flagName); - enableCustomDrawOrderFabric_.emplace(currentProvider_->enableCustomDrawOrderFabric()); + flagValue = currentProvider_->enableCustomDrawOrderFabric(); + enableCustomDrawOrderFabric_ = flagValue; } - return enableCustomDrawOrderFabric_.value(); + return flagValue.value(); } bool ReactNativeFeatureFlagsAccessor::enableFixForClippedSubviewsCrash() { - if (!enableFixForClippedSubviewsCrash_.has_value()) { + auto flagValue = enableFixForClippedSubviewsCrash_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "enableFixForClippedSubviewsCrash"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(6, flagName); - enableFixForClippedSubviewsCrash_.emplace(currentProvider_->enableFixForClippedSubviewsCrash()); + flagValue = currentProvider_->enableFixForClippedSubviewsCrash(); + enableFixForClippedSubviewsCrash_ = flagValue; } - return enableFixForClippedSubviewsCrash_.value(); + return flagValue.value(); } void ReactNativeFeatureFlagsAccessor::override( std::unique_ptr provider) { - if (!accessedFeatureFlags_.empty()) { - std::ostringstream featureFlagListBuilder; - for (const auto& featureFlagName : accessedFeatureFlags_) { + ensureFlagsNotAccessed(); + currentProvider_ = std::move(provider); +} + +void ReactNativeFeatureFlagsAccessor::markFlagAsAccessed( + int position, + const char* flagName) { + accessedFeatureFlags_[position] = flagName; +} + +void ReactNativeFeatureFlagsAccessor::ensureFlagsNotAccessed() { + std::string accessedFeatureFlagNames; + + std::ostringstream featureFlagListBuilder; + for (const auto& featureFlagName : accessedFeatureFlags_) { + if (featureFlagName != nullptr) { featureFlagListBuilder << featureFlagName << ", "; } - std::string accessedFeatureFlagNames = featureFlagListBuilder.str(); - if (!accessedFeatureFlagNames.empty()) { - accessedFeatureFlagNames = accessedFeatureFlagNames.substr( - 0, accessedFeatureFlagNames.size() - 2); - } + } + accessedFeatureFlagNames = featureFlagListBuilder.str(); + if (!accessedFeatureFlagNames.empty()) { + accessedFeatureFlagNames = + accessedFeatureFlagNames.substr(0, accessedFeatureFlagNames.size() - 2); + } + + if (!accessedFeatureFlagNames.empty()) { throw std::runtime_error( "Feature flags were accessed before being overridden: " + accessedFeatureFlagNames); } - - currentProvider_ = std::move(provider); } } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.h b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.h index bd3fbaee40fec3..48d7c4f43e4c29 100644 --- a/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.h +++ b/packages/react-native/ReactCommon/react/featureflags/ReactNativeFeatureFlagsAccessor.h @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<1514c04fb9d175308e106b84a14bc89d>> + * @generated SignedSource<<40315ae6cc418effd421dff2be6831c7>> */ /** @@ -20,6 +20,8 @@ #pragma once #include +#include +#include #include #include #include @@ -41,16 +43,19 @@ class ReactNativeFeatureFlagsAccessor { void override(std::unique_ptr provider); private: + void markFlagAsAccessed(int position, const char* flagName); + void ensureFlagsNotAccessed(); + std::unique_ptr currentProvider_; - std::vector accessedFeatureFlags_; - - std::optional commonTestFlag_; - std::optional useModernRuntimeScheduler_; - std::optional enableMicrotasks_; - std::optional batchRenderingUpdatesInEventLoop_; - std::optional enableSpannableBuildingUnification_; - std::optional enableCustomDrawOrderFabric_; - std::optional enableFixForClippedSubviewsCrash_; + std::array, 7> accessedFeatureFlags_; + + std::atomic> commonTestFlag_; + std::atomic> useModernRuntimeScheduler_; + std::atomic> enableMicrotasks_; + std::atomic> batchRenderingUpdatesInEventLoop_; + std::atomic> enableSpannableBuildingUnification_; + std::atomic> enableCustomDrawOrderFabric_; + std::atomic> enableFixForClippedSubviewsCrash_; }; } // namespace facebook::react diff --git a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlags.h-template.js b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlags.h-template.js index 829f61822a4254..ed4e5faf47ec75 100644 --- a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlags.h-template.js +++ b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlags.h-template.js @@ -38,7 +38,8 @@ namespace facebook::react { /** * This class provides access to internal React Native feature flags. * - * All the methods are thread-safe if you handle \`override\` correctly. + * All the methods are thread-safe (as long as the methods in the overridden + * provider are). */ class ReactNativeFeatureFlags { public: diff --git a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.cpp-template.js b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.cpp-template.js index 0c83a6b546045a..de6e313bc9292d 100644 --- a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.cpp-template.js +++ b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.cpp-template.js @@ -40,47 +40,64 @@ ReactNativeFeatureFlagsAccessor::ReactNativeFeatureFlagsAccessor() ${Object.entries(config.common) .map( - ([flagName, flagConfig]) => + ([flagName, flagConfig], flagPosition) => `${getCxxTypeFromDefaultValue( flagConfig.defaultValue, )} ReactNativeFeatureFlagsAccessor::${flagName}() { - if (!${flagName}_.has_value()) { + auto flagValue = ${flagName}_.load(); + + if (!flagValue.has_value()) { + // This block is not exclusive but it is not necessary. + // If multiple threads try to initialize the feature flag, we would only + // be accessing the provider multiple times but the end state of this + // instance and the returned flag value would be the same. + // Mark the flag as accessed. static const char* flagName = "${flagName}"; - if (std::find( - accessedFeatureFlags_.begin(), - accessedFeatureFlags_.end(), - flagName) == accessedFeatureFlags_.end()) { - accessedFeatureFlags_.push_back(flagName); - } + markFlagAsAccessed(${flagPosition}, flagName); - ${flagName}_.emplace(currentProvider_->${flagName}()); + flagValue = currentProvider_->${flagName}(); + ${flagName}_ = flagValue; } - return ${flagName}_.value(); + return flagValue.value(); }`, ) .join('\n\n')} void ReactNativeFeatureFlagsAccessor::override( std::unique_ptr provider) { - if (!accessedFeatureFlags_.empty()) { - std::ostringstream featureFlagListBuilder; - for (const auto& featureFlagName : accessedFeatureFlags_) { + ensureFlagsNotAccessed(); + currentProvider_ = std::move(provider); +} + +void ReactNativeFeatureFlagsAccessor::markFlagAsAccessed( + int position, + const char* flagName) { + accessedFeatureFlags_[position] = flagName; +} + +void ReactNativeFeatureFlagsAccessor::ensureFlagsNotAccessed() { + std::string accessedFeatureFlagNames; + + std::ostringstream featureFlagListBuilder; + for (const auto& featureFlagName : accessedFeatureFlags_) { + if (featureFlagName != nullptr) { featureFlagListBuilder << featureFlagName << ", "; } - std::string accessedFeatureFlagNames = featureFlagListBuilder.str(); - if (!accessedFeatureFlagNames.empty()) { - accessedFeatureFlagNames = accessedFeatureFlagNames.substr( - 0, accessedFeatureFlagNames.size() - 2); - } + } + accessedFeatureFlagNames = featureFlagListBuilder.str(); + if (!accessedFeatureFlagNames.empty()) { + accessedFeatureFlagNames = + accessedFeatureFlagNames.substr(0, accessedFeatureFlagNames.size() - 2); + } + + if (!accessedFeatureFlagNames.empty()) { throw std::runtime_error( "Feature flags were accessed before being overridden: " + accessedFeatureFlagNames); } - - currentProvider_ = std::move(provider); } } // namespace facebook::react diff --git a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.h-template.js b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.h-template.js index e2cd096586da22..5478b3b0fab538 100644 --- a/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.h-template.js +++ b/packages/react-native/scripts/featureflags/templates/common-cxx/ReactNativeFeatureFlagsAccessor.h-template.js @@ -30,6 +30,8 @@ ${DO_NOT_MODIFY_COMMENT} #pragma once #include +#include +#include #include #include #include @@ -50,15 +52,20 @@ ${Object.entries(config.common) void override(std::unique_ptr provider); private: + void markFlagAsAccessed(int position, const char* flagName); + void ensureFlagsNotAccessed(); + std::unique_ptr currentProvider_; - std::vector accessedFeatureFlags_; + std::array, ${ + Object.keys(config.common).length + }> accessedFeatureFlags_; ${Object.entries(config.common) .map( ([flagName, flagConfig]) => - ` std::optional<${getCxxTypeFromDefaultValue( + ` std::atomic ${flagName}_;`, + )}>> ${flagName}_;`, ) .join('\n')} };