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

UI CPU and memory utilization graphs in Chrome debugging mode #2050

Closed
wants to merge 22 commits into from

Conversation

@hharnisc
Copy link
Contributor

hharnisc commented Jul 20, 2015

Chrome debugging UI is currently only showing connection state and logs in the console, leaving room for plenty of interesting information.

I've pushed the UI (using the same convention set by FPS -- UI/JS) CPU and memory utilization data over the debug Websocket and tapped into the existing stream of JS calls that get ran in V8.

The number of JS calls in a time interval is counted for all sub calls in a batch
https://github.com/hharnisc/react-native/blob/master/packager/debugger.html#L150

The last 5 batches of JS calls are displayed in a list format.

screen shot 2015-07-19 at 7 34 00 pm

Charts are created with Chart.JS (MIT licensed).

@cyprusglobe

This comment has been minimized.

Copy link

cyprusglobe commented Jul 20, 2015

+1

var jsCallsStartingData = {
labels: labels,
datasets: [
{

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

use 2 spaces for indentation please :)

labels: labels,
datasets: [
{
fillColor: "rgba(238, 210, 2,0.2)",

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

use single quotes

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

nit: space after ,

@@ -112,6 +276,29 @@
})();
</script>
<style type="text/css">
.legend ul{

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

nit: space after ul

<div id="memory-legend" class="legend"></div>
</div>
</div>
<div style='width:49%; display:inline-block; vertical-align:top;'>

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

use class

stringItem += "</li>";
list.push(stringItem);
}
document.getElementById('js-calls').innerHTML = '<ul>' + list.join('') + '</ul>';

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

Can you use React for this? It's super lame to go back to raw html mutations on the react native dev tools. It's also going to limit the number of people that want to contribute

This comment has been minimized.

Copy link
@hharnisc

hharnisc Jul 20, 2015

Author Contributor

Agreed, least dependency effort here isn't good enough

I've spun up another branch to work this out before merging into my forked master

https://github.com/hharnisc/react-native/blob/chrome-debugger-react/packager/debugger.html#L116-L158

The only real downside to this approach is that is uses the in-browser transformer. Seems livable to me for a while, but I'd like to hear your thoughts on that.

A long term approach would be to create a separate NPM module for this and in turn it into a SPA, which would require some more refactoring -- but I'd be keen to see if people find this useful before going down that route.

Anyways if you could take a look at https://github.com/hharnisc/react-native/blob/chrome-debugger-react/packager/debugger.html - I'll be happy to merge that into this PR.

This comment has been minimized.

Copy link
@hharnisc

hharnisc Jul 20, 2015

Author Contributor

going to go ahead an merge this anyways - feels much more smooth using React (of course right?? 😺 )

@@ -14,8 +14,144 @@
<!-- Fake favicon, to avoid extra request to server -->
<link rel="icon" href="data:;base64,iVBORw0KGgo=">
<title>React Native Debugger</title>
<script src="//cdnjs.cloudflare.com/ajax/libs/Chart.js/1.0.2/Chart.min.js"></script>

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

Can you bring in Chart.min.js in the codebase, we want this to work offline as well and don't want it to die if next year the URL becomes invalid.

}
};
function convertToMB(val) {
return val / 1024 / 1024;

This comment has been minimized.

Copy link
@vjeux

vjeux Jul 20, 2015

Contributor

add parenthesis, otherwise it's unclear how it's going to be parsed

@vjeux

This comment has been minimized.

Copy link
Contributor

vjeux commented Jul 20, 2015

I'll let @tadeuzagallo review the C part

@hharnisc

This comment has been minimized.

Copy link
Contributor Author

hharnisc commented Jul 20, 2015

Thanks for the review @vjeux! I'll make these changes

@brentvatne

This comment has been minimized.

Copy link
Collaborator

brentvatne commented Jul 20, 2015

This is very cool @hharnisc 😄

@hharnisc

This comment has been minimized.

Copy link
Contributor Author

hharnisc commented Jul 20, 2015

Thanks @brentvatne!

@@ -8,6 +8,7 @@
*/

#import "RCTDefines.h"
#import "RCTProfile.h"

This comment has been minimized.

Copy link
@tadeuzagallo

tadeuzagallo Jul 20, 2015

Contributor

This should be in the .m file

@@ -32,6 +32,7 @@ @implementation RCTWebSocketExecutor
dispatch_semaphore_t _socketOpenSemaphore;
NSMutableDictionary *_injectedObjects;
NSURL *_url;
NSTimer *_useage_metrics_timer;

This comment has been minimized.

Copy link
@tadeuzagallo

tadeuzagallo Jul 20, 2015

Contributor

NSTimer *_usageMetricsTimer;

@@ -83,6 +84,8 @@ - (void)setUp
[self invalidate];
return;
}
_useage_metrics_timer = [NSTimer scheduledTimerWithTimeInterval:1.0f

This comment has been minimized.

Copy link
@tadeuzagallo

tadeuzagallo Jul 20, 2015

Contributor

This timer is sad... It would probably be better to just include it on bridge calls, and set a longer timer in case the bridge is very idle. Also maybe a way to disable it? If you were profiling on chrome you probably wouldn't want it.

* Exposes device cpu usage metrics - Note this does not include JS Runtime CPU usage
*/

NSNumber *RCTProfileGetCPUUsage();

This comment has been minimized.

Copy link
@tadeuzagallo

tadeuzagallo Jul 31, 2015

Contributor

nit: NSNumber *RCTProfileGetCPUUsage(void);

@"suspend_count": @(info.suspend_count),
@"virtual_size": RCTProfileMemory(info.virtual_size),
@"resident_size": RCTProfileMemory(info.resident_size),
};

This comment has been minimized.

Copy link
@tadeuzagallo

tadeuzagallo Jul 31, 2015

Contributor

I'd write it as:

vm_size_t vs = info.virtual_size;
vm_size_t rs = info.resident_size;
return @{
  @"suspend_count": @(info.suspend_count),
  @"virtual_size": raw ? @(vs) : RCTProfileMemory(vs),
  @"resident_size": raw ? @(rs) : RCTProfileMemory(rs),
};
@tadeuzagallo

This comment has been minimized.

Copy link
Contributor

tadeuzagallo commented Jul 31, 2015

Sorry for taking so long, just a couple of silly nits, but looks good to me. @vjeux do you have any requests or can I merge it?

hharnisc added 2 commits Aug 5, 2015
cleanup whitespace
@hharnisc

This comment has been minimized.

Copy link
Contributor Author

hharnisc commented Aug 5, 2015

Hey @tadeuzagallo! I've cleaned up the code with the changes you pointed out. If its not ready its close to be :shipit:

@tadeuzagallo

This comment has been minimized.

Copy link
Contributor

tadeuzagallo commented Aug 6, 2015

Hey @hharnisc that looks fine to me!

@facebook-github-bot import

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Aug 6, 2015

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1475582742755495/int_phab to review.

@casesandberg

This comment has been minimized.

Copy link

casesandberg commented Aug 15, 2015

👏

@tadeuzagallo

This comment has been minimized.

Copy link
Contributor

tadeuzagallo commented Aug 21, 2015

Hey, I got it merged internally, but perf was an issue :(

I'll try to investigate it later, but I'll just be back to the office on the 29th, if you have any ideas on the mean time, feel free to update the PR and let me know.

PS: this PR will probably be closed, because the diff landed, but it was reverted right after, so feel free to reopen it once it happens.

@mkonicek mkonicek closed this in 46c6cde Aug 25, 2015
@ide ide reopened this Aug 26, 2015
@ide

This comment has been minimized.

Copy link
Contributor

ide commented Aug 26, 2015

Reopening since it was reverted in 8d07df4 as mentioned above.

@facebook-github-bot

This comment has been minimized.

Copy link

facebook-github-bot commented Dec 12, 2015

@hharnisc updated the pull request.

@bradennapier

This comment has been minimized.

Copy link

bradennapier commented Jan 22, 2016

+1

@ghost

This comment has been minimized.

Copy link

ghost commented Mar 1, 2016

@frantic would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Mar 1, 2016

@tadeuzagallo I assume you don't have any bandwidth to continue working on merging this and debugging the perf regression? I know it's been very long (since August) but do you by chance remember what the regression was?

@tadeuzagallo

This comment has been minimized.

Copy link
Contributor

tadeuzagallo commented Mar 2, 2016

I didn't investigate too much, but it added extra calls over the bridge and extra metadata on every call, plus UI updates - which at the time ran in the same thread as the JavaScript in the Executor.

Maybe now that we run it in a webworker inside Chrome it could be less noticeable? If @hharnisc is still willing to rebase it I can go over it again and take another look (or profile it, since we have better tooling now).

@mkonicek

This comment has been minimized.

Copy link
Contributor

mkonicek commented Mar 20, 2016

@hharnisc Do you still plan to work on this? Do you know what caused those perf regressions? I don't have much context, just going though PRs not making sure we either make progress on them or close them.

@ghost

This comment has been minimized.

Copy link

ghost commented May 19, 2016

It's been a while since the last commit was reviewed and the labels show this pull request needs review. Based on the blame information for the files in this pull request we identified @sebmarkbage as a potential reviewer. Could you take a look please or cc someone with more context?

@ghost ghost added the CLA Signed label Jul 12, 2016
@javache

This comment has been minimized.

Copy link
Member

javache commented Aug 4, 2016

This pull request hasn't seen activity in a while. If you still want to merge this, please open a new pull request.

@javache javache closed this Aug 4, 2016
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
Summary:
Chrome debugging UI is currently only showing connection state and logs in the console, leaving room for plenty of interesting information.

I've pushed the UI (using the same convention set by FPS -- UI/JS) CPU and memory utilization data over the debug Websocket and tapped into the existing stream of JS calls that get ran in V8.

The number of JS calls in a time interval is counted for all sub calls in a batch
https://github.com/hharnisc/react-native/blob/master/packager/debugger.html#L150

The last 5 batches of JS calls are displayed in a list format.

<img width="951" alt="screen shot 2015-07-19 at 7 34 00 pm" src="https://cloud.githubusercontent.com/assets/1388079/8769257/edc42f70-2e4d-11e5-8813-e86ef530a446.png">

Charts are created with [Chart.JS](https://github.com/nnnick/Chart.js) (MIT licensed).
Closes facebook/react-native#2050
Github Author: Harrison Harnisch <hharnisc@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.