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 1 commit
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
2 changes: 2 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 @@ -164,6 +164,7 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator<ReentrancyCheck> {
runtime_, hermesRuntime_, jsQueue);
facebook::hermes::inspector::chrome::enableDebugging(
std::move(adapter), "Hermes React Native");
hermesRuntime_.registerForProfiling();
janicduplessis marked this conversation as resolved.
Show resolved Hide resolved
#else
(void)hermesRuntime_;
#endif
Expand All @@ -172,6 +173,7 @@ class DecoratedRuntime : public jsi::WithRuntimeDecorator<ReentrancyCheck> {
~DecoratedRuntime() {
#ifdef HERMES_ENABLE_DEBUGGER
facebook::hermes::inspector::chrome::disableDebugging(hermesRuntime_);
hermesRuntime_.unregisterForProfiling();
#endif
}

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