Implement size analyzer to unzip & parse APK and AOT size snapshot to generate analysis json #62495

peterdjlee · 2020-07-29T15:18:14Z

Description

Implemented SizeAnalyzer where an APK file and an AOT size snapshot can be passed in through analyzeApkSize(apk, aotSizeJson) which returns a Map that contains the breakdown of APK data with AOT snapshot data included under libapp.so. `analyzeApkSize(apk, aotSizeJson) also prints out a top level summary to the terminal as such:

Related Issues

#51594

Tests

I added a test file analyze_size_test.dart to test that this tool

builds APK analysis correctly
outputs summary to the command line correctly

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change.

kenzieschmoll · 2020-07-29T15:35:39Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+      json.decode(aotSizeJson.readAsStringSync()) as List<dynamic>,
+    );
+
+    // Rearrange children to ensure the type key shows at the top.


why does the order of the keys in the output map matter?

It's to make sure that the type field isn't just hidden at the bottom of the json because of the giant AOT data. I'm not sure when the user would need to check the type manually by opening up the json, but I was thinking it'd be better to make the type field more visible.

packages/flutter_tools/lib/src/base/analyze_size.dart

kenzieschmoll · 2020-07-29T15:37:34Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+
+  static const int tableWidth = 80;
+
+  Future<Map<String, dynamic>> analyzeApkSize({


add dart doc describing what this method does. Add a note saying that aotSizeJson can be from instruction sizes or v8 snapshot (if that is correct)

From other PR: if this function now does 2 things, reflect it in the name.

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart

jonahwilliams · 2020-07-29T15:45:57Z

packages/flutter_tools/pubspec.yaml

@@ -34,6 +34,7 @@ dependencies:
  native_stack_traces: 0.3.7
  build_daemon: 2.1.4
  shelf: 0.7.5
+  vm_snapshot_analysis: ^0.5.0+1


this should be an exact version, then run flutter update-packages --force-upgrade to update the checksums

jonahwilliams · 2020-07-29T15:46:54Z

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart

+  BufferLogger logger;
+  FakeProcessManager processManager;
+
+  const FakeCommand unzipCommmand = FakeCommand(


for the consts here, I would hoist them out of main at put them at the top level

jonahwilliams · 2020-07-29T15:47:26Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+      currentParent = currentParent.childByName(pathPart) ??
+          currentParent.addChild(SymbolNode(pathPart));
+    }
+    // TODO: this shouldn't be necessary, https://github.com/dart-lang/sdk/issues/41137


this issue is marked as fixed, is this still necessary

jonahwilliams · 2020-07-29T15:52:13Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    return child;
+  }
+
+  List<SymbolNode> get ancestors {


Neither ancestors nor siblings are used in the test, so they currently have 0 coverage

Removed since those are not used (the entire SymbolNode code was copied from here: #51594 (comment))

kenzieschmoll · 2020-07-29T15:44:09Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    return rootNode;
+  }
+
+  /// Prints the paths from currentNode all leaf nodes.


make this documentation more specific. This method seems like it is specific to the lib/ directory in an APK. note that here

kenzieschmoll · 2020-07-29T15:44:45Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+
+    if (currentNode.children.isNotEmpty && !currentNode.name.contains('libapp.so')) {
+      for (final SymbolNode child in currentNode.children) {
+        _printLibDetails(child, totalPath + '/', aotSizeJson);


use string concatenation '$totalPath/'

kenzieschmoll · 2020-07-29T15:46:49Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    int numBytes,
+    int level, {


make these two named and required parameters

kenzieschmoll · 2020-07-29T15:47:27Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+        _printLibDetails(child, totalPath + '/', aotSizeJson);
+      }
+    } else {
+      // Print total path and size if currentPath does not have any chilren.


nit: if currentNode does not have any children

kenzieschmoll · 2020-07-29T15:49:54Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+      if (libraryUri != null) ...libraryUri.split('/') else '@stubs',
+      if (className?.isNotEmpty ?? false) className,


i'm surprised that we are checking that className.isNotEmpty and checking that libraryUri != null. I would expect these two checks to be the same. Why are we checking for null on one and empty on the other?

kenzieschmoll · 2020-07-29T16:10:46Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+          } else if (path.endsWith('libflutter.so')) {
+            childWithPathAsName.name += ' (Flutter Engine)';
+          }
+          currentNode.addChild(childWithPathAsName);


does childWithPathAsName.addValue(pathsToSize[paths]); need to be called before currentNode.addChild(childWithPathAsName); so that the childWithPathAsName's value is summed into currentNode's value when calling addChild?

childWithPathAsName's value should already have been added to currentNode on the previous iteration of the loop.

kenzieschmoll · 2020-07-29T16:12:08Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+
+    final List<SymbolNode> sortedSymbols = root.children.toList()
+      ..sort((SymbolNode a, SymbolNode b) => b.value.compareTo(a.value));
+    for (final SymbolNode node in sortedSymbols.take(10)) {


why 10? put in a const with a descriptive name

kenzieschmoll · 2020-07-29T16:12:23Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    final List<SymbolNode> sortedSymbols = root.children.toList()
+      ..sort((SymbolNode a, SymbolNode b) => b.value.compareTo(a.value));
+    for (final SymbolNode node in sortedSymbols.take(10)) {
+      _printEntitySize(node.name, node.value, 4);


same comment here. why 4?

kenzieschmoll · 2020-07-29T16:12:42Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    Map<String, dynamic> apkAnalysisJson,
+    List<String> path, 
+    List<dynamic> aotSizeJson,


make these required named parameters

kenzieschmoll · 2020-07-29T16:13:52Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+  }
+
+  /// Adds breakdown of aot size data as the children of the node at the given path.
+  Map<String, dynamic> addAotSizeDataToJson(


rename to addAotSizeDataToApkAnalysis

xster · 2020-07-29T19:49:21Z

Note the analysis test https://github.com/flutter/flutter/pull/62495/checks?check_run_id=923824683

Also, high level UX suggestion: I think we should list all the files in devtools, but for CLI where space is limited, the various small files aren't that useful to breakdown. I suggest bundling every single file at the root that's <1kB into just one line that says Others. If users want to know, they can click into the devtool.

xster

Great start!

packages/flutter_tools/lib/src/base/analyze_size.dart

xster · 2020-07-29T19:53:46Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+import 'logger.dart';
+
+class SizeAnalyzer {
+  SizeAnalyzer(this._fileSystem, this._logger, this._processUtils);


Using required named parameters will be clearer than positional parameters here.

xster · 2020-07-29T19:54:30Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+
+  static const int tableWidth = 80;
+
+  Future<Map<String, dynamic>> analyzeApkSize({


From other PR: if this function now does 2 things, reflect it in the name.

xster · 2020-07-29T19:54:52Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    // TODO: implement Windows.
+    String unzipOut;
+    try {
+      unzipOut = (await _processUtils.run(<String>[


From other PR: using zipinfo rather than unzip could be an optimization (feel free to do it in a separate PR)

Added a TODO

packages/flutter_tools/lib/src/base/analyze_size.dart

xster · 2020-07-29T20:03:28Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    // TODO(peterdjlee): Add aot size for all platforms.
+    apkAnalysisJson = addAotSizeDataToJson(
+      apkAnalysisJson,
+      'lib/arm64-v8a/libapp.so (Dart AOT)'.split('/'), 


add code comments here for what this is doing

packages/flutter_tools/lib/src/base/analyze_size.dart

xster · 2020-07-29T20:19:48Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+  }
+}
+
+class SymbolNode {


This can probably be private. Also add comments

xster · 2020-07-29T20:22:39Z

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart

+''';
+
+  setUp(() {
+    fileSystem = MemoryFileSystem.test();


ah is this why those parameters were in the constructor? There is already a dependency injection mechanism. See other tests using testUsingContext. Then you can remove those construction params.

@jonahwilliams helped me put in those constructors explaining we might not want to use the global keyword. Would removing the construction params force us to use the global keyword?

Yes, new code should not use the global zone injection - it makes testing the code very brittle as we've found that new contributors frequently do not understand which interfaces need to be overriden.

So the new consensus is to do constructor injection through the entire graph? Is there a tracking bug to migrate existing code?

#47161 , but I haven't done a good job of keeping it updated

peterdjlee · 2020-07-30T13:40:54Z

Analysis test is failing with trailing U+0020 space character for a few lines. What does this mean? Couldn't find any documentation on it.

kenzieschmoll · 2020-07-30T17:02:31Z

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart

@@ -0,0 +1,166 @@
+// Copyright 2014 The Flutter Authors. All rights reserved.


confusingly it should be 2014

packages/flutter_tools/lib/src/base/analyze_size.dart

kenzieschmoll · 2020-07-30T17:02:52Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    }
+    return json;
+  }
+}


new line at end of file

jonahwilliams · 2020-07-30T17:07:25Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    );
+    logger.printStatus('━' * tableWidth);
+    final Directory tempApkContent = fileSystem.systemTempDirectory.createTempSync('flutter_tools.');
+    // TODO: implement Windows.


This needs a bug assigned to it

@peterdjlee

jonahwilliams · 2020-07-30T17:07:55Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+        tempApkContent.path
+      ])).stdout;
+    } on Exception catch (e) {
+      print(e);


this should be a printError and not a regular print

jonahwilliams · 2020-07-30T17:08:32Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    // 'path/to/file' where file = 1500 => pathsToSize[['path', 'to', 'file']] = 1500
+    for (final String line in const LineSplitter().convert(unzipOut)) {
+      // Expression to match 'Size' column to group 1 and 'Name' column to group 2.
+      final RegExp parseUnzipOutput = RegExp(r'^\s*\d+\s+[\w|:]+\s+(\d+)\s+.*  (.+)$');


Prefer hoisting this into a private static, to avoid recompiling the regexp every time it is created

jonahwilliams · 2020-07-30T17:09:14Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+  /// A pretty printer for an entity with a size.
+  void _printEntitySize(
+    String entityName, {
+    @required int byteSize,


formatting is off a bit here, indent the required params by 2 spaces

jonahwilliams · 2020-07-30T21:21:15Z

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart

+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+import 'package:file/memory.dart';


you need a newline here to make analysis pass

jonahwilliams · 2020-07-30T21:34:08Z

You should be able to sync to the latest master to get a correctly formatted pubspec

… generate analysis json

kenzieschmoll · 2020-07-31T14:40:08Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+import 'package:flutter_tools/src/base/process.dart';
+import 'package:flutter_tools/src/base/terminal.dart';


Do these need to be relative imports to fix the analyze bot?

Looks like other files have relative imports! Changed to relative imports.

jonahwilliams · 2020-07-31T16:47:41Z

packages/flutter_tools/lib/src/base/analyze_size.dart

+    // TODO(peterdjlee): Implement a way to unzip the APK for Windows. See issue #62603.
+    String unzipOut;
+    try {
+      // TODO(peterdjlee): Use zipinfo instead of unzip.


FYI, as far as I know we do not document requiring zipinfo to be installed, so switching may not be straightforward

both zipinfo and unzip are built-in on macOS and neither are built-in on Ubuntu so we're more or less in the same position either way.

No, we're not in the same state - we require unzip and warn if it is not installed and do no such thing for zipinfo

ah I see, SG, let's leave it then. This makes it simpler.

jonahwilliams

LGTM

jonahwilliams · 2020-07-31T17:03:54Z

As soon as https://cirrus-ci.com/task/5692958575951872 finishes this should be good to land. The g3 breakage is expected because we've added a new dependency (not in this PR but a prior one)

… generate analysis json (flutter#62495) * Implement size analyzer to unzip & parse APK and AOT size snapshot to generate analysis json

fluttergithubbot added c: contributor-productivity Team-specific productivity, code health, technical debt. tool Affects the "flutter" command-line tool. See also t: labels. labels Jul 29, 2020

googlebot added the cla: yes label Jul 29, 2020

peterdjlee requested a review from jonahwilliams July 29, 2020 15:18

kenzieschmoll reviewed Jul 29, 2020

View reviewed changes

packages/flutter_tools/lib/src/base/analyze_size.dart Show resolved Hide resolved

kenzieschmoll reviewed Jul 29, 2020

View reviewed changes

jonahwilliams reviewed Jul 29, 2020

View reviewed changes

packages/flutter_tools/test/general.shard/base/analyze_size_test.dart Show resolved Hide resolved

jonahwilliams reviewed Jul 29, 2020

View reviewed changes

kenzieschmoll reviewed Jul 29, 2020

View reviewed changes

xster reviewed Jul 29, 2020

View reviewed changes

kenzieschmoll reviewed Jul 30, 2020

View reviewed changes

packages/flutter_tools/lib/src/base/analyze_size.dart Show resolved Hide resolved

kenzieschmoll reviewed Jul 30, 2020

View reviewed changes

packages/flutter_tools/lib/src/base/analyze_size.dart Outdated

}

return json;

}

}

Copy link

Member

kenzieschmoll Jul 30, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new line at end of file

kenzieschmoll approved these changes Jul 30, 2020

View reviewed changes

jonahwilliams reviewed Jul 30, 2020

View reviewed changes

peterdjlee mentioned this pull request Jul 30, 2020

Add windows support for APK analysis #62603

Closed

jonahwilliams reviewed Jul 30, 2020

View reviewed changes

peterdjlee added 3 commits July 31, 2020 10:13

Implement size analyzer to unzip & parse APK and AOT size snapshot to…

8a740f5

… generate analysis json

Review comments

c84996d

Review comments

3018350

peterdjlee added 3 commits July 31, 2020 10:13

Review comments

afa8ae4

Add tracking issue for adding windows support

a90bbd3

Fix tests

ee9e677

peterdjlee force-pushed the size-analyzer branch from 3046f07 to ee9e677 Compare July 31, 2020 14:24

kenzieschmoll reviewed Jul 31, 2020

View reviewed changes

Use relative imports

11de74d

jonahwilliams reviewed Jul 31, 2020

View reviewed changes

jonahwilliams approved these changes Jul 31, 2020

View reviewed changes

peterdjlee merged commit 8b39af2 into flutter:master Jul 31, 2020

xster mentioned this pull request Aug 10, 2020

A rough draft for flutter build apk --analyze to show APK size breakdown #59409

Closed

github-actions bot locked as resolved and limited conversation to collaborators Aug 1, 2021


		static const int tableWidth = 80;

		Future<Map<String, dynamic>> analyzeApkSize({

		if (libraryUri != null) ...libraryUri.split('/') else '@stubs',
		if (className?.isNotEmpty ?? false) className,

		@@ -0,0 +1,166 @@
		// Copyright 2014 The Flutter Authors. All rights reserved.

		import 'package:flutter_tools/src/base/process.dart';
		import 'package:flutter_tools/src/base/terminal.dart';

Implement size analyzer to unzip & parse APK and AOT size snapshot to generate analysis json #62495

Implement size analyzer to unzip & parse APK and AOT size snapshot to generate analysis json #62495

Uh oh!

Conversation

peterdjlee commented Jul 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kenzieschmoll Jul 29, 2020 • 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!

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!

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!

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!

xster commented Jul 29, 2020

Uh oh!

peterdjlee commented Jul 29, 2020 •

edited

Loading

kenzieschmoll Jul 29, 2020 •

edited

Loading