Skip to content

Commit

Permalink
Back out "Pass mutation list to RCTMountingTransactionObserving callb…
Browse files Browse the repository at this point in the history
…acks"

Summary:
https://fb.workplace.com/groups/fbapp.commerce.engsupport/permalink/2074812256012212/

Back out "[react-native][PR] Pass mutation list to RCTMountingTransactionObserving callbacks"

Original commit changeset: f40afc512f2c

Original Phabricator Diff: D35214478 (91fc2c0)

Changelog: [Internal]

Reviewed By: cipolleschi

Differential Revision: D35825832

fbshipit-source-id: b53b616dca39c84b3a8e8e4cbaa4a45834e53fe3
  • Loading branch information
makovkastar authored and facebook-github-bot committed Apr 21, 2022
1 parent 476330a commit 2f5a1e6
Show file tree
Hide file tree
Showing 11 changed files with 84 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -192,8 +192,7 @@ - (void)ensurePresentedOnlyIfNeeded

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionWillMount:(MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry
- (void)mountingTransactionWillMountWithMetadata:(MountingTransactionMetadata const &)metadata
{
_modalContentsSnapshot = [self.viewController.view snapshotViewAfterScreenUpdates:NO];
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,8 +147,7 @@ - (void)dealloc

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionDidMount:(MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry
- (void)mountingTransactionDidMountWithMetadata:(MountingTransactionMetadata const &)metadata
{
[self _remountChildren];
}
Expand Down
4 changes: 2 additions & 2 deletions React/Fabric/Mounting/RCTComponentViewFactory.mm
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ - (RCTComponentViewClassDescriptor)_componentViewClassDescriptorFromClass:(Class
{
.viewClass = viewClass,
.observesMountingTransactionWillMount =
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMount:withSurfaceTelemetry:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMountWithMetadata:)),
.observesMountingTransactionDidMount =
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMount:withSurfaceTelemetry:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMountWithMetadata:)),
};
#pragma clang diagnostic pop
}
Expand Down
13 changes: 6 additions & 7 deletions React/Fabric/Mounting/RCTMountingManager.mm
Original file line number Diff line number Diff line change
Expand Up @@ -266,16 +266,15 @@ - (void)performTransaction:(MountingCoordinator::Shared const &)mountingCoordina
auto surfaceId = mountingCoordinator->getSurfaceId();

mountingCoordinator->getTelemetryController().pullTransaction(
[&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) {
[&](MountingTransactionMetadata metadata) {
[self.delegate mountingManager:self willMountComponentsWithRootTag:surfaceId];
_observerCoordinator.notifyObserversMountingTransactionWillMount(transaction, surfaceTelemetry);
_observerCoordinator.notifyObserversMountingTransactionWillMount(metadata);
},
[&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) {
RCTPerformMountInstructions(
transaction.getMutations(), _componentViewRegistry, _observerCoordinator, surfaceId);
[&](ShadowViewMutationList const &mutations) {
RCTPerformMountInstructions(mutations, _componentViewRegistry, _observerCoordinator, surfaceId);
},
[&](MountingTransaction const &transaction, SurfaceTelemetry const &surfaceTelemetry) {
_observerCoordinator.notifyObserversMountingTransactionDidMount(transaction, surfaceTelemetry);
[&](MountingTransactionMetadata metadata) {
_observerCoordinator.notifyObserversMountingTransactionDidMount(metadata);
[self.delegate mountingManager:self didMountComponentsWithRootTag:surfaceId];
});
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
#import <butter/map.h>
#import <butter/set.h>

#include <react/renderer/mounting/MountingTransaction.h>
#import <react/renderer/mounting/MountingTransactionMetadata.h>

class RCTMountingTransactionObserverCoordinator final {
public:
Expand All @@ -31,11 +31,9 @@ class RCTMountingTransactionObserverCoordinator final {
* To be called from `RCTMountingManager`.
*/
void notifyObserversMountingTransactionWillMount(
facebook::react::MountingTransaction const &transaction,
facebook::react::SurfaceTelemetry const &surfaceTelemetry) const;
facebook::react::MountingTransactionMetadata const &metadata) const;
void notifyObserversMountingTransactionDidMount(
facebook::react::MountingTransaction const &transaction,
facebook::react::SurfaceTelemetry const &surfaceTelemetry) const;
facebook::react::MountingTransactionMetadata const &metadata) const;

private:
facebook::butter::map<
Expand Down
18 changes: 8 additions & 10 deletions React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm
Original file line number Diff line number Diff line change
Expand Up @@ -40,37 +40,35 @@
}

void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionWillMount(
MountingTransaction const &transaction,
SurfaceTelemetry const &surfaceTelemetry) const
MountingTransactionMetadata const &metadata) const
{
auto surfaceId = transaction.getSurfaceId();
auto surfaceId = metadata.surfaceId;
auto surfaceRegistryIterator = registry_.find(surfaceId);
if (surfaceRegistryIterator == registry_.end()) {
return;
}
auto &surfaceRegistry = surfaceRegistryIterator->second;
for (auto const &componentViewDescriptor : surfaceRegistry) {
if (componentViewDescriptor.observesMountingTransactionWillMount) {
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view mountingTransactionWillMount:transaction
withSurfaceTelemetry:surfaceTelemetry];
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view
mountingTransactionWillMountWithMetadata:metadata];
}
}
}

void RCTMountingTransactionObserverCoordinator::notifyObserversMountingTransactionDidMount(
MountingTransaction const &transaction,
SurfaceTelemetry const &surfaceTelemetry) const
MountingTransactionMetadata const &metadata) const
{
auto surfaceId = transaction.getSurfaceId();
auto surfaceId = metadata.surfaceId;
auto surfaceRegistryIterator = registry_.find(surfaceId);
if (surfaceRegistryIterator == registry_.end()) {
return;
}
auto &surfaceRegistry = surfaceRegistryIterator->second;
for (auto const &componentViewDescriptor : surfaceRegistry) {
if (componentViewDescriptor.observesMountingTransactionDidMount) {
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view mountingTransactionDidMount:transaction
withSurfaceTelemetry:surfaceTelemetry];
[(id<RCTMountingTransactionObserving>)componentViewDescriptor.view
mountingTransactionDidMountWithMetadata:metadata];
}
}
}
8 changes: 3 additions & 5 deletions React/Fabric/Mounting/RCTMountingTransactionObserving.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@

#import <UIKit/UIKit.h>

#include <react/renderer/mounting/MountingTransaction.h>
#import <react/renderer/mounting/MountingTransactionMetadata.h>

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -50,16 +50,14 @@ NS_ASSUME_NONNULL_BEGIN
* Is not being called for a component view which is being mounted as part of the transaction (because the view is not
* registered as an observer yet).
*/
- (void)mountingTransactionWillMount:(facebook::react::MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry;
- (void)mountingTransactionWillMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata;

/*
* Called right after the last mutation instruction is executed.
* Is not being called for a component view which was being unmounted as part of the transaction (because the view is
* not registered as an observer already).
*/
- (void)mountingTransactionDidMount:(facebook::react::MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry;
- (void)mountingTransactionDidMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata;

@end

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#include "MountingTransactionMetadata.h"

namespace facebook {
namespace react {} // namespace react
} // namespace facebook
32 changes: 32 additions & 0 deletions ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
/*
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

#pragma once

#include <react/renderer/mounting/MountingTransaction.h>
#include <react/renderer/telemetry/TransactionTelemetry.h>

namespace facebook {
namespace react {

/*
* Contains all (meta)information related to a MountingTransaction except a list
* of mutation instructions.
* The class is meant to be used when a consumer should not have access to all
* information about the transaction (incapsulation) but still needs to observe
* it to produce some side-effects.
*/
class MountingTransactionMetadata final {
public:
SurfaceId surfaceId;
MountingTransaction::Number number;
TransactionTelemetry telemetry;
SurfaceTelemetry surfaceTelemetry;
};

} // namespace react
} // namespace facebook
15 changes: 9 additions & 6 deletions ReactCommon/react/renderer/mounting/TelemetryController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,32 +17,35 @@ TelemetryController::TelemetryController(
: mountingCoordinator_(mountingCoordinator) {}

bool TelemetryController::pullTransaction(
MountingTransactionCallback const &willMount,
MountingTransactionCallback const &doMount,
MountingTransactionCallback const &didMount) const {
std::function<void(MountingTransactionMetadata metadata)> const &willMount,
std::function<void(ShadowViewMutationList const &mutations)> const &doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
const {
auto optional = mountingCoordinator_.pullTransaction();
if (!optional.has_value()) {
return false;
}

auto transaction = std::move(*optional);

auto surfaceId = transaction.getSurfaceId();
auto number = transaction.getNumber();
auto telemetry = transaction.getTelemetry();
auto numberOfMutations = static_cast<int>(transaction.getMutations().size());

mutex_.lock();
auto compoundTelemetry = compoundTelemetry_;
mutex_.unlock();

willMount(transaction, compoundTelemetry);
willMount({surfaceId, number, telemetry, compoundTelemetry});

telemetry.willMount();
doMount(transaction, compoundTelemetry);
doMount(transaction.getMutations());
telemetry.didMount();

compoundTelemetry.incorporate(telemetry, numberOfMutations);

didMount(transaction, compoundTelemetry);
didMount({surfaceId, number, telemetry, compoundTelemetry});

mutex_.lock();
compoundTelemetry_ = compoundTelemetry;
Expand Down
14 changes: 7 additions & 7 deletions ReactCommon/react/renderer/mounting/TelemetryController.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,17 +11,14 @@
#include <mutex>

#include <react/renderer/mounting/MountingTransaction.h>
#include <react/renderer/mounting/MountingTransactionMetadata.h>
#include <react/renderer/telemetry/TransactionTelemetry.h>

namespace facebook {
namespace react {

class MountingCoordinator;

using MountingTransactionCallback = std::function<void(
MountingTransaction const &transaction,
SurfaceTelemetry const &surfaceTelemetry)>;

/*
* Provides convenient tools for aggregating and accessing telemetry data
* associated with running Surface.
Expand All @@ -46,9 +43,12 @@ class TelemetryController final {
* Calls `MountingCoordinator::pullTransaction()` and aggregates telemetry.
*/
bool pullTransaction(
MountingTransactionCallback const &willMount,
MountingTransactionCallback const &doMount,
MountingTransactionCallback const &didMount) const;
std::function<void(MountingTransactionMetadata metadata)> const
&willMount,
std::function<void(ShadowViewMutationList const &mutations)> const
&doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
const;

private:
MountingCoordinator const &mountingCoordinator_;
Expand Down

2 comments on commit 2f5a1e6

@kmagiera
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @makovkastar – can you share some details as to why this change has been reverted wither here or on the original PR #33510. Happy to work on fixing this but w/o any details I don't know what exactly should be changed.

@makovkastar
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kmagiera, it caused an insta crash on Marketplace, so I had to revert it to prevent bleeding. The exact error was [error][tid:main][TransactionTelemetry.cpp:141] react_native_assert failure: mountStartTime_ != kTelemetryUndefinedTimePoint. We already have a fix, so we are going to re-land it with the fix applied. The fix ensures that the transaction telemetry modified by the transaction controller is the same as the one sent in the view callbacks.

Please sign in to comment.