From b8f3478b370f408ce7471a4ecb4e5cf5a0439464 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rub=C3=A9n=20Norte?= Date: Wed, 22 May 2024 10:26:55 -0700 Subject: [PATCH] Implement re-entrancy and unlocked scenarios for LazyShadowTreeRevisionConsistencyManager (#44620) Summary: Pull Request resolved: https://github.com/facebook/react-native/pull/44620 We added a log message when trying to lock revisions in `LazyShadowTreeRevisionConsistencyManager` when they were already locked, and we've seen that being logged in existing experiments, which could indicate we're doing re-entrance from the JS runtime. This protects against that case migrating the boolean flag to an integer. Changelog: [internal] Reviewed By: NickGerleman Differential Revision: D57509193 fbshipit-source-id: 1712aa84d665c9dfe50630818e7f56de7d7e145c --- ...zyShadowTreeRevisionConsistencyManager.cpp | 44 +++++++------ ...LazyShadowTreeRevisionConsistencyManager.h | 3 +- ...adowTreeRevisionConsistencyManagerTest.cpp | 62 +++++++++++++++++++ 3 files changed, 89 insertions(+), 20 deletions(-) diff --git a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.cpp b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.cpp index b3f98e26f50..85cd9cd5158 100644 --- a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.cpp +++ b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.cpp @@ -19,7 +19,13 @@ void LazyShadowTreeRevisionConsistencyManager::updateCurrentRevision( SurfaceId surfaceId, RootShadowNode::Shared rootShadowNode) { std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); - capturedRootShadowNodesForConsistency_[surfaceId] = std::move(rootShadowNode); + + // We don't need to store the revision if we haven't locked. + // We can resolve lazily when requested. + if (lockCount > 0) { + capturedRootShadowNodesForConsistency_[surfaceId] = + std::move(rootShadowNode); + } } #pragma mark - ShadowTreeRevisionProvider @@ -29,10 +35,11 @@ LazyShadowTreeRevisionConsistencyManager::getCurrentRevision( SurfaceId surfaceId) { { std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); - - auto it = capturedRootShadowNodesForConsistency_.find(surfaceId); - if (it != capturedRootShadowNodesForConsistency_.end()) { - return it->second; + if (lockCount > 0) { + auto it = capturedRootShadowNodesForConsistency_.find(surfaceId); + if (it != capturedRootShadowNodesForConsistency_.end()) { + return it->second; + } } } @@ -47,7 +54,9 @@ LazyShadowTreeRevisionConsistencyManager::getCurrentRevision( { std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); - capturedRootShadowNodesForConsistency_[surfaceId] = rootShadowNode; + if (lockCount > 0) { + capturedRootShadowNodesForConsistency_[surfaceId] = rootShadowNode; + } } return rootShadowNode; @@ -56,29 +65,26 @@ LazyShadowTreeRevisionConsistencyManager::getCurrentRevision( #pragma mark - ConsistentShadowTreeRevisionProvider void LazyShadowTreeRevisionConsistencyManager::lockRevisions() { - if (isLocked_) { - LOG(WARNING) - << "LazyShadowTreeRevisionConsistencyManager::lockRevisions() called without unlocking a previous lock"; - return; - } + std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); // We actually capture the state lazily the first time we access it, so we // don't need to do anything here. - isLocked_ = true; + lockCount++; } void LazyShadowTreeRevisionConsistencyManager::unlockRevisions() { - if (!isLocked_) { + std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); + + if (lockCount == 0) { LOG(WARNING) << "LazyShadowTreeRevisionConsistencyManager::unlockRevisions() called without a previous lock"; - // We don't return here because we want to do the cleanup anyway - // to free up resources. + } else { + lockCount--; } - isLocked_ = false; - - std::unique_lock lock(capturedRootShadowNodesForConsistencyMutex_); - capturedRootShadowNodesForConsistency_.clear(); + if (lockCount == 0) { + capturedRootShadowNodesForConsistency_.clear(); + } } } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.h b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.h index 3de32c12e84..527e6bde1d3 100644 --- a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.h +++ b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/LazyShadowTreeRevisionConsistencyManager.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -47,7 +48,7 @@ class LazyShadowTreeRevisionConsistencyManager std::unordered_map capturedRootShadowNodesForConsistency_; ShadowTreeRegistry& shadowTreeRegistry_; - bool isLocked_{false}; + uint_fast32_t lockCount{0}; }; } // namespace facebook::react diff --git a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/tests/LazyShadowTreeRevisionConsistencyManagerTest.cpp b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/tests/LazyShadowTreeRevisionConsistencyManagerTest.cpp index fac644047c4..0cd1fe194fb 100644 --- a/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/tests/LazyShadowTreeRevisionConsistencyManagerTest.cpp +++ b/packages/react-native/ReactCommon/react/renderer/uimanager/consistency/tests/LazyShadowTreeRevisionConsistencyManagerTest.cpp @@ -93,6 +93,27 @@ TEST_F(LazyShadowTreeRevisionConsistencyManagerTest, testLockedOnNoRevision) { consistencyManager_.unlockRevisions(); } +TEST_F(LazyShadowTreeRevisionConsistencyManagerTest, testNotLocked) { + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), nullptr); + + shadowTreeRegistry_.add(createShadowTree(0)); + + auto element = Element(); + auto builder = simpleComponentBuilder(); + auto newRootShadowNode = builder.build(element); + + shadowTreeRegistry_.visit( + 0, [newRootShadowNode](const ShadowTree& shadowTree) { + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return newRootShadowNode; + }, + {}); + }); + + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), newRootShadowNode); +} + TEST_F( LazyShadowTreeRevisionConsistencyManagerTest, testLockedOnNoRevisionWithUpdate) { @@ -330,4 +351,45 @@ TEST_F(LazyShadowTreeRevisionConsistencyManagerTest, testUpdateToUnmounted) { consistencyManager_.unlockRevisions(); } +TEST_F(LazyShadowTreeRevisionConsistencyManagerTest, testReentrance) { + consistencyManager_.lockRevisions(); + + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), nullptr); + + shadowTreeRegistry_.add(createShadowTree(0)); + + auto element = Element(); + auto builder = simpleComponentBuilder(); + auto newRootShadowNode = builder.build(element); + + shadowTreeRegistry_.visit( + 0, [newRootShadowNode](const ShadowTree& shadowTree) { + shadowTree.commit( + [&](const RootShadowNode& /*oldRootShadowNode*/) { + return newRootShadowNode; + }, + {}); + }); + + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), nullptr); + + // Re-entrance + consistencyManager_.lockRevisions(); + + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), nullptr); + + // Exit second lock + consistencyManager_.unlockRevisions(); + + EXPECT_EQ(consistencyManager_.getCurrentRevision(0), nullptr); + + // Exit first lock + + consistencyManager_.unlockRevisions(); + + // Updated! + EXPECT_EQ( + consistencyManager_.getCurrentRevision(0).get(), newRootShadowNode.get()); +} + } // namespace facebook::react