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.

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

Implement Runtime.getHeapUsage for hermes chrome inspector #32895

Closed

Conversation

janicduplessis
Copy link
Contributor

Summary

I was looking at the hermes chrome devtools integration and noticed requests to Runtime.getHeapUsage which was not implemented. When implemented it will show a summary of memory usage of the javascript instance in devtools.

image

Changelog

[General] [Added] - Implement Runtime.getHeapUsage for hermes chrome inspector

Test Plan

Before

image

After

image

@facebook-github-bot 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 Jan 15, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Jan 15, 2022
@facebook-github-bot 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 Jan 15, 2022
@analysis-bot
Copy link

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

Base commit: 6b42bcc
Branch: main

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,292,280 -150
android hermes armeabi-v7a 7,627,521 -144
android hermes x86 8,764,571 -149
android hermes x86_64 8,701,499 -145
android jsc arm64-v8a 9,679,496 -102
android jsc armeabi-v7a 8,671,414 -112
android jsc x86 9,637,281 -113
android jsc x86_64 10,231,803 -107

Base commit: 6b42bcc
Branch: main

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Jan 15, 2022

I realized most of the code is generated using tools/run_msggen. I was able to adapt the script to make it usable outside fb, except for the signedsource part. So someone will have to re-run the codegen in the fb repo. (cd ReactCommon/hermes/inspector/tools && ./run_msggen).

I also added support for including experimental commands to msggen since Runtime.getHeapUsage is considered experimental. I added a config with a list of experimental commands to include otherwise it will generate a lot more types that were previously ignored if not passing --ignore-experimental.

Updating the chrome devtools npm package caused some changes to the schema so there are a few unrelated new properties.

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @janicduplessis in 3568a72.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 2, 2022
@janicduplessis janicduplessis deleted the profile-heap-info branch February 2, 2022 19:24
@ShikaSD
Copy link
Contributor

ShikaSD commented Feb 4, 2022

Hey, @janicduplessis, seems like update of the devtools version made the "params" field required for the resume command, breaking some internal tooling. Would it be possible to either implement the same command without upgrading the version or update codegen to make sure params field is optional?
I am reverting it for now, as we have a cutoff for internal tooling release in a hour :)

@facebook-github-bot
Copy link
Contributor

This pull request has been reverted by 2bc883e.

@janicduplessis
Copy link
Contributor Author

Sadly the command does not exist when using the current devtools protocol version. I checked and it was added after the params object to resume, so there is also no version that exist where it would work.

Would it be hard to update the internal tool to pass params? Other option would be to make the codegen check if params exist, but would cause lots of changes to the codegen output.

@ShikaSD
Copy link
Contributor

ShikaSD commented Feb 4, 2022

Would it be hard to update the internal tool to pass params?

It seems like we have quite a few tools that use this codegen as a source of truth, so probably it is better to modify codegen to check for optional param :)
Does input for the codegen specify whether the param is optional or not? I believe we can add this check to only apply when the parameter is not required, so it should be negligible here.

@janicduplessis
Copy link
Contributor Author

janicduplessis commented Feb 4, 2022

Actually it might be possible to address with minimal changes by checking if all parameters are optional, then make the whole params object optional.

@janicduplessis
Copy link
Contributor Author

Does input for the codegen specify whether the param is optional or not? I believe we can add this check to only apply when the parameter is not required, so it should be negligible here.

Yep https://github.com/ChromeDevTools/devtools-protocol/blob/master/json/js_protocol.json#L641

janicduplessis added a commit to janicduplessis/react-native that referenced this pull request Feb 24, 2022
…32895)

Summary:
I was looking at the hermes chrome devtools integration and noticed requests to `Runtime.getHeapUsage` which was not implemented. When implemented it will show a summary of memory usage of the javascript instance in devtools.

<img width="325" alt="image" src="https://user-images.githubusercontent.com/2677334/149637113-e1d95d26-9e26-46c2-9be6-47d22284f15f.png">

## Changelog

[General] [Added] - Implement Runtime.getHeapUsage for hermes chrome inspector

Pull Request resolved: facebook#32895

Test Plan:
Before

<img width="912" alt="image" src="https://user-images.githubusercontent.com/2677334/149637073-15f4e1fa-8183-42dc-8673-d4371731415c.png">

After

<img width="1076" alt="image" src="https://user-images.githubusercontent.com/2677334/149637085-579dee8f-5efb-4658-b0a8-2400bd119924.png">

Reviewed By: christophpurrer

Differential Revision: D33616658

Pulled By: ShikaSD

fbshipit-source-id: 49d863e6a58d4a92d4c86f9a288ac33ed8d2cb0d
facebook-github-bot pushed a commit that referenced this pull request Mar 1, 2022
Summary:
Reland of #32895 with fix for optional params object.

Original description:

I was looking at the hermes chrome devtools integration and noticed requests to `Runtime.getHeapUsage` which was not implemented. When implemented it will show a summary of memory usage of the javascript instance in devtools.

<img width="325" alt="image" src="https://user-images.githubusercontent.com/2677334/149637113-e1d95d26-9e26-46c2-9be6-47d22284f15f.png">

## Changelog

[General] [Added] - Implement Runtime.getHeapUsage for hermes chrome inspector

Pull Request resolved: #33173

Test Plan:
I was able to reproduce the issue that caused the initial PR to be reverted using the resume request in Flipper. Pausing JS execution then resuming it will cause it to crash before this change, and works fine after.

Before

<img width="912" alt="image" src="https://user-images.githubusercontent.com/2677334/149637073-15f4e1fa-8183-42dc-8673-d4371731415c.png">

After

<img width="1076" alt="image" src="https://user-images.githubusercontent.com/2677334/149637085-579dee8f-5efb-4658-b0a8-2400bd119924.png">

Reviewed By: jpporto

Differential Revision: D34446672

Pulled By: ShikaSD

fbshipit-source-id: 6e26b8d53cd88cddded36437c72a01822551b9d0
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
…33173)

Summary:
Reland of facebook#32895 with fix for optional params object.

Original description:

I was looking at the hermes chrome devtools integration and noticed requests to `Runtime.getHeapUsage` which was not implemented. When implemented it will show a summary of memory usage of the javascript instance in devtools.

<img width="325" alt="image" src="https://user-images.githubusercontent.com/2677334/149637113-e1d95d26-9e26-46c2-9be6-47d22284f15f.png">

[General] [Added] - Implement Runtime.getHeapUsage for hermes chrome inspector

Pull Request resolved: facebook#33173

Test Plan:
I was able to reproduce the issue that caused the initial PR to be reverted using the resume request in Flipper. Pausing JS execution then resuming it will cause it to crash before this change, and works fine after.

Before

<img width="912" alt="image" src="https://user-images.githubusercontent.com/2677334/149637073-15f4e1fa-8183-42dc-8673-d4371731415c.png">

After

<img width="1076" alt="image" src="https://user-images.githubusercontent.com/2677334/149637085-579dee8f-5efb-4658-b0a8-2400bd119924.png">

Reviewed By: jpporto

Differential Revision: D34446672

Pulled By: ShikaSD

fbshipit-source-id: 6e26b8d53cd88cddded36437c72a01822551b9d0
shwanton pushed a commit to shwanton/react-native-macos that referenced this pull request Feb 13, 2023
…32895)

Summary:
I was looking at the hermes chrome devtools integration and noticed requests to `Runtime.getHeapUsage` which was not implemented. When implemented it will show a summary of memory usage of the javascript instance in devtools.

<img width="325" alt="image" src="https://user-images.githubusercontent.com/2677334/149637113-e1d95d26-9e26-46c2-9be6-47d22284f15f.png">

## Changelog

[General] [Added] - Implement Runtime.getHeapUsage for hermes chrome inspector

Pull Request resolved: facebook#32895

Test Plan:
Before

<img width="912" alt="image" src="https://user-images.githubusercontent.com/2677334/149637073-15f4e1fa-8183-42dc-8673-d4371731415c.png">

After

<img width="1076" alt="image" src="https://user-images.githubusercontent.com/2677334/149637085-579dee8f-5efb-4658-b0a8-2400bd119924.png">

Reviewed By: christophpurrer

Differential Revision: D33616658

Pulled By: ShikaSD

fbshipit-source-id: 49d863e6a58d4a92d4c86f9a288ac33ed8d2cb0d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Merged This PR has been merged. Reverted Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants