Register memory info command on vmservice for Android devices by jonahwilliams · Pull Request #45568 · flutter/flutter

jonahwilliams · 2019-11-25T21:00:50Z

Description

Allows the Device sub-classes to provide a queryRss method which is connected to the vmservice. implements this on AndroidDevice by calling #41225 and parsing the results.

Also cleans up flutter device to support a single vmservice instance.

Fixes #41225

cc @terrylucas to determine what specific fields we need.

jonahwilliams · 2019-11-25T21:06:59Z

+  Device _device;
+  set device(Device device) {
+    _device = device;
+    _peer.registerMethod('flutterRss', (rpc.Parameters params) async {


TBD: good method name

Updated to flutterMemoryInfo

jonahwilliams · 2019-11-25T21:07:42Z

-      vmServices = <VMService>[service];
-      device.getLogReader(app: package).connectedVMServices = vmServices;
+      vmService = service;
+      service.device = device;


This is where the vmservice learns about the device type.

Could you pass the device to VMService.connect ?

jonahwilliams · 2019-11-25T21:07:55Z

FYI, @zanderso @jmagman

zanderso · 2019-11-25T21:16:53Z

 }

+
+const String _kJavaHeapKey = 'Java Heap';


The string 'Java Heap' isn't in the example below or in the linked docs.

Updated to full example

zanderso · 2019-11-25T21:17:21Z

+  for (String line in input.split('\n')) {
+    line = line.trim();
+    if (line.startsWith(_kJavaHeapKey)) {
+      result[_kJavaHeapKey] = int.tryParse(line.split(':').last.trim());


There are no colons in the example above or in the linked documentation.

Parsing of 'TOTAL:' would be a little different because of 'TOTAL SWAP PSS:' on the same line I would ignore 'TOTAL:' as the other 7 will add up to this anyway.

Since the only data we care about is the App Summary. It might be safer to limit parsing - only calling int.tryParse in the App Summary section e.g.,

start parsing after 'App Summary' is encountered

stop parsing after 'Objects' is encountered

zanderso · 2019-11-25T21:21:21Z

+      Unknown    43971    43968        0        1
+        TOTAL    95085    61120    31392       62    23108    13495     9612
+
+ App Summary


This section is missing from the documentation. Do different Android versions output different info?

Added to documentation. Not that I know of

Limiting the parsing to just the 'App Summary' section might help with different versions. The only differences I've seen have to do with older Android version 7 I think.

Older (Android 7 I think)

Dalvik Heap 5110(3) 0 4136 4988(3) 0 0 9168 8958(6) 210

This is the modern output for a few years:

Dalvik Heap 34340 33816 0 0 62436 53883 8553

But I only care about the App Summary section.

zanderso · 2019-11-25T21:23:33Z

  }

+  @override
+  Future<Map<String, Object>> queryRss() async {


Instead of returning a Map, consider promoting the return type here to a class with platform agnostic fields from a base class and android-specific fields from an inherited class.

Then, instead of naming this method queryRss you can name it queryBaseClass since it's really doing a lot more than just grabbing Rss.

I would be converting that back to JSON almost immediately though, so it would be a fairly short-lived object.

I'll wait for terry to chime in with what information we actually need from this dumpinfo first.

When adding support for this to a new platform, it will be good to have support from the language/type-system ensuring that the expected info is filled in.

Jonah I was hopping to get all information in the App Summary section:

Java Heap:
Native Heap:
Code::
Stack:
Graphics:
Private Other:
System:
TOTAL:

these numbers seem the most useful for Android users (or at least its what Android Studio reports in their memory profiler).

At some point, we we'll probably hookup to dumpsys battery and dumpsys network stats too. These might be passed as a row(s) or data instead of lots of parsing (but that's in the future). The JSON/Map seems fine for me as it will be coming via a service call.

zanderso · 2019-11-25T21:31:50Z

  final Device device;
  final ResidentCompiler generator;
  Stream<Uri> observatoryUris;
-  List<VMService> vmServices;


Maybe this cleanup should go in a separate PR.

zanderso · 2019-11-25T21:38:39Z

-      vmServices = <VMService>[service];
-      device.getLogReader(app: package).connectedVMServices = vmServices;
+      vmService = service;
+      service.device = device;


Could you pass the device to VMService.connect ?

zanderso · 2019-11-25T21:39:19Z


+  /// The connected physical device instance.
+  Device _device;
+  set device(Device device) {


If you pass the device to VMService.connect, then maybe you don't need this complicated setter, and you can set up the extension in the constructor with all the other extensions.

jonahwilliams · 2019-11-25T21:51:24Z

        targetModel: TargetModel.dartdevc,
        experimentalFlags: experimentalFlags,
-        platformDill: artifacts.getArtifactPath(Artifact.webPlatformKernelDill, mode: buildMode),
+        platformDill: fs.file(artifacts


This is an unrelated change, will revert

terrylucas

Thanks Jonah this is exactly what I needed. Added a few suggestions and comments.

jonahwilliams · 2019-11-26T18:50:33Z

I've updated the example with the fields Java Heap, Native Heap, Code, Stack, Graphics, Private Other, System, and Total.

This is provided via the vmservice method flutterMemoryInfo (taking better name suggestions). The map also contains an identifier for the platform returned.

In the case where I fail to parse a field, I'm defaulting it to 0.

zanderso

lgtm w/ nit

zanderso · 2019-11-26T19:08:36Z

+    .takeWhile((String line) => !line.contains('TOTAL'));
+  final AndroidMemoryInfo androidMemoryInfo = AndroidMemoryInfo();
+  for (String line in lines) {
+    if (!line.contains(':')) {


To avoid mixing styles:

Maybe move to after the takeWhile, like .where((String line) => line.contains(':'))

Then you can .forEach((String line) {...}) instead of a for loop.

register rss command on vmservice

08660f2

fluttergithubbot added tool Affects the "flutter" command-line tool. See also t: labels. work in progress; do not review labels Nov 25, 2019

googlebot added the cla: yes label Nov 25, 2019

jonahwilliams commented Nov 25, 2019

View reviewed changes

zanderso reviewed Nov 25, 2019

View reviewed changes

jonahwilliams added 2 commits November 25, 2019 13:39

fix closed condition check

3a309e7

rever vmservice refactor, move to constructor

e387e7d

jonahwilliams commented Nov 25, 2019

View reviewed changes

jonahwilliams added 2 commits November 25, 2019 13:52

revert unrelated change

2aa160a

Merge branch 'master' of github.com:flutter/flutter into wire_adb

9f0565f

terrylucas reviewed Nov 26, 2019

View reviewed changes

add MemoryInfo serialization and test cases

3be4d70

jonahwilliams changed the title ~~Register rss command on vmservice for Android devices~~ Register memory info command on vmservice for Android devices Nov 26, 2019

Add registration test

777e94d

jonahwilliams marked this pull request as ready for review November 26, 2019 18:58

jonahwilliams removed the work in progress; do not review label Nov 26, 2019

zanderso approved these changes Nov 26, 2019

View reviewed changes

terrylucas approved these changes Nov 26, 2019

View reviewed changes

fix test and address comments

174ec16

jonahwilliams merged commit 831163f into flutter:master Nov 26, 2019

jonahwilliams deleted the wire_adb branch November 26, 2019 22:50

liyuqian mentioned this pull request Nov 26, 2019

Persist the memory profile timeline #44013

Closed

github-actions Bot locked as resolved and limited conversation to collaborators Aug 3, 2021

Conversation

jonahwilliams commented Nov 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrylucas Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrylucas Nov 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terrylucas left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Nov 26, 2019

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonahwilliams commented Nov 25, 2019 •

edited

Loading

terrylucas Nov 26, 2019 •

edited

Loading

terrylucas Nov 26, 2019 •

edited

Loading