Memory profiler patches #399
Conversation
Summary: The function `StackTracesTree::syncWithRuntimeStack` is called whenever allocation profiling is started, in order to ensure the current stack's contents are set up for future call stack pushes and pops. However, it used to stop the sync as soon as it found a null code block. This happens whenever there's a native call in the stack frame. If multiple native calls are layered, it'll miss the JS frames in the middle. Eventually, when those frames are popped, they won't have their parent set up. Instead of stopping completely at the first null code block, go further to find all JS code blocks and push them onto the stack. Also, change all tests in StackTraceTreeTest to be doubled for tracking from the start, to tracking only right before the allocation, to further test `syncWithRuntimeStack` Reviewed By: neildhar Differential Revision: D23889042 fbshipit-source-id: bc094127ed6c3bc57d533799a2c4e50bb0f0e589
Summary: Before, in a heap timeline, instead of a script ID, the script name was emitted twice. This means if the debugger is currently attached, the profiler won't link up to the sources being debugged. The script ID is easy to get, so use it here. Reviewed By: neildhar Differential Revision: D23983086 fbshipit-source-id: 502049540e6d993641245edd08391723798efb47
Summary: For the heap timeline format, Chrome expects there to only be a single root node, and it is the only node with children. To do this, just emit the root that already exists as part of the output. Rename it from `(invalid function name)` to `(root)` to match V8's output. Reviewed By: neildhar Differential Revision: D23882605 fbshipit-source-id: a9458d93d6244528c7331b45f459b8bd500e4edc
… inspector Summary: When taking a heap timeline, Hermes wasn't showing any data until the timeline was written to disk and then reloaded. Turns out we were missing support for two events: * `HeapProfiler.lastSeenObjectId`: This event reports the most recently allocated object ID. Used to know when objects were allocated in the timeline. * `HeapProfiler.heapStatsUpdate`: Report how many objects and bytes exist for a "time fragment", represented by a fragment index. Later updates to the same index can decrease the amount of live memory, which show up as grey spikes instead of blue spikes Previously, we only supported these by writing out to a file, and they didn't work with a "live" profiling view. To fix this, I changed the periodic sampling thread to instead be a periodic flush of a sample every few allocations. The performance impact is tucked away only when profiling is turned on, and it's very non-invasive to the rest of the GC. The flush calls a callback with the relevant information if the inspector is on, and the inspector sends a message back to the browser. Changelog: [Internal] Fix for Hermes heap timeline profiling Reviewed By: neildhar Differential Revision: D23993363 fbshipit-source-id: 8e0b571130cbb7e839dfb009b04f584f5179085d
facebook-github-bot
added
the
CLA Signed
|
Ok I verified that RNTester (Android) worked with this release. I'm seeing a crash when using the Heap profiler, but that might be an unrelated bug that hasn't been resolved in hermes master either. |
|
With the great help from @alloy, I can verify that the artifacts from the 1st revision of this PR (the first 4 commits) did resolve the Before: After: Looks like we are in the very good track and just miss patch for one more API change. |
|
Oh whoops I left a conflict marker in one of those commits. I'll edit and force-push some new commits, which will restart the CI jobs. |
Summary: When logging when a GC occurs, it's good to know if the GC was forced, or was part of the natural allocation paths. This allows us to see if there are too many forced GCs in apps. Reviewed By: neildhar Differential Revision: D23737916 fbshipit-source-id: fd8d6068bed14197b752c3a012f72c1309bc8311
Summary: Continuing the adding of a "cause" field for logging to GCs. This allows embedders of Hermes (such as React Native) to specify the cause of a call to `collectGarbage`. Notably, this allows Hermes to know when a GC is triggered by a memory warning. Changelog: [Internal] Reviewed By: sammy-SC Differential Revision: D23742099 fbshipit-source-id: 99453e632328c00045b92a72f789d41c898dc518
|
Ok force-pushed after verifying |
|
Ok downloaded the new npm and tested with RNTester. Seems to work fine, although heap snapshots seem to be throwing an exception, and the memory profiler is still not working. None of these issues are worth blocking the React Native release, although I'll see if I can attach a debugger There's no difference between EDIT: Looks like the e2e test failed: When I saw this issue locally, it was fixed when I switched from mixing npm and yarn to just yarn. to fixed this issue for me. Or perhaps the difference is just encoding the dependency in the package.json file as |
|
For visibility, I tested the artifact from the updated PR. Seems like there is some non-JSI/Hermes breakages. But at least we've got passed the aforementioned JSI/Hermes errors hence we merged it.
I'll re-upload the Github Release of v0.7.1 one more time so you guys can purge the local CocoaPod cache and update the hermes-engine to see if it repro for you and diagnose it. But the plan would be to have a v0.7.2 release with
|
|
Awesome, thank you. I will test it on my branch and I think we can treat CI failures as a good indicator of what is there to be done. @Huxpro It may be worth to just release another |
Summary
Cherry pick the following commits into release-v0.7:
These are all about the memory profiler, and also ensure the JSI API is compatible with RN 0.64.
Include some merge conflict fixes.
Test Plan
CI will build a Hermes library to embed into an app.
Download that into a test app, connect Chrome following the instructions here:
https://reactnative.dev/docs/hermes#debugging-hermes-using-google-chromes-devtools
Right now the memory profiler crashes, but it's not crashing from a linker error, just a bug in the
memory profiler that will be fixed somewhere else, and will not block shipping a v0.7.2 or RN v0.64