Fix hermes profiler #34129

janicduplessis · 2022-07-05T04:34:53Z

Summary

The hermes profiler doesn't work currently, I tracked down the problem to a couple of things.

Need to call registerForProfiling to enable profiling for a specific runtime. I added the call at the same place where we enable the debugger.
runInExecutor didn't work and call its callback. Not sure exactly why, but using executor_->add like we do in a lot of other places to run code on the executor works.
GetHeapUsageRequest seems to cause some deadlocks. JS contexts were not detected reliably, I suspect this is related to deadlocks when trying to run on inspector executor. GetHeapUsageRequest doesn't actually need any data from the inspector so there is no need to run it on that queue. To fix it I moved the call to use runInExecutor instead.

Changelog

[General] [Fixed] - Fix hermes profiler

Test Plan

analysis-bot · 2022-07-05T05:04:15Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	7,831,006	-1,430
android	hermes	armeabi-v7a	7,217,268	-1,371
android	hermes	x86	8,139,775	-1,654
android	hermes	x86_64	8,121,728	-1,451
android	jsc	arm64-v8a	9,696,493	-807
android	jsc	armeabi-v7a	8,451,913	-661
android	jsc	x86	9,647,125	-886
android	jsc	x86_64	10,244,951	-876

Base commit: 22a067b
Branch: main

analysis-bot · 2022-07-05T06:47:19Z

Platform	Engine	Arch	Size (bytes)	Diff
ios	-	universal	n/a	--

Base commit: 303aaf8
Branch: main

cortinico

Thanks for sending this over @janicduplessis 👌 I'd like to get a pass from @neildhar @jpporto

ReactCommon/hermes/inspector/chrome/Connection.cpp

ReactCommon/hermes/executor/HermesExecutorFactory.cpp

jpporto · 2022-07-05T16:58:05Z

ReactCommon/hermes/inspector/chrome/Connection.cpp

-  resp->id = req.id;
-
-  inspector_
-      ->executeIfEnabled(


executeIfEnabled() ensures that the JS VM is not running when making calls to it -- it therefore serializes accesses to the VM. I am not sure it is safe to make this change. @neildhar, any thoughts?

I think in this case we want it to run if the js vm is running since this command gets called periodically by devtools to get the current memory usage.

I had a quick look and it seems fine to call from any thread since this info comes from the GC and uses a lock.

getHeapUsage() is only synchronized with the GC, not with the VM execution. Therefore it is not safe to be invoked with the VM running. You could use onPause/onResume to determine whether to invoke getHeapUsage(), or whether return a cached value.

I think it would be nice to be able to run getHeapUsage while the VM is running, it is used to display realtime memory usage in chrome devtools, so requiring that the VM is paused kind of defeats the purpose of this.

Is there any info coming from the VM that needs synchronization? Also can anything bad happen if we execute this on a running VM, except from potential wrong values, which I think is fine in this case.

I could also just disable this for now if we're not comfortable landing this change. This feature is not very important compared to be able to profile.

jpporto · 2022-07-06T17:35:25Z

ReactCommon/hermes/executor/HermesExecutorFactory.cpp

@@ -221,6 +221,12 @@ std::unique_ptr<JSExecutor> HermesExecutorFactory::createJSExecutor(
      decoratedRuntime, delegate, jsQueue, timeoutInvoker_, runtimeInstaller_);
 }

+::hermes::vm::RuntimeConfig HermesExecutorFactory::defaultRuntimeConfig() {
+  return ::hermes::vm::RuntimeConfig::Builder()
+      .withEnableSampleProfiling(true)


This enables the sampling profiler for all RN users by default. I don't know RN enough, but how is the HermesExecutorFactory create? You need to pipe the options to that call, and not set it here by default...

Yes, this would enable it by default, which I think is reasonable. Do you know if there’s any overhead in enabling it? Should we do it only for debug builds?

It would still be possible to change by passing a hermes config when creating the HermesExecutorFactory. This is done in the user’s AppDelegate.mm for iOS (https://github.com/facebook/react-native/blob/main/packages/rn-tester/RNTester/AppDelegate.mm#L195) and Android also supports passing in a JavaScriptExecutor factory (

react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java

Line 87 in b6f7689

public ReactInstanceManagerBuilder setJavaScriptExecutorFactory(

), although the Java binding doesn't seem to expose any options except heap size (https://github.com/facebook/react-native/blob/8bd3edec88148d0ab1f225d2119435681fbbba33/ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/RuntimeConfig.java). The android implementation also seems to enable sample profiler by default in a way that's not really configurable (

react-native/ReactAndroid/src/main/java/com/facebook/hermes/reactexecutor/OnLoad.cpp

Line 50 in 1bf84a6

.withEnableSampleProfiling(true)

).

The problem is that the profiler machinery is thread-affined. To be honest we tried to change this several times but we couldn't find a solution :(

I understand your frustration -- we REALLY want to enable usage of the sampling profiler more broadly, but enabling it by default for all users could lead to usability issues -- incorrect stacks, runtime crashes etc.

I think at this point I'll defer to the RN folks on what they want to do. @cortinico?

Would it be possible to enable the profiler only when devtools are connected? I know chrome devtools send Profiler.enable event when connected. Can registerForProfiling / unregisterForProfiling be called on a running VM?

registerForProfiling/unregisterForProfiling must be invoked in the thread running JS (i.e., the "VM" thread), so the VM will be stopped by necessity.

Is there a way to enable this for iOS in a similar manner than it is currently done for Android?

With that being said, someone from RN should have the final say here as to how they want this done. @cortinico

I think I'm sadly not the right person to make this call.

I understand your frustration -- we REALLY want to enable usage of the sampling profiler more broadly, but enabling it by default for all users could lead to usability issues -- incorrect stacks, runtime crashes etc.

My 2 cents here, I won't enable sampling profiler for all the users, but I'll wrap it around a boolean guard that users can control. I understand the value of this, but I think the set of people that will actually use a sampling profiler are a niche.

From what I can tell, the sampling profiler has been enabled for all users before, and still is for android. It is no longer enabled by default since a recent change in hermes.

I haven't tried to see if it works, but would a better solution be to enable profiler when devtools connect?

The current sampling profiler API requires (un)registerForProfiling() to be called from the thread running the VM, so running it from the inspector thread is wrong.

I imagine it would be possible to run that code on the JS VM thread, not sure if some other methods already do that.

What's the consensus here. I would like to drive this forward and try to merge it. @jpporto I believe this is fine on RN's end. Are we fine merging it?

facebook-github-bot · 2022-07-06T23:25:24Z

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico · 2022-08-05T16:05:55Z

I had a discussion with @jpporto. This is safe to land. We're a bit afraid this might lead to crashes internally due to how we call EnableSampleProfiling. If that is the case, we might have to revert this.

janicduplessis · 2022-08-05T17:10:35Z

Sounds good, I’m fine with exploring other solutions, or trying to enable it only for open source if it causes an issue internally.

facebook-github-bot · 2022-08-30T18:35:33Z

@philIip has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

react-native-bot · 2022-08-31T02:25:04Z

This pull request was successfully merged by @janicduplessis in 81564c1.

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

Summary: The hermes profiler doesn't work currently, I tracked down the problem to a couple of things. - Need to call `registerForProfiling` to enable profiling for a specific runtime. I added the call at the same place where we enable the debugger. - `runInExecutor` didn't work and call its callback. Not sure exactly why, but using `executor_->add` like we do in a lot of other places to run code on the executor works. - `GetHeapUsageRequest` seems to cause some deadlocks. JS contexts were not detected reliably, I suspect this is related to deadlocks when trying to run on inspector executor. `GetHeapUsageRequest` doesn't actually need any data from the inspector so there is no need to run it on that queue. To fix it I moved the call to use `runInExecutor` instead. ## Changelog  [General] [Fixed] - Fix hermes profiler Pull Request resolved: #34129 Reviewed By: cortinico Differential Revision: D37669469 Pulled By: philIip fbshipit-source-id: 6cf3b2857ac60b0a1518837b9c56b9c093ed222f

Fix hermes profiler

186582e

facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. labels Jul 5, 2022

react-native-bot added the Bug label Jul 5, 2022

facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Jul 5, 2022

cortinico reviewed Jul 5, 2022

View reviewed changes

ReactCommon/hermes/inspector/chrome/Connection.cpp Show resolved Hide resolved

jpporto reviewed Jul 5, 2022

View reviewed changes

ReactCommon/hermes/executor/HermesExecutorFactory.cpp Outdated Show resolved Hide resolved

jpporto reviewed Jul 5, 2022

View reviewed changes

Use hermes runtime config

bc3a9cc

jpporto reviewed Jul 6, 2022

View reviewed changes

kelset added the Debugger Issues related to React Native DevTools and the New Debugger label Jul 22, 2022

cortinico approved these changes Aug 5, 2022

View reviewed changes

janicduplessis mentioned this pull request Aug 15, 2022

Fix hermes profiler Expensify/react-native#24

Merged

facebook-github-bot closed this in 81564c1 Aug 31, 2022

react-native-bot added the Merged This PR has been merged. label Aug 31, 2022

janicduplessis deleted the @janic/fix-inspector branch August 31, 2022 12:57

cortinico mentioned this pull request Sep 16, 2022

Hermes Profiler is broken in React Native 0.69.5 and 0.70.0 #34703

Closed

snyk-bot mentioned this pull request Oct 11, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.70.1 NOUIY/react-native-macos#806

Open

janicduplessis mentioned this pull request Oct 22, 2022

Frequent app crashes with message: 'SamplingProfiler should be destroyed on the same thread it is created' #35030

Closed

NOUIY mentioned this pull request Oct 25, 2022

[Snyk] Upgrade react-native from 0.63.3 to 0.70.2 NOUIY/react-native-macos#857

Open

cipolleschi mentioned this pull request Nov 1, 2023

[RN][CI]Update node installation on debian (0.70) #41278

Closed

cipolleschi mentioned this pull request Jan 8, 2024

[0.70][RN][General] Migrate boost download url away from JFrog #42178

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix hermes profiler #34129

Fix hermes profiler #34129

janicduplessis commented Jul 5, 2022 •

edited

Loading

analysis-bot commented Jul 5, 2022 •

edited

Loading

analysis-bot commented Jul 5, 2022 •

edited

Loading

cortinico left a comment

jpporto Jul 5, 2022

janicduplessis Jul 5, 2022

janicduplessis Jul 5, 2022

jpporto Jul 5, 2022

janicduplessis Jul 6, 2022

janicduplessis Jul 6, 2022

jpporto Jul 6, 2022

janicduplessis Jul 6, 2022

jpporto Jul 6, 2022

janicduplessis Jul 6, 2022 •

edited

Loading

jpporto Jul 6, 2022

cortinico Jul 12, 2022

janicduplessis Jul 12, 2022

jpporto Jul 12, 2022

janicduplessis Jul 12, 2022

cortinico Aug 5, 2022

facebook-github-bot commented Jul 6, 2022

cortinico commented Aug 5, 2022

janicduplessis commented Aug 5, 2022

facebook-github-bot commented Aug 30, 2022

react-native-bot commented Aug 31, 2022

Fix hermes profiler #34129

Fix hermes profiler #34129

Conversation

janicduplessis commented Jul 5, 2022 • edited Loading

Summary

Changelog

Test Plan

analysis-bot commented Jul 5, 2022 • edited Loading

analysis-bot commented Jul 5, 2022 • edited Loading

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janicduplessis Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 6, 2022

cortinico commented Aug 5, 2022

janicduplessis commented Aug 5, 2022

facebook-github-bot commented Aug 30, 2022

react-native-bot commented Aug 31, 2022

janicduplessis commented Jul 5, 2022 •

edited

Loading

analysis-bot commented Jul 5, 2022 •

edited

Loading

analysis-bot commented Jul 5, 2022 •

edited

Loading

janicduplessis Jul 6, 2022 •

edited

Loading