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

Fix hermes profiler #34129

Closed
wants to merge 2 commits into from
Closed

Fix hermes profiler #34129

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
186582e
Fix hermes profiler
janicduplessis Jul 5, 2022
bc3a9cc
Use hermes runtime config
janicduplessis Jul 6, 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
6 changes: 6 additions & 0 deletions ReactCommon/hermes/executor/HermesExecutorFactory.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)
).

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@janicduplessis janicduplessis Jul 6, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

.build();
}

HermesExecutor::HermesExecutor(
std::shared_ptr<jsi::Runtime> runtime,
std::shared_ptr<ExecutorDelegate> delegate,
Expand Down
4 changes: 3 additions & 1 deletion ReactCommon/hermes/executor/HermesExecutorFactory.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ class HermesExecutorFactory : public JSExecutorFactory {
JSIExecutor::RuntimeInstaller runtimeInstaller,
const JSIScopedTimeoutInvoker &timeoutInvoker =
JSIExecutor::defaultTimeoutInvoker,
::hermes::vm::RuntimeConfig runtimeConfig = ::hermes::vm::RuntimeConfig())
::hermes::vm::RuntimeConfig runtimeConfig = defaultRuntimeConfig())
: runtimeInstaller_(runtimeInstaller),
timeoutInvoker_(timeoutInvoker),
runtimeConfig_(std::move(runtimeConfig)) {
Expand All @@ -33,6 +33,8 @@ class HermesExecutorFactory : public JSExecutorFactory {
std::shared_ptr<MessageQueueThread> jsQueue) override;

private:
static ::hermes::vm::RuntimeConfig defaultRuntimeConfig();

JSIExecutor::RuntimeInstaller runtimeInstaller_;
JSIScopedTimeoutInvoker timeoutInvoker_;
::hermes::vm::RuntimeConfig runtimeConfig_;
Expand Down
24 changes: 9 additions & 15 deletions ReactCommon/hermes/inspector/chrome/Connection.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ class Connection::Impl : public inspector::InspectorObserver,

template <typename C>
void runInExecutor(int id, C callback) {
folly::via(executor_.get(), [cb = std::move(callback)]() { cb(); });
executor_->add([cb = std::move(callback)]() { cb(); });
}

std::shared_ptr<RuntimeAdapter> runtimeAdapter_;
Expand Down Expand Up @@ -1411,20 +1411,14 @@ Connection::Impl::makePropsFromValue(
}

void Connection::Impl::handle(const m::runtime::GetHeapUsageRequest &req) {
auto resp = std::make_shared<m::runtime::GetHeapUsageResponse>();
resp->id = req.id;

inspector_
->executeIfEnabled(
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

image

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

"Runtime.getHeapUsage",
[this, req, resp](const debugger::ProgramState &state) {
auto heapInfo = getRuntime().instrumentation().getHeapInfo(false);
resp->usedSize = heapInfo["hermes_allocatedBytes"];
resp->totalSize = heapInfo["hermes_heapSize"];
})
.via(executor_.get())
.thenValue([this, resp](auto &&) { sendResponseToClient(*resp); })
.thenError<std::exception>(sendErrorToClient(req.id));
runInExecutor(req.id, [this, req]() {
auto heapInfo = getRuntime().instrumentation().getHeapInfo(false);
auto resp = std::make_shared<m::runtime::GetHeapUsageResponse>();
resp->id = req.id;
resp->usedSize = heapInfo["hermes_allocatedBytes"];
resp->totalSize = heapInfo["hermes_heapSize"];
sendResponseToClient(*resp);
});
}

void Connection::Impl::handle(const m::runtime::GetPropertiesRequest &req) {
cortinico marked this conversation as resolved.
Show resolved Hide resolved
Expand Down