Skip to content
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

Jump to bottom

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Closed
wants to merge 10 commits into from
Closed

Pass mutation list to RCTMountingTransactionObserving callbacks #33510

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
f840a22
Stashing work
kmagiera Feb 22, 2022
33d26c3
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Mar 28, 2022
d125f38
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Mar 28, 2022
edff89a
Drop unused variables
kmagiera Mar 29, 2022
f2a6105
Remove MountingTransactionMetadata
kmagiera Apr 1, 2022
124c731
Revert changes in mounting observer for modal component
kmagiera Apr 1, 2022
87084d0
Commit podfile lock changes (perhaps not a valid ones) to see if CI p…
kmagiera Apr 1, 2022
d08c67f
Revert changes in rn-tester's podfile.lock
kmagiera Apr 4, 2022
d9b3edb
Add SurfaceTelemetry as additional argument to all transaction observ…
kmagiera Apr 5, 2022
f03b034
Merge remote-tracking branch 'upstream/main' into fabric-mutations-ca…
kmagiera Apr 5, 2022
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
2 changes: 1 addition & 1 deletion React/Fabric/Mounting/ComponentViews/Modal/RCTModalHostViewComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ - (void)ensurePresentedOnlyIfNeeded

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionWillMountWithMetadata:(MountingTransactionMetadata const &)metadata
- (void)mountingTransactionWillMount:(MountingTransaction const &)transaction
{
_modalContentsSnapshot = [self.viewController.view snapshotViewAfterScreenUpdates:NO];
}
Expand Down
2 changes: 1 addition & 1 deletion React/Fabric/Mounting/ComponentViews/ScrollView/RCTScrollViewComponentView.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ - (void)dealloc

#pragma mark - RCTMountingTransactionObserving

- (void)mountingTransactionDidMountWithMetadata:(MountingTransactionMetadata const &)metadata
- (void)mountingTransactionDidMount:(MountingTransaction const &)transaction
{
[self _remountChildren];
}
Expand Down
4 changes: 2 additions & 2 deletions React/Fabric/Mounting/RCTComponentViewFactory.mm
View file
Open in desktop
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(mountingTransactionWillMountWithMetadata:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionWillMount:)),
.observesMountingTransactionDidMount =
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMountWithMetadata:)),
(bool)class_respondsToSelector(viewClass, @selector(mountingTransactionDidMount:)),
};
#pragma clang diagnostic pop
}
Expand Down
12 changes: 6 additions & 6 deletions React/Fabric/Mounting/RCTMountingManager.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,15 +266,15 @@ - (void)performTransaction:(MountingCoordinator::Shared const &)mountingCoordina
auto surfaceId = mountingCoordinator->getSurfaceId();

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

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

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

private:
facebook::butter::map<
Expand Down
14 changes: 8 additions & 6 deletions React/Fabric/Mounting/RCTMountingTransactionObserverCoordinator.mm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,9 +40,9 @@
}

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

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

#import <UIKit/UIKit.h>

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

NS_ASSUME_NONNULL_BEGIN

Expand Down Expand Up @@ -50,14 +50,16 @@ 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)mountingTransactionWillMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata;
- (void)mountingTransactionWillMount:(facebook::react::MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry;

/*
* 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)mountingTransactionDidMountWithMetadata:(facebook::react::MountingTransactionMetadata const &)metadata;
- (void)mountingTransactionDidMount:(facebook::react::MountingTransaction const &)transaction
withSurfaceTelemetry:(facebook::react::SurfaceTelemetry const &)surfaceTelemetry;

@end

Expand Down
12 changes: 0 additions & 12 deletions ReactCommon/react/renderer/mounting/MountingTransactionMetadata.cpp
View file
Open in desktop

This file was deleted.

32 changes: 0 additions & 32 deletions ReactCommon/react/renderer/mounting/MountingTransactionMetadata.h
View file
Open in desktop

This file was deleted.

14 changes: 6 additions & 8 deletions ReactCommon/react/renderer/mounting/TelemetryController.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ TelemetryController::TelemetryController(
: mountingCoordinator_(mountingCoordinator) {}

bool TelemetryController::pullTransaction(
std::function<void(MountingTransactionMetadata metadata)> const &willMount,
std::function<void(ShadowViewMutationList const &mutations)> const &doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
MountingTransactionCallback const &willMount,
MountingTransactionCallback const &doMount,
MountingTransactionCallback const &didMount)
const {
auto optional = mountingCoordinator_.pullTransaction();
if (!optional.has_value()) {
Expand All @@ -28,24 +28,22 @@ bool TelemetryController::pullTransaction(

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({surfaceId, number, telemetry, compoundTelemetry});
willMount(transaction, compoundTelemetry);

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

compoundTelemetry.incorporate(telemetry, numberOfMutations);

didMount({surfaceId, number, telemetry, compoundTelemetry});
Copy link
Contributor

Choose a reason for hiding this comment

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

I started updating internal usages of this method, seems like compoundTelemetry is not available in the transaction .
I am not sure what's the best course of action here, maybe we could pass both transaction and telemetry as separate parameters or rollback deletion of metadata and just have it as a { transaction, compoundTelemetry } type of object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about passing SurfaceTelemetry as a second argument to all the observer callbacks? For me it'd seem more natural than adding additional struct to carry them both. If you'd prefer to have such struct then I'd consider either naming it differently than "*Metadata" as it'd seem unintuitive that "TransactionMetadata" actually carries "Transaction". Alternatively I can revert "Metadata" to its previous form and add only a reference to "mutations" to it as that's the only thing we need from transaction to fulfil out usecase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, SurfaceTelemetry as a separate parameter sounds good to me :)

didMount(transaction, compoundTelemetry);

mutex_.lock();
compoundTelemetry_ = compoundTelemetry;
Expand Down
12 changes: 6 additions & 6 deletions ReactCommon/react/renderer/mounting/TelemetryController.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,16 @@
#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 @@ -43,11 +45,9 @@ class TelemetryController final {
* Calls `MountingCoordinator::pullTransaction()` and aggregates telemetry.
*/
bool pullTransaction(
std::function<void(MountingTransactionMetadata metadata)> const
&willMount,
std::function<void(ShadowViewMutationList const &mutations)> const
&doMount,
std::function<void(MountingTransactionMetadata metadata)> const &didMount)
MountingTransactionCallback const &willMount,
MountingTransactionCallback const &doMount,
MountingTransactionCallback const &didMount)
const;

private:
Expand Down