Skip to content

Destroy preallocated views that the pull model never mounted (#58890) #58890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
bartlomiejbloniarz wants to merge 1 commit into
base: main
Choose a base branch
from
+404 −78
Open

Destroy preallocated views that the pull model never mounted (#58890) #58890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
100 changes: 71 additions & 29 deletions packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,11 @@ FabricMountingManager::~FabricMountingManager() {
void FabricMountingManager::onSurfaceStart(SurfaceId surfaceId) {
std::lock_guard lock(allocatedViewsMutex_);
allocatedViewRegistry_.emplace(
surfaceId, std::unordered_map<Tag, Props::Shared>{{surfaceId, nullptr}});
surfaceId,
std::unordered_map<Tag, AllocatedView>{
{surfaceId,
{.preallocatedProps = nullptr,
.state = AllocatedViewState::Mounted}}});
}

void FabricMountingManager::onSurfaceStop(SurfaceId surfaceId) {
Expand Down Expand Up @@ -615,9 +619,9 @@ void FabricMountingManager::executeMount(
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);

auto allocatedViewsIterator = allocatedViewRegistry_.find(surfaceId);
auto defaultAllocatedViews = std::unordered_map<Tag, Props::Shared>{};
auto defaultAllocatedViews = std::unordered_map<Tag, AllocatedView>{};
// Do not remove `defaultAllocatedViews` or initialize
// `std::unordered_map<Tag, Props::Shared>{}` inline in below ternary
// `std::unordered_map<Tag, AllocatedView>{}` inline in below ternary
// expression - if falsy operand is a value type, the compiler will decide
// the expression to be a value type, an unnecessary (sometimes expensive)
// copy will happen as a result.
Expand All @@ -640,13 +644,18 @@ void FabricMountingManager::executeMount(
bool isVirtual = mutation.mutatedViewIsVirtual();
switch (mutationType) {
case ShadowViewMutation::Create: {
bool shouldCreateView =
!allocatedViewTags.contains(newChildShadowView.tag);
auto [allocatedView, shouldCreateView] =
allocatedViewTags.try_emplace(
newChildShadowView.tag,
AllocatedView{
.preallocatedProps = nullptr,
.state = AllocatedViewState::Mounted});

if (shouldCreateView) {
cppCommonMountItems.push_back(
CppMountItem::CreateMountItem(newChildShadowView));
allocatedViewTags.emplace(newChildShadowView.tag, nullptr);
} else {
allocatedView->second.state = AllocatedViewState::Mounted;
}
break;
}
Expand Down Expand Up @@ -755,7 +764,8 @@ void FabricMountingManager::executeMount(
.push_back(
CppMountItem::UpdatePropsMountItem(
{}, newChildShadowView));
} else if (auto& preallocatedProps = allocatedView->second;
} else if (auto& preallocatedProps =
allocatedView->second.preallocatedProps;
preallocatedProps != nullptr) {
if (preallocatedProps != newChildShadowView.props) {
auto preallocatedShadowView = newChildShadowView;
Expand Down Expand Up @@ -1056,22 +1066,55 @@ void FabricMountingManager::executeMount(
}

void FabricMountingManager::drainPreallocateViewsQueue() {
std::vector<QueuedView> queuedViews;
std::vector<ShadowView> shadowViews;

{
std::lock_guard lock(preallocateMutex_);
std::swap(shadowViews, preallocatedViewsQueue_);
std::swap(queuedViews, preallocatedViewsQueue_);
}

bool pullModel =
ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid();
{
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);
for (auto& [shadowView, family] : queuedViews) {
if (pullModel && family.expired()) {
continue;
}
auto allocatedViewsIterator =
allocatedViewRegistry_.find(shadowView.surfaceId);
if (allocatedViewsIterator != allocatedViewRegistry_.end() &&
allocatedViewsIterator->second
.emplace(
shadowView.tag,
AllocatedView{
.preallocatedProps =
shouldDiffInsertPropsAgainstPreallocatedProps()
? shadowView.props
: nullptr,
.state = AllocatedViewState::Preallocated})
.second) {
shadowViews.push_back(std::move(shadowView));
}
}
}

for (const auto& shadowView : shadowViews) {
preallocateShadowView(shadowView);
preallocateRegisteredView(shadowView);
}
}

void FabricMountingManager::destroyUnmountedShadowNode(
const ShadowNodeFamily& family) {
auto tag = family.getTag();
auto surfaceId = family.getSurfaceId();
bool pullModel =
ReactNativeFeatureFlags::enableMountingCoordinatorPullModelAndroid();

if (!pullModel && family.hasBeenMounted()) {
return;
}

// Remove from allocatedViewRegistry so that executeMount does not skip
// the Create mount item for this tag. Without this, if the view was
Expand All @@ -1082,7 +1125,20 @@ void FabricMountingManager::destroyUnmountedShadowNode(
{
std::lock_guard allocatedViewsLock(allocatedViewsMutex_);
auto allocatedViewsIterator = allocatedViewRegistry_.find(surfaceId);
if (allocatedViewsIterator != allocatedViewRegistry_.end()) {
if (pullModel) {
// In the pull model, a committed family can have a preallocated view that
// no mount created (its Create and Delete were never pulled), and a
// mounted view's family can be destroyed before its Delete is pulled.
if (allocatedViewsIterator == allocatedViewRegistry_.end()) {
return;
}
auto allocatedView = allocatedViewsIterator->second.find(tag);
if (allocatedView == allocatedViewsIterator->second.end() ||
allocatedView->second.state != AllocatedViewState::Preallocated) {
return;
}
allocatedViewsIterator->second.erase(allocatedView);
} else if (allocatedViewsIterator != allocatedViewRegistry_.end()) {
allocatedViewsIterator->second.erase(tag);
}
}
Expand Down Expand Up @@ -1113,29 +1169,15 @@ void FabricMountingManager::maybePreallocateShadowNode(

{
std::lock_guard lock(preallocateMutex_);
preallocatedViewsQueue_.push_back(std::move(shadowView));
preallocatedViewsQueue_.push_back(
{.shadowView = std::move(shadowView),
.family = shadowNode.getFamilyShared()});
}
}

void FabricMountingManager::preallocateShadowView(
void FabricMountingManager::preallocateRegisteredView(
const ShadowView& shadowView) {
TraceSection section("FabricMountingManager::preallocateShadowView");

{
std::lock_guard lock(allocatedViewsMutex_);
auto allocatedViewsIterator =
allocatedViewRegistry_.find(shadowView.surfaceId);
if (allocatedViewsIterator == allocatedViewRegistry_.end()) {
return;
}
const auto [_, inserted] = allocatedViewsIterator->second.emplace(
shadowView.tag,
shouldDiffInsertPropsAgainstPreallocatedProps() ? shadowView.props
: nullptr);
if (!inserted) {
return;
}
}
TraceSection section("FabricMountingManager::preallocateRegisteredView");

bool isLayoutableShadowNode = shadowView.layoutMetrics != EmptyLayoutMetrics;

Expand Down
43 changes: 26 additions & 17 deletions packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricMountingManager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,19 +7,20 @@

#pragma once

#include <memory>
#include <mutex>
#include <unordered_map>

#include <React/RendererCore.h>
#include <fbjni/fbjni.h>
#include <react/fabric/JFabricUIManager.h>
#include <react/renderer/mounting/ShadowView.h>

namespace facebook::react {

struct AnimatedProps;

class MountingTransaction;
struct ShadowView;

class FabricMountingManager final {
public:
Expand All @@ -36,18 +37,13 @@ class FabricMountingManager final {
void destroyUnmountedShadowNode(const ShadowNodeFamily &family);

/*
* Drains preallocatedViewsQueue_ by calling preallocateShadowView on each
* item in the queue. Can be called by any thread.
* Drains preallocatedViewsQueue_: preallocates each queued view on the Java
* side and registers its tag in allocatedViewRegistry_ so that executeMount
* skips the redundant Create mount item for this tag. In the pull model, it
* skips the views whose family was destroyed. Can be called by any thread.
*/
void drainPreallocateViewsQueue();

/*
* Preallocates a view on the Java side and registers the tag in
* allocatedViewRegistry_ so that executeMount skips the redundant Create
* mount item for this tag.
*/
void preallocateShadowView(const ShadowView &shadowView);

/*
* Returns true if the given tag is registered in allocatedViewRegistry_
* for the given surface. A registered tag means executeMount will skip
Expand Down Expand Up @@ -98,6 +94,8 @@ class FabricMountingManager final {
private:
bool isOnMainThread();

void preallocateRegisteredView(const ShadowView &shadowView);

jni::global_ref<JFabricUIManager::javaobject> javaUIManager_;

std::recursive_mutex commitMutex_;
Expand All @@ -107,17 +105,28 @@ class FabricMountingManager final {
*/
std::mutex preallocateMutex_;

struct QueuedView {
ShadowView shadowView;
std::weak_ptr<const ShadowNodeFamily> family;
};

/*
* A queue of views to be preallocated on the Java side.
*/
std::vector<ShadowView> preallocatedViewsQueue_{};
std::vector<QueuedView> preallocatedViewsQueue_{};

/*
* Allocated tags per surface. With enablePreallocatedPropsDiffOnInsertAndroid
* a preallocated tag maps to the props it was preallocated with until its
* first Insert; every other tag maps to nullptr.
*/
std::unordered_map<SurfaceId, std::unordered_map<Tag, Props::Shared>> allocatedViewRegistry_{};
enum class AllocatedViewState { Preallocated, Mounted };

struct AllocatedView {
/*
* With enablePreallocatedPropsDiffOnInsertAndroid, the props the view was
* preallocated with, until its first Insert. nullptr otherwise.
*/
Props::Shared preallocatedProps;
AllocatedViewState state;
};

std::unordered_map<SurfaceId, std::unordered_map<Tag, AllocatedView>> allocatedViewRegistry_{};
std::recursive_mutex allocatedViewsMutex_;
};

Expand Down
8 changes: 4 additions & 4 deletions packages/react-native/ReactAndroid/src/main/jni/react/fabric/FabricUIManagerBinding.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -807,11 +807,11 @@ void FabricUIManagerBinding::schedulerDidRequestPreliminaryViewAllocation(
return;
}
mountingManager->maybePreallocateShadowNode(shadowNode);
// Only the Views of ShadowNode that were pre-allocated (forms views) needs
// to be destroyed if the ShadowNode is destroyed but it was never mounted
// on the screen.
// Only the Views of ShadowNode that were pre-allocated (forms views) need
// to be destroyed if the ShadowNode is destroyed and no mount created its
// View.
if (shadowNode.getTraits().check(ShadowNodeTraits::Trait::FormsView)) {
shadowNode.getFamilyShared()->onUnmountedFamilyDestroyed(
shadowNode.getFamilyShared()->onFamilyDestroyed(
[weakMountingManager =
std::weak_ptr(mountingManager)](const ShadowNodeFamily& family) {
if (auto mountingManager = weakMountingManager.lock()) {
Expand Down
13 changes: 6 additions & 7 deletions ...es/react-native/ReactAndroid/src/main/jni/react/fabric/test/FabricMountingManagerTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,10 +60,9 @@ class FabricMountingManagerTest : public ::testing::Test {
* and to short-circuit a redundant Create mount item for the root view.
*
* Bug this catches: if a refactor stops registering the root surface tag
* (e.g. switches to `unordered_set<Tag>{}` instead of
* `unordered_set<Tag>({surfaceId})`), the root view would be re-created on
* every commit. Without this assertion, that regression would only surface
* via end-to-end Java tests.
* (e.g. starts each surface with an empty registry), the root view would be
* re-created on every commit. Without this assertion, that regression would
* only surface via end-to-end Java tests.
*/
TEST_F(
FabricMountingManagerTest,
Expand All @@ -79,7 +78,7 @@ TEST_F(
/*
* Verifies that `isViewAllocated` returns false for a surface that has not
* been started. The contract is "false when the surface is unknown", not
* "throw" or "abort"; `executeMount` and `preallocateShadowView` both rely
* "throw" or "abort"; `executeMount` and `drainPreallocateViewsQueue` both rely
* on this to gracefully no-op when a surface has been stopped concurrently.
*/
TEST_F(
Expand All @@ -98,7 +97,7 @@ TEST_F(
* must return false for *any* tag on that surface, including the surface
* tag itself (which `onSurfaceStart` had inserted).
*
* Bug this catches: if `onSurfaceStop` only `.clear()`-ed the inner set
* Bug this catches: if `onSurfaceStop` only `.clear()`-ed the inner map
* instead of erasing the outer map entry, `executeMount`'s "surface was
* stopped" log would never fire, masking commit-after-stop bugs.
*/
Expand All @@ -122,7 +121,7 @@ TEST_F(
* the same process.
*
* Bug this catches: if a future change accidentally shared the
* `unordered_set<Tag>` across surfaces (e.g. via a misplaced static), tags
* per-surface registry across surfaces (e.g. via a misplaced static), tags
* from one surface would falsely appear as allocated on another, and
* stopping any surface would purge unrelated surfaces' registries.
*/
Expand Down
Loading
Loading