Skip to content

Commit

Permalink
Core/Differ: detect and optimize reparenting
Browse files Browse the repository at this point in the history
Summary:
# Summary

In previous diffs earlier in 2020, we made changes to detect and optimize reordering of views when the order of views changed underneath the same parent.

However, until now we have ignored reparenting and there's evidence of issues because of that. Because Fabric flattens views more aggressively, reparenting is also marginally more likely to happen.

This diff introduces a very general Reparenting detection. It will work with view flattening/unflattening, as well as tree grafting - subtrees moved to entirely different parts of the tree, not just a single
parent disappearing or reappearing because of flattening/unflattening.

There is also another consideration: previously, we were generating strictly too many Create+Delete operations that were redundant and could cause consistency issues, crashes, or bugs on platforms that do not handle that gracefully -
especially since the ordering of the Create+Delete is not guaranteed (a reparented view could be created "first" and then the differ could later issue a "delete" for the same view).

Intuition behind how it works: we know the cases where we can detect reparenting: it's when nodes are *not* matched up with another node from the other tree, and we're either trying to delete an entire subtree, or create an entire subtree. For perf reasons, we generate whatever set of operations comes first (say, we generate all the Delete and Remove instructions) and take note in the `ReparentingMetadata` data-structure that Delete and/or Remove have been performed for each tag (if ordering is different, we do the same for Create+Insert if those come first). Then if we later detect a corresponding subtree creation/deletion, we don't generate those mutations and we mark the previous mutations for deletion. This incurs some map lookup cost, but this is only wasteful for commits where a large tree is deleted and a large tree is created, without reparenting.

We may be able to improve perf further for certain edge-cases in the future.

# Why can't we solve this in JS?

Two things:

1. We certainly can avoid reparenting situations in JS, but it's trickier than before because of Fabric's view flattening logic - product engineers would have to think much harder about how to prevent reparenting in the general case.
2. In the case of specific views like BottomSheet that may crash if they're reparented, the solution is to make sure that the BottomSheet and the first child of the BottomSheet is never memoized, so that lifecycle functions and render are called more often; and that in every render, the BottomSheet manually clones its child, so that when the Views are recreated, the child of the BottomSheet has a tag and is an entirely different instance. This is certainly possible to do but feels like an onerous requirement for product teams, and it could be challenging to track down every specific BottomSheet that is memoized and/or hoist them higher in the view hierarchy so they're not reparented as often.

Reviewed By: shergin

Differential Revision: D23123575

fbshipit-source-id: 2fa7e1f026f87b6f0c60cad469a3ba85cdc234de
  • Loading branch information
JoshuaGross authored and facebook-github-bot committed Aug 16, 2020
1 parent 4720ad9 commit 1e4d8d9
Show file tree
Hide file tree
Showing 10 changed files with 852 additions and 97 deletions.
417 changes: 404 additions & 13 deletions ReactCommon/react/renderer/mounting/Differentiator.cpp

Large diffs are not rendered by default.

3 changes: 2 additions & 1 deletion ReactCommon/react/renderer/mounting/Differentiator.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ enum class DifferentiatorMode { Classic, OptimizedMoves };
*/
ShadowViewMutationList calculateShadowViewMutations(
ShadowNode const &oldRootShadowNode,
ShadowNode const &newRootShadowNode);
ShadowNode const &newRootShadowNode,
bool enableReparentingDetection = false);

/*
* Generates a list of `ShadowViewNodePair`s that represents a layer of a
Expand Down
10 changes: 7 additions & 3 deletions ReactCommon/react/renderer/mounting/MountingCoordinator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,13 @@ namespace react {

MountingCoordinator::MountingCoordinator(
ShadowTreeRevision baseRevision,
std::weak_ptr<MountingOverrideDelegate const> delegate)
std::weak_ptr<MountingOverrideDelegate const> delegate,
bool enableReparentingDetection)
: surfaceId_(baseRevision.getRootShadowNode().getSurfaceId()),
baseRevision_(baseRevision),
mountingOverrideDelegate_(delegate),
telemetryController_(*this) {
telemetryController_(*this),
enableReparentingDetection_(enableReparentingDetection) {
#ifdef RN_SHADOW_TREE_INTROSPECTION
stubViewTree_ = stubViewTreeFromShadowNode(baseRevision_.getRootShadowNode());
#endif
Expand Down Expand Up @@ -139,7 +141,9 @@ better::optional<MountingTransaction> MountingCoordinator::pullTransaction()
telemetry.willDiff();
if (lastRevision_.hasValue()) {
diffMutations = calculateShadowViewMutations(
baseRevision_.getRootShadowNode(), lastRevision_->getRootShadowNode());
baseRevision_.getRootShadowNode(),
lastRevision_->getRootShadowNode(),
enableReparentingDetection_);
}
telemetry.didDiff();

Expand Down
5 changes: 4 additions & 1 deletion ReactCommon/react/renderer/mounting/MountingCoordinator.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ class MountingCoordinator final {
*/
MountingCoordinator(
ShadowTreeRevision baseRevision,
std::weak_ptr<MountingOverrideDelegate const> delegate);
std::weak_ptr<MountingOverrideDelegate const> delegate,
bool enableReparentingDetection = false);

/*
* Returns the id of the surface that the coordinator belongs to.
Expand Down Expand Up @@ -109,6 +110,8 @@ class MountingCoordinator final {

TelemetryController telemetryController_;

bool enableReparentingDetection_{false}; // temporary

#ifdef RN_SHADOW_TREE_INTROSPECTION
void validateTransactionAgainstStubViewTree(
ShadowViewMutationList const &mutations,
Expand Down
11 changes: 8 additions & 3 deletions ReactCommon/react/renderer/mounting/ShadowTree.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,11 @@ ShadowTree::ShadowTree(
LayoutContext const &layoutContext,
RootComponentDescriptor const &rootComponentDescriptor,
ShadowTreeDelegate const &delegate,
std::weak_ptr<MountingOverrideDelegate const> mountingOverrideDelegate)
: surfaceId_(surfaceId), delegate_(delegate) {
std::weak_ptr<MountingOverrideDelegate const> mountingOverrideDelegate,
bool enableReparentingDetection)
: surfaceId_(surfaceId),
delegate_(delegate),
enableReparentingDetection_(enableReparentingDetection) {
const auto noopEventEmitter = std::make_shared<const ViewEventEmitter>(
nullptr, -1, std::shared_ptr<const EventDispatcher>());

Expand All @@ -241,7 +244,9 @@ ShadowTree::ShadowTree(
family));

mountingCoordinator_ = std::make_shared<MountingCoordinator const>(
ShadowTreeRevision{rootShadowNode_, 0, {}}, mountingOverrideDelegate);
ShadowTreeRevision{rootShadowNode_, 0, {}},
mountingOverrideDelegate,
enableReparentingDetection);
}

ShadowTree::~ShadowTree() {
Expand Down
7 changes: 6 additions & 1 deletion ReactCommon/react/renderer/mounting/ShadowTree.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@ class ShadowTree final {
LayoutContext const &layoutContext,
RootComponentDescriptor const &rootComponentDescriptor,
ShadowTreeDelegate const &delegate,
std::weak_ptr<MountingOverrideDelegate const> mountingOverrideDelegate);
std::weak_ptr<MountingOverrideDelegate const> mountingOverrideDelegate,
bool enableReparentingDetection = false);

~ShadowTree();

Expand Down Expand Up @@ -87,6 +88,9 @@ class ShadowTree final {
void setEnableNewStateReconciliation(bool value) {
enableNewStateReconciliation_ = value;
}
void setEnableReparentingDetection(bool value) {
enableReparentingDetection_ = value;
}

private:
RootShadowNode::Unshared cloneRootShadowNode(
Expand All @@ -106,6 +110,7 @@ class ShadowTree final {
0}; // Protected by `commitMutex_`.
MountingCoordinator::Shared mountingCoordinator_;
bool enableNewStateReconciliation_{false};
bool enableReparentingDetection_{false};
};

} // namespace react
Expand Down
2 changes: 1 addition & 1 deletion ReactCommon/react/renderer/mounting/ShadowViewMutation.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ struct ShadowViewMutation final {

#pragma mark - Type

enum Type { Create, Delete, Insert, Remove, Update };
enum Type { Create = 1, Delete = 2, Insert = 4, Remove = 8, Update = 16 };

#pragma mark - Fields

Expand Down
Loading

1 comment on commit 1e4d8d9

@sunnylqm
Copy link
Contributor

@sunnylqm sunnylqm commented on 1e4d8d9 Aug 17, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoshuaGross Could this kind of crash #17530 be related to the diff strategy described here?

Please sign in to comment.