Page MenuHomePhabricator
Differential D9233

[iOS] Add `withEnableSampleProfiling(true)` to `makeRuntimeConfig`
ClosedPublic
Actions

Authored by atul on Sep 19 2023, 1:28 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Nov 12, 2:15 AM
Unknown Object (File)
Sat, Nov 9, 3:59 PM
Unknown Object (File)
Sun, Nov 3, 8:09 PM
Unknown Object (File)
Thu, Oct 31, 12:51 PM
Unknown Object (File)
Thu, Oct 31, 12:51 PM
Unknown Object (File)
Thu, Oct 31, 12:51 PM
Unknown Object (File)
Thu, Oct 31, 12:51 PM
Unknown Object (File)
Thu, Oct 31, 12:49 PM
View All 78 Files
Subscribers
None

Details

Reviewers
ashoat
ginsu
tomek
bartek
rohan
Commits
rCOMM125b29a6d73e: [iOS] Add `withEnableSampleProfiling(true)` to `makeRuntimeConfig`
Summary

This enables us to use Hermes sampling profiler. Without this change we were getting facebook::jsi::JSINativeException: Runtime not registered for profiling when trying to do a Hermes sample profiling session.

Conveniently this isn't really documented (anywhere I could find), but was able to figure it out searching through hermes and react-native sources on GitHub.

Test Plan

Here's an example:

0bee45.png (772×2 px, 377 KB)

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D9233 (branched from master)
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul created this revision.Sep 19 2023, 1:28 PM
atul published this revision for review.Sep 19 2023, 1:31 PM
atul edited the test plan for this revision. (Show Details)
ashoat added a comment.Sep 19 2023, 1:43 PM

It seems sketchy to do this in production, especially given it's undocumented. I'm worried about the potential impact on performance. Can we restrict this change to dev builds?

Harbormaster completed remote builds in B22741: Diff 31279.Sep 19 2023, 1:47 PM
atul added a comment.EditedSep 19 2023, 1:47 PM
In D9233#271796, @ashoat wrote:

It seems sketchy to do this in production, especially given it's undocumented. I'm worried about the potential impact on performance. Can we restrict this change to dev builds?

So after reading through https://github.com/facebook/react-native/pull/34129/files#diff-45899233397345155a5d7cf81063ce4a749c8c40966cfe30f4189c34c4340436 it looks like this is enabled by default now

Here is is in latest 0.70-stable: https://github.com/facebook/react-native/pull/34129/files#diff-45899233397345155a5d7cf81063ce4a749c8c40966cfe30f4189c34c4340436

atul added a comment.Sep 19 2023, 1:48 PM

(I think the profiler gets disabled on prod builds, I'll do a little more research)

ashoat accepted this revision.Sep 19 2023, 1:50 PM

Okay, please make sure that upstream React Native does this on all builds too before landing!

This revision is now accepted and ready to land.Sep 19 2023, 1:50 PM
atul added a comment.Sep 20 2023, 10:21 AM
In D9233#271801, @ashoat wrote:

Okay, please make sure that upstream React Native does this on all builds too before landing!

Yeah here's the latest HermesExecutorFactory: https://github.com/facebook/react-native/blob/0.70-stable/ReactCommon/hermes/executor/HermesExecutorFactory.cpp. That said I'll toggle it based on DEBUG to be safe.

atul updated this revision to Diff 31319.Sep 20 2023, 10:24 AM

toggle withEnableSampleProfiling on DEBUG

This revision was landed with ongoing or failed builds.Sep 20 2023, 10:31 AM
Closed by commit rCOMM125b29a6d73e: [iOS] Add `withEnableSampleProfiling(true)` to `makeRuntimeConfig` (authored by atul). · Explain Why
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM125b29a6d73e: [iOS] Add `withEnableSampleProfiling(true)` to `makeRuntimeConfig`.
Harbormaster completed remote builds in B22763: Diff 31319.Sep 20 2023, 10:42 AM

Revision Contents
Changeset List

PathSize
native/
ios/
Comm/
7 lines

Diff 31319

View Options

native/ios/Comm/AppDelegate.mm

Loading...