Fix memory usage display in Perf Monitor by dexterhahaha · Pull Request #19664 · facebook/react-native

dexterhahaha · 2018-06-12T08:12:14Z

According to: https://github.com/apple/darwin-xnu/blob/master/osfmk/kern/bsd_kern.c#L420
The result is the same as Xcode app memory uesd.

Test Plan

Before Fix:

After Fix:

Related PRs

Release Notes

[IOS] [BUGFIX] [Monitor] - fix memory perf monitor. Make sure the memory footprint is the same as Jetsam.

According to: https://github.com/apple/darwin-xnu/blob/master/osfmk/kern/bsd_kern.c#L420 The result is the same as Xcode app memory uesd.

fix: Fix memory usage

facebook-github-bot · 2018-06-12T08:15:33Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

facebook-github-bot · 2018-06-12T17:31:08Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

hramos · 2018-06-12T17:34:56Z

Can you elaborate? What issue does this fix, how was this tested?

dexterhahaha · 2018-06-13T09:49:55Z

Make sure the memory footprint is the same as Xcode monitor.
iOS Jetsam(out of memory) use this method to measure process memory usage.

Before change:

**After change: **

zhongwuzw · 2018-06-13T10:35:00Z

@JunyiXie Xcode 's memory gauge is good for noticing memory usage trends, and is only a barometer refer to the developer from Apple, see Difference in iOS memory allocation measurement between XCode and instruments.

You can have Allocation template of Instruments to see the memory use.

dexterhahaha · 2018-06-13T11:20:35Z

@zhongwuzw
Monitoring should accurately reflect the process memory footprint. There is no relationship with memory debugging.
iOS Jetsam(out of memory) use this method to measure process memory usage.

It is necessary to be consistent with the system memory processing mechanism.

hramos

Please provide a proper test plan and release notes.

dexterhahaha · 2018-06-14T06:15:37Z

Is the pull request format correct now?

hramos · 2018-06-14T22:26:57Z

Thank you!

hramos

I'll let @shergin or someone more familiar with iOS tooling approve this.

henrikra · 2018-07-09T12:09:33Z

+    if(kernelReturn == KERN_SUCCESS) {
+        memoryUsageInByte = (vm_size_t) vmInfo.phys_footprint;
+    } else {
+    }


Can you remove this empty else block safely?

Delete useless logic. Don't have bad effect .

dexterhahaha · 2018-07-10T08:09:59Z

Delete useless logic.

facebook-github-bot · 2018-08-09T17:06:15Z

@JunyiXie I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

facebook-github-bot

hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

react-native-bot · 2018-08-09T20:05:26Z

This pull request was closed by @JunyiXie in eb1ce28.

Once this commit is added to a release, you will see the corresponding version tag below the description at eb1ce28. If the commit has a single master tag, it is not yet part of a release.

Summary: According to: https://github.com/apple/darwin-xnu/blob/master/osfmk/kern/bsd_kern.c#L420 The result is the same as Xcode app memory uesd. Before Fix: <img width="250" alt="before" src="https://user-images.githubusercontent.com/22233256/41394508-ce810362-6fdc-11e8-8d40-73703ec93bc3.png"> <img width="266" alt="before1" src="https://user-images.githubusercontent.com/22233256/41394511-d00a9a0e-6fdc-11e8-9f18-2198e8de2410.png"> After Fix: <img width="270" alt="after" src="https://user-images.githubusercontent.com/22233256/41394517-d5a56ff2-6fdc-11e8-8d19-41e046c4b511.png"> <img width="267" alt="after1" src="https://user-images.githubusercontent.com/22233256/41394520-d7a3c600-6fdc-11e8-98c4-e3024a4532ec.png">  [IOS] [BUGFIX] [Monitor] - fix memory perf monitor. Make sure the memory footprint is the same as Jetsam.  Pull Request resolved: #19664 Differential Revision: D9246307 Pulled By: hramos fbshipit-source-id: f85efc54cdcb0c19677594d465752c666d5af18b

Dexter added 2 commits June 12, 2018 16:09

fix: Fix memory usage

7836f63

According to: https://github.com/apple/darwin-xnu/blob/master/osfmk/kern/bsd_kern.c#L420 The result is the same as Xcode app memory uesd.

Merge pull request #1 from JunyiXie/memory-usage-patch

457dd63

fix: Fix memory usage

react-native-bot added ✅Test Plan Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jun 12, 2018

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 12, 2018

hramos added Missing Test Plan This PR appears to be missing a test plan. and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. ✅Test Plan labels Jun 13, 2018

hramos requested a review from shergin June 13, 2018 17:37

hramos added the Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. label Jun 13, 2018

hramos suggested changes Jun 13, 2018

View reviewed changes

hramos changed the title ~~Fix memory usage~~ Fix memory usage display in Perf Monitor Jun 13, 2018

react-native-bot added ✅Test Plan and removed Missing Test Plan This PR appears to be missing a test plan. labels Jun 13, 2018

react-native-bot added 🐛Bug Fix Platform: iOS iOS applications. and removed Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. labels Jun 14, 2018

hramos reviewed Jun 14, 2018

View reviewed changes

henrikra reviewed Jul 9, 2018

View reviewed changes

Delete useless logic.

045c5df

Delete useless logic. Don't have bad effect .

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 9, 2018

facebook-github-bot approved these changes Aug 9, 2018

View reviewed changes

facebook-github-bot closed this in eb1ce28 Aug 9, 2018

facebook locked as resolved and limited conversation to collaborators Aug 9, 2018

react-native-bot added the Merged This PR has been merged. label Aug 9, 2018

hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019

react-native-bot added the Bug label May 3, 2019

Conversation

dexterhahaha commented Jun 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Plan

Related PRs

Release Notes

Uh oh!

facebook-github-bot commented Jun 12, 2018

Uh oh!

facebook-github-bot commented Jun 12, 2018

Uh oh!

hramos commented Jun 12, 2018

Uh oh!

dexterhahaha commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhongwuzw commented Jun 13, 2018

Uh oh!

dexterhahaha commented Jun 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hramos left a comment

Choose a reason for hiding this comment

Uh oh!

dexterhahaha commented Jun 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hramos commented Jun 14, 2018

Uh oh!

hramos left a comment

Choose a reason for hiding this comment

Uh oh!

henrikra Jul 9, 2018

Choose a reason for hiding this comment

Uh oh!

dexterhahaha commented Jul 10, 2018

Uh oh!

facebook-github-bot commented Aug 9, 2018

Uh oh!

facebook-github-bot left a comment

Choose a reason for hiding this comment

Uh oh!

react-native-bot commented Aug 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dexterhahaha commented Jun 12, 2018 •

edited

Loading

dexterhahaha commented Jun 13, 2018 •

edited

Loading

dexterhahaha commented Jun 13, 2018 •

edited

Loading

dexterhahaha commented Jun 14, 2018 •

edited

Loading