Fix broken perf marker logging for RN > 0.58 on Android #23851

karanjthakkar · 2019-03-11T13:53:39Z

Summary

Changelog

[Android] [Fixed] - Fix broken perf marker logging for RN > 0.58 on Android

Test Plan

The relevant markers are now logged whenever you initialise RN bridge.

karanjthakkar · 2019-03-11T13:55:05Z

The intention of this PR is not to be a final solution but to allow someone from FB to import this internally and see if it causes any regressions.

facebook-github-bot

@axe-fb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

karanjthakkar · 2019-03-25T13:34:12Z

@axe-fb Do you have any updates from the result of importing this internally? There was a merge conflict that occurred from a recent change to master that I have now fixed as well.

facebook-github-bot

@axe-fb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

axe-fb · 2019-03-25T17:18:27Z

@karanjthakkar - I imported it internally, and looks like there are a ton of apps internally that use the old signature to initialize the JSC factory, causing compilation errors. Need to see if there is a way to change them all, or not add a new param to the constructor.

A side question - can this instead be a C++ module, instead of passing it in the constructor ?

karanjthakkar · 2019-03-26T10:19:54Z

A side question - can this instead be a C++ module, instead of passing it in the constructor ?

@axe-fb I don't know what you mean by this. I'm not super well versed with the C++ architecture of RN. Do you have an example?

Need to see if there is a way to change them all, or not add a new param to the constructor.

The constructor technique was something Ben suggested in the original issue. I did not know internally folks create different JSC Factory. What is the usecase for that? Would overloading the constructor to allow zero arguments help?

Essentially I just want the metrics to work like they did before. There are already plans in place for v0.60 whereas this has been broken since v0.58. I'm happy to do whatever changes you think are appropriate to get this resolved. As you can see this has been sitting for quite some time and at some point it would get too difficult for me to resolve the conflicts that come from continuous updates to master. Its super unfortunate that this bug wasn't caught because they depend on different build systems for OSS and FB.

karanjthakkar · 2019-03-26T10:45:50Z

ReactAndroid/src/main/java/com/facebook/react/jscexecutor/OnLoad.cpp

@@ -19,6 +20,7 @@ namespace {

 class JSCExecutorFactory : public JSExecutorFactory {
 public:
+  JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}


Suggested change

JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}

JSCExecutorFactory() { logTaggedMarker_ = nullptr; }

JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}

@axe-fb Maybe something like this? Assuming the other instances of JSC factory that you are talking about don't need marker logging.

Yeah. I was thinking we could have a second constructor, which would be hte original signature and hence existing apps will not break.

@axe-fb I can make that change but I wonder if that would be enough. This change would not break those features but again it will swallow any marker logs from the features because the logger won't be available. Do you think that would create any problems?

facebook-github-bot

@axe-fb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

karanjthakkar · 2019-03-26T17:28:34Z

FYI, test_android job is failing on master as well. So I assume it's unrelated to this PR.

karanjthakkar · 2019-03-28T12:22:13Z

@axe-fb @mhorowitz Do you folks have any update here? Anything I can do to get this to resolution faster?

axe-fb · 2019-04-25T05:15:14Z

React/CxxBridge/RCTCxxBridge.mm

@@ -112,9 +112,9 @@ static void notifyAboutModuleSetup(RCTPerformanceLogger *performanceLogger, cons
  }
 }

-static void registerPerformanceLoggerHooks(RCTPerformanceLogger *performanceLogger) {
+static ReactMarker::LogTaggedMarker registerPerformanceLoggerHooks(RCTPerformanceLogger *performanceLogger) {


Comment from @mhorowitz

This is also a strange name, since it's not "registering" anymore.

I've made it getPerformanceLoggerHooks now. Let me know if that's okay.

karanjthakkar · 2019-04-25T10:53:10Z

React/CxxBridge/JSCExecutorFactory.h

@@ -15,6 +15,10 @@ namespace react {

 class JSCExecutorFactory : public JSExecutorFactory {
 public:
+  explicit JSCExecutorFactory(


This change is needed to make the RNTester turbo module example work.

facebook-github-bot

@axe-fb has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

karanjthakkar · 2019-05-09T18:20:56Z

@axe-fb Do you have any updates here? Did the internal tests pass?

axemclion · 2019-05-10T06:32:43Z

Sorry, did not mean change the constructor of JSCExecutor. Meant change JSIExecutor. Never mind though. I have made the changes, and am now trying to get it building internally.

axe-fb · 2019-05-10T23:34:40Z

So internal tests succeed, but OSS tests are failing !!

xplat/js/react-native-github/RNTester/RNTester/AppDelegate.mm:139:15: note: in instantiation of function template specialization 'std::__1::make_unique<facebook::react::JSCExecutorFactory, (lambda at xplat/js/react-native-github/RNTester/RNTester/AppDelegate.mm:139:64)>' requested here
  return std::make_unique<facebook::react::JSCExecutorFactory>([weakSelf, bridge](facebook::jsi::Runtime &runtime) {

now working on fixing those !!

This is indeed a game of whack-a-mole !! But atleast, we have good tests to catch these !!

karanjthakkar · 2019-05-11T06:42:19Z

Yay! That's great! If you could push the commit here that you did to fix internal tests, I can try to look into why the oss tests are failing.

axe-fb · 2019-05-12T15:11:20Z

Update: I fixed all internal and OSS tests, so I think we should be good. I am now waiting for internal code review now, since the commit now has grown to touch a lot more files now.

karanjthakkar · 2019-05-12T17:31:47Z

@axe-fb Thanks for the update! Look forward to seeing this resolved!

karanjthakkar · 2019-06-06T14:53:51Z

@axe-fb Any update here? 🙂

karanjthakkar requested a review from mhorowitz as a code owner March 11, 2019 13:53

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 Mar 11, 2019

react-native-bot added the Platform: Android Android applications. label Mar 11, 2019

react-native-bot added the PR: Includes Changelog label Mar 11, 2019

karanjthakkar force-pushed the fix_react_markers branch 2 times, most recently from 5d7931c to 1aad713 Compare March 13, 2019 17:09

react-native-bot added PR: Includes Changelog labels Mar 14, 2019

hramos removed the PR: Includes Changelog label Mar 14, 2019

facebook-github-bot reviewed Mar 21, 2019

View reviewed changes

karanjthakkar force-pushed the fix_react_markers branch from 1aad713 to e5ab170 Compare March 25, 2019 13:33

facebook-github-bot reviewed Mar 25, 2019

View reviewed changes

karanjthakkar force-pushed the fix_react_markers branch from e5ab170 to a9c94b3 Compare March 26, 2019 10:35

karanjthakkar commented Mar 26, 2019

View reviewed changes

facebook-github-bot reviewed Mar 26, 2019

View reviewed changes

axe-fb reviewed Apr 25, 2019

View reviewed changes

Fix missing perf marker logging

163fdfd

karanjthakkar force-pushed the fix_react_markers branch 2 times, most recently from 8d1a7a6 to 319d2da Compare April 25, 2019 10:51

karanjthakkar commented Apr 25, 2019

View reviewed changes

Overload JSCExecutorFactory constructor with new signature.

661c827

karanjthakkar force-pushed the fix_react_markers branch from 319d2da to 661c827 Compare April 25, 2019 11:08

facebook-github-bot reviewed Apr 25, 2019

View reviewed changes

react-native-bot added the Bug label May 3, 2019

hramos removed Type: Bug Fix🐛 labels May 6, 2019

karanjthakkar closed this Dec 18, 2020

karanjthakkar deleted the fix_react_markers branch December 18, 2020 01:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix broken perf marker logging for RN > 0.58 on Android #23851

Fix broken perf marker logging for RN > 0.58 on Android #23851

karanjthakkar commented Mar 11, 2019

karanjthakkar commented Mar 11, 2019 •

edited

Loading

facebook-github-bot left a comment

karanjthakkar commented Mar 25, 2019

facebook-github-bot left a comment

axe-fb commented Mar 25, 2019

karanjthakkar commented Mar 26, 2019

karanjthakkar Mar 26, 2019

axe-fb Apr 25, 2019

karanjthakkar Apr 25, 2019

facebook-github-bot left a comment

karanjthakkar commented Mar 26, 2019

karanjthakkar commented Mar 28, 2019

axe-fb Apr 25, 2019

karanjthakkar Apr 25, 2019

karanjthakkar Apr 25, 2019

facebook-github-bot left a comment

karanjthakkar commented May 9, 2019

axemclion commented May 10, 2019

axe-fb commented May 10, 2019

karanjthakkar commented May 11, 2019

axe-fb commented May 12, 2019

karanjthakkar commented May 12, 2019

karanjthakkar commented Jun 6, 2019

	JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}
	JSCExecutorFactory() { logTaggedMarker_ = nullptr; }
	JSCExecutorFactory(ReactMarker::LogTaggedMarker logTaggedMarker) : logTaggedMarker_(logTaggedMarker) {}

Fix broken perf marker logging for RN > 0.58 on Android #23851

Fix broken perf marker logging for RN > 0.58 on Android #23851

Conversation

karanjthakkar commented Mar 11, 2019

Summary

Changelog

Test Plan

karanjthakkar commented Mar 11, 2019 • edited Loading

facebook-github-bot left a comment

Choose a reason for hiding this comment

karanjthakkar commented Mar 25, 2019

facebook-github-bot left a comment

Choose a reason for hiding this comment

axe-fb commented Mar 25, 2019

karanjthakkar commented Mar 26, 2019

karanjthakkar Mar 26, 2019

Choose a reason for hiding this comment

axe-fb Apr 25, 2019

Choose a reason for hiding this comment

karanjthakkar Apr 25, 2019

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

karanjthakkar commented Mar 26, 2019

karanjthakkar commented Mar 28, 2019

axe-fb Apr 25, 2019

Choose a reason for hiding this comment

karanjthakkar Apr 25, 2019

Choose a reason for hiding this comment

karanjthakkar Apr 25, 2019

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

karanjthakkar commented May 9, 2019

axemclion commented May 10, 2019

axe-fb commented May 10, 2019

karanjthakkar commented May 11, 2019

axe-fb commented May 12, 2019

karanjthakkar commented May 12, 2019

karanjthakkar commented Jun 6, 2019

karanjthakkar commented Mar 11, 2019 •

edited

Loading