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.

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

[iOS] Introduce TurboModule Setup Metric #24732

Closed

Conversation

a-dilettante
Copy link
Contributor

@a-dilettante a-dilettante commented May 7, 2019

Summary

With the introduction of TurboModules, it would be beneficial to measure the setup time of these modules, as we currently have it in place for NativeModules.

The instantiation of the TMs occurs in the RCTTurboModuleManager. In order to successfully measure the time it took to setup the module, we need to ensure that we don't take into account cached modules. As such, we need to:

  1. Check if module is in _turboModuleCache
    a. Start mark for RCTPLTurboModuleSetup tag if not found
  2. Get the TM via [self provideTurboModule:]
  3. Check if module is in _turboModuleCache
    a. Stop mark for RCTPLTurboModuleSetup if we did not find module in cache prior to step 2 and if it's now present in the cache.
    b. Notify about setup time if the above is true.
  4. Return TM

Changelog

[iOS] [Added] - Gain insights on the the turbo module setup times by observing RCTDidSetupModuleNotification. The userInfo dictionary will contain the module name and setup time in milliseconds. These values can be extracted via RCTDidSetupModuleNotificationModuleNameKey and RCTDidSetupModuleNotificationSetupTimeKey.

Test Plan

Observe for RCTDidSetupModuleNotification

- (void)setup {
    [_notificationCenter addObserver:self
                            selector:@selector(didSetupModule:)
                                name:RCTDidSetupModuleNotification
                              object:nil];
}

- (void)didSetupModule:(NSNotification *)notification {
    NSDictionary *userInfo = notification.userInfo;
    if (userInfo) {
        NSString *moduleName = userInfo[RCTDidSetupModuleNotificationModuleNameKey];
        NSNumber *setupTime = userInfo[RCTDidSetupModuleNotificationSetupTimeKey];
        NSLog(@"%@ setup took %li ms", moduleName, [setupTime integerValue]);
    }
}

Output

TestTurboModule setup took 1 ms

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 7, 2019
@react-native-bot react-native-bot added Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature. labels May 7, 2019
@shergin shergin requested review from RSNara and removed request for shergin May 9, 2019 16:26
@shergin
Copy link
Contributor

shergin commented May 9, 2019

cc @RSNara

@a-dilettante
Copy link
Contributor Author

Hello @shergin and @RSNara,

Have you managed to take a look at this PR?

Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Whoops. Sorry for the late review!

This looks mostly good, but I'm not convinced that we should split the RCTDidSetupModuleNotification into RCTDidSetupNativeModuleNotification and RCTDidSetupTurboModuleNotification. So, I'm requesting changes to get your input.

if (!didFindModuleInCacheBeforeCreation) {
[strongSelf->_bridge.performanceLogger markStartForTag:RCTPLTurboModuleSetup];
}

Copy link
Contributor

Choose a reason for hiding this comment

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

nit: The variable name didFindModuleInCacheBeforeCreation is a bit strange. What you're really checking here is: "Does this TurboModule exist?". So you could just use the moduleIsInitialized function defined further down in the file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahh, perfect - How did I miss that one out? 😅


std::shared_ptr<react::TurboModule> turboModule = [strongSelf provideTurboModule:moduleName];

bool didFindModuleInCacheAfterCreation = strongSelf->_turboModuleCache.find(moduleName) != strongSelf->_turboModuleCache.end();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same deal here.

@@ -27,7 +27,8 @@
NSString *const RCTJavaScriptDidLoadNotification = @"RCTJavaScriptDidLoadNotification";
NSString *const RCTJavaScriptDidFailToLoadNotification = @"RCTJavaScriptDidFailToLoadNotification";
NSString *const RCTDidInitializeModuleNotification = @"RCTDidInitializeModuleNotification";
NSString *const RCTDidSetupModuleNotification = @"RCTDidSetupModuleNotification";
NSString *const RCTDidSetupNativeModuleNotification = @"RCTDidSetupNativeModuleNotification";
NSString *const RCTDidSetupTurboModuleNotification = @"RCTDidSetupTurboModuleNotification";
Copy link
Contributor

Choose a reason for hiding this comment

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

A module is either a NativeModule or a TurboModule (and you know which is which). So I'm not sure why it's necessary to to break this notification down into two. Eventually, we plan to remove the Bridge and its accompanying NativeModule system, so we'll have to undo this change anyway. Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good shout. It really didn't make sense to do so when I looked at this for the second time.

@a-dilettante a-dilettante force-pushed the turbo_module_setup_metrics branch from 766d7f4 to bc95edb Compare May 15, 2019 21:28
@a-dilettante
Copy link
Contributor Author

Hi @RSNara, I've implemented the changes thanks to your feedback and I think we've covered everything.

Copy link
Contributor

@RSNara RSNara left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks 😁

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@RSNara is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @RCiesielczuk in cd2f8c5.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 15, 2019
@a-dilettante a-dilettante deleted the turbo_module_setup_metrics branch May 16, 2019 08:07
a-dilettante added a commit to Skyscanner/react-native that referenced this pull request May 16, 2019
Summary:
With the introduction of TurboModules, it would be beneficial to measure the setup time of these modules, as we currently have it in place for NativeModules.

The instantiation of the TMs occurs in the `RCTTurboModuleManager`. In order to successfully measure the time it took to setup the module, we need to ensure that we don't take into account cached modules. As such, we need to:

1. Check if module is in `_turboModuleCache`
  a. Start mark for `RCTPLTurboModuleSetup` tag if not found
2. Get the TM via `[self provideTurboModule:]`
3. Check if module is in `_turboModuleCache`
   a. Stop mark for `RCTPLTurboModuleSetup` if we did not find module in cache prior to **step 2** and if it's now present in the cache.
   b. Notify about setup time if the above is true.
4. Return TM

[iOS] [Added] - Gain insights on the the turbo module setup times by observing `RCTDidSetupModuleNotification`. The userInfo dictionary will contain the module name and setup time in milliseconds. These values can be extracted via `RCTDidSetupModuleNotificationModuleNameKey` and `RCTDidSetupModuleNotificationSetupTimeKey`.
Pull Request resolved: facebook#24732

Differential Revision: D15362088

Pulled By: RSNara

fbshipit-source-id: e6a8044e4aba5a12ae63e9c7dbf707a17ec00180
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
Summary:
With the introduction of TurboModules, it would be beneficial to measure the setup time of these modules, as we currently have it in place for NativeModules.

The instantiation of the TMs occurs in the `RCTTurboModuleManager`. In order to successfully measure the time it took to setup the module, we need to ensure that we don't take into account cached modules. As such, we need to:

1. Check if module is in `_turboModuleCache`
  a. Start mark for `RCTPLTurboModuleSetup` tag if not found
2. Get the TM via `[self provideTurboModule:]`
3. Check if module is in `_turboModuleCache`
   a. Stop mark for `RCTPLTurboModuleSetup` if we did not find module in cache prior to **step 2** and if it's now present in the cache.
   b. Notify about setup time if the above is true.
4. Return TM

## Changelog

[iOS] [Added] - Gain insights on the the turbo module setup times by observing `RCTDidSetupModuleNotification`. The userInfo dictionary will contain the module name and setup time in milliseconds. These values can be extracted via `RCTDidSetupModuleNotificationModuleNameKey` and `RCTDidSetupModuleNotificationSetupTimeKey`.
Pull Request resolved: facebook#24732

Differential Revision: D15362088

Pulled By: RSNara

fbshipit-source-id: e6a8044e4aba5a12ae63e9c7dbf707a17ec00180
facebook-github-bot pushed a commit that referenced this pull request Sep 6, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
cipolleschi pushed a commit that referenced this pull request Sep 7, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([facebook#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([facebook#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766))" ([facebook#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([facebook#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([facebook#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([facebook#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([facebook#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([facebook#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([facebook#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([facebook#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([facebook#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([facebook#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([facebook#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([facebook#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([facebook#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([facebook#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([facebook#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([facebook#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([facebook#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants