Teach frontend compiler to replace `toString` with `super.toString` for selected packages by dnfield · Pull Request #17068 · flutter/engine

dnfield · 2020-03-10T16:53:23Z

The idea is to eventually apply this to dart:ui and package:flutter/*. This saves about 200kb on the gallery snapshot. An audit of the codebase (and the flutter style guide) point to these methods always being used for debug only info.

dnfield · 2020-03-10T16:54:42Z

Ah. I should write some tests for this.

flutter_frontend_server/lib/server.dart

jonahwilliams

There is a script in here somewhere that populates a .packages, maybe that is working?

dnfield · 2020-03-10T21:13:50Z

Yeah, it's the python script in the folder I believe, which is getting invoked by the build process, which creates the .packages file we need.

dnfield · 2020-03-10T21:14:05Z

It seems a bit overly complex TBH, it might be good to just replace it with a hard coded .packages file.

zanderso

fyi @goderbauer

zanderso · 2020-03-11T03:15:24Z

flutter_frontend_server/lib/server.dart

+
+  @override
+  void visitProcedure(Procedure node) {
+    if (node.name.name == 'toString' && node.enclosingLibrary != null) {


nit: You can flip the sense of the ifs to lower the nesting depth.

if (node.name.name != 'toString' || node.enclosingLibrary == null) { super.visitProcedure(node); return; } assert(node.enclosingClass != null); // Turn 'dart:ui' into 'dart:ui', or // 'package:flutter/src/semantics_event.dart' into 'package:flutter'. final String packageUri = '${node.enclosingLibrary.importUri.scheme}:' '${node.enclosingLibrary.importUri.pathSegments.first}'; if (!_packageUris.contains(packageUri)) { super.visitProcedure(node); return; } node.function.replaceWith( FunctionNode( ReturnStatement( SuperMethodInvocation( node.name, Arguments(<Expression>[]), ), ), ), ); super.visitProcedure(node);

I did this slightly differently so that I don't have to do multiple returns but still dropped some nesting.

This also made me realize it's a mistake to assert enclosingClass is null - I should be checking for it instead, in case someone put a toString method at the top of their library. Added a test for that.

dnfield · 2020-03-11T04:32:53Z

/cc @alexmarkov

a14n · 2020-03-11T07:16:57Z

flutter_frontend_server/lib/server.dart

+
+  final frontend.ProgramTransformer _child;
+
+  /// A set of package URIs to apply this transformer too, e.g. 'dart:ui' and


typo: "to" ?

Yes, thanks, fixing this and the others

a14n · 2020-03-11T07:17:02Z

flutter_frontend_server/lib/server.dart

+  }
+}
+
+/// Replaces [Object.toString] overides with calls to super for the specified


typo: "overides" -> "overrides"

a14n · 2020-03-11T07:17:08Z

flutter_frontend_server/lib/server.dart

+  /// The [packageUris] must not be null.
+  ToStringVisitor(this._packageUris) : assert(_packageUris != null);
+
+  /// A set of package URIs to apply this transformer too, e.g. 'dart:ui' and


typo: "too" -> "to" ?

dnfield · 2020-03-11T15:51:19Z

I'm starting to think that this could be improved so that it reads a specific annotation, as suggested in the original issue.

I think we'd still want to avoid completely removing the method, as that gets really messy (we have to update any callsites, which might be hard to find, and it gets a lot harder to figure out what's going on if anything goes wrong).

alexmarkov · 2020-03-11T16:12:36Z

flutter_frontend_server/lib/server.dart

+      _packageUris.contains(_importUriToPackage(node.enclosingLibrary.importUri))
+    ) {
+      node.function.replaceWith(
+        FunctionNode(


We might need to verify that toString() doesn't have any extra optional parameters. Otherwise AST would be incorrect if there are calls to such toString which pass those optional parameters, but this transformation removed them.

Or maybe we can replace a node.function.body instead of replacing the whole FunctionNode.

Trying to replace node.function.body. If that works that sounds better.

Done, and updated tests.

alexmarkov · 2020-03-11T16:13:15Z

flutter_frontend_server/lib/server.dart

+  void visitProcedure(Procedure node) {
+    if (
+      node.name.name        == 'toString' &&
+      node.enclosingClass   != null       &&


We can also check !node.isStatic && !node.isAbstract

alexmarkov · 2020-03-11T16:13:53Z

flutter_frontend_server/lib/server.dart

+    if (
+      node.name.name        == 'toString' &&
+      node.enclosingClass   != null       &&
+      node.enclosingLibrary != null       &&


This check looks unnecessary: if there is an enclosing class, then there should be an enclosing library as well.

I guess I'd be more concerned about somehow getting an NPE below this (where I access enclosingLibrary.importUri) than saving the time on the extra null check.

alexmarkov · 2020-03-11T16:17:04Z

flutter_frontend_server/lib/server.dart

+        ),
+      );
+    }
+    super.visitProcedure(node);


Do we really need to go deeper? Not going beyond members might save a little bit of compilation time.
Also consider adding

@OverRide
defaultMember(Member node) {}

Ah, I missed something here. I'll ping this again when I push up the change that should fix it.

Ok, I removed the super call this time. I've also added the ability to add an @pragma annotation to a toString method so we let it through.

dnfield · 2020-03-11T16:57:30Z

I'm starting to suspect we could more safely and clearly attain these results with something like this:

const bool kReleaseMode = true;

mixin ToStringHelper {
  String debugToString() {
    return '$runtimeType';
  }
  
  @override
  String toString() {
    if (kReleaseMode) {
      return super.toString();
    }  
    return debugToString();
  }
}

class MyWidget with ToStringHelper {
  @override
  String debugToString() {
    return '$runtimeType(blah)';
  }
}

void main() {
  var w = MyWidget();
  print(w.toString());
}

We could make such a mixin in dart:ui and either export it to use in the framework or just make DiagnosticableMixin do it in the framework.

This would let someone pretty easily "opt out" if they really needed to.

dnfield · 2020-03-11T16:58:30Z

It would also let packages outside of the framework safely adopt this pattern.

dnfield · 2020-03-11T21:31:54Z

The following annotation would now make this transformer ignore a specific toString method:

@pragma('flutter_frontend:keep')
@override
String toString() => 'MyImportantToSTringEvenInReleaseMode';

alexmarkov · 2020-03-11T21:38:59Z

flutter_frontend_server/lib/server.dart

+    for (ConstantExpression expression in node.annotations.whereType<ConstantExpression>()) {
+      final InstanceConstant constant = expression.constant as InstanceConstant;
+      if (constant.classNode.name == 'pragma' && constant.classNode.enclosingLibrary.importUri.toString() == 'dart:core') {
+        final StringConstant pragmaName = constant.fieldValues.values.first as StringConstant;


constant.fieldValues.values.first doesn't look reliable as there are multiple fields which may go in an arbitrary order in constant.fieldValues map.

Aww phooey. I'll try to fix that up. My hope was that since this should be a stable order map (the contract for Map is that order remains the same), and we know the type has its first parameter as the name, this could be ok. But I'll try to do something better here.

alexmarkov · 2020-03-11T21:43:40Z

flutter_frontend_server/lib/server.dart

+
+  bool _hasKeepAnnotation(Procedure node) {
+    for (ConstantExpression expression in node.annotations.whereType<ConstantExpression>()) {
+      final InstanceConstant constant = expression.constant as InstanceConstant;


Consider testing if expression.constant is InstanceConstant, as there could be other kinds of constants.

Done, and did it for the cast below as well.

zanderso · 2020-03-16T20:54:56Z

flutter_frontend_server/lib/server.dart

+import 'package:kernel/ast.dart';
 import 'package:path/path.dart' as path;

 import 'package:vm/incremental_compiler.dart';


nit: alphabetize imports.

zanderso · 2020-03-16T20:55:58Z

flutter_frontend_server/lib/server.dart

  return completer.future;
 }
+
+/// A [RecursiveVisitor] that replaces [Object.toString] overrides with


Should this code go in a separate file/library?

I started doing that but it seemed to be overly complicating things to me - these classes are pretty short right now and the style in the engine and flutter repos tend to be to group related things together rather than splitting files.

That said I don't have a strong opinion about this one either way.

It probably makes more sense to move the transformations to (a) separate library(ies) when the second one gets added. Let's keep an eye out for that, or leave a comment/TODO.

alexmarkov

LGTM

alexmarkov · 2020-03-16T19:30:30Z

flutter_frontend_server/lib/server.dart

+        if (pragmaName is! StringConstant) {
+          continue;
+        }
+        if ((pragmaName as StringConstant).value == 'flutter_frontend:keep') {


Nit: maybe call the pragma simply 'flutter:keep' ?

@jonahwilliams @zanderso wdyt?

Could you give an example of where this annotation would go? In what situations is it needed?

class MyStringBuffer { StringBuffer _stringBuffer = StringBuffer(); // ... @override @pragma('flutter_frontend:keep') String toString() { return _stringBuffer.toString(); } }

Would prevent the transformer from overwriting the method.

Are there cases where a program would be incorrect, or the transformation would do something wrong if the annotation is not present?

If someone tried to apply this transfomer to dart:core, it would break their program - all the primitive type classes override toString in meaningful ways, as does StringBuffer.

You shouldn't do that, and we shouldn't make it easy for people to do that. The plan right now is to use this for dart:ui and package:flutter to save binary space, and to test this in a controlled internal environment with programs that have good test coverage in the modes we apply it to.

Talked offline. Going to add a type to dart:ui called keepToString.

Added annotation to dart:ui for this called keepToString, plus documentation there about this feature and how to use the annotation.

…_tostring

alexmarkov · 2020-03-17T15:25:10Z

flutter_frontend_server/lib/server.dart

+        continue;
+      }
+      final InstanceConstant constant = expression.constant as InstanceConstant;
+      if (constant.classNode.name == 'keepToString' && constant.classNode.enclosingLibrary.importUri.toString() == 'dart:ui') {


Class name is _KeepToString, not keepToString.

Ahh. I really need an integration test for this. I'll add one.

Fixed and added an integration test that would catch this in the future.

zanderso

lgtm w/ nits

zanderso · 2020-03-17T19:31:21Z

flutter_frontend_server/lib/server.dart

  return completer.future;
 }
+
+/// A [RecursiveVisitor] that replaces [Object.toString] overrides with


It probably makes more sense to move the transformations to (a) separate library(ies) when the second one gets added. Let's keep an eye out for that, or leave a comment/TODO.

zanderso · 2020-03-17T19:33:57Z

lib/ui/annotations.dart

+/// `toString` bodies with `return super.toString()` during compilation. This
+/// significantly reduces release code size, and would make it impossible to
+/// implement a meaningful override of `toString` for release mode without
+/// disabling the feature and losing the size savings. If package uses this


If package uses --> If a package uses

Done and done.

zanderso · 2020-03-17T19:34:24Z

lib/web_ui/lib/src/ui/annotations.dart

+/// `toString` bodies with `return super.toString()` during compilation. This
+/// significantly reduces release code size, and would make it impossible to
+/// implement a meaningful override of `toString` for release mode without
+/// disabling the feature and losing the size savings. If package uses this


zanderso · 2020-03-17T19:34:52Z

lib/ui/annotations.dart

+// @dart = 2.6
+part of dart.ui;
+
+/// Annotation used by Flutter's Dart compiler to indicate that an


Should the short description reference what happens in release vs. debug?

The way the flag is implemented, it's not really specific to release or debug.

When we get to a point where we'd turn this on by default in release we should probably mention something. I'll add a TODO to update the docs for this when that happens.

alexmarkov

Still lgtm.

dnfield · 2020-03-17T22:39:53Z

Filed flutter/flutter#52770 to track the infra flake, landing this on red to kick flaky infra failures.

…tring` for selected packages (flutter/engine#17068)

dnfield added 7 commits March 2, 2020 12:43

..

a3f1502

..

1463fb3

merge

f27e331

option

e2afa75

Merge remote-tracking branch 'upstream/master' into shake_tostring

ebb9b80

asserts

5c8dab9

Merge remote-tracking branch 'upstream/master' into shake_tostring

9d09a79

dnfield requested review from jonahwilliams, mraleph and zanderso March 10, 2020 16:53

auto-assign bot requested a review from gw280 March 10, 2020 16:53

googlebot added the cla: yes label Mar 10, 2020

jonahwilliams reviewed Mar 10, 2020

View reviewed changes

flutter_frontend_server/lib/server.dart Outdated Show resolved Hide resolved

dnfield added 2 commits March 10, 2020 13:52

tests

7c6fd9e

make analyzer happy

2e93bd0

jonahwilliams reviewed Mar 10, 2020

View reviewed changes

dnfield mentioned this pull request Mar 10, 2020

@debug annotation as a tree shaking hint dart-lang/sdk#35303

Closed

zanderso reviewed Mar 11, 2020

View reviewed changes

review, another edge case

ff94eed

a14n reviewed Mar 11, 2020

View reviewed changes

typos

4ebcb50

alexmarkov reviewed Mar 11, 2020

View reviewed changes

..

98f7e37

uncomment test, add keep annotation, cleanup code

af52de7

alexmarkov reviewed Mar 11, 2020

View reviewed changes

safer type checking, find key of reference

ba53656

dnfield requested review from alexmarkov and zanderso March 16, 2020 16:44

zanderso reviewed Mar 16, 2020

View reviewed changes

Alphebetize imports

01ed4bf

alexmarkov approved these changes Mar 16, 2020

View reviewed changes

dnfield added 2 commits March 16, 2020 22:31

annotation

2a8c13c

Merge branch 'shake_tostring' of github.com:dnfield/engine into shake…

9e138a3

…_tostring

alexmarkov suggested changes Mar 17, 2020

View reviewed changes

zanderso approved these changes Mar 17, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into shake_tostring

8c82d24

dnfield mentioned this pull request Mar 17, 2020

Update documentation when we make --delete-tostring-package-uri defaulted for certain modes flutter/flutter#52759

Closed

dnfield added 4 commits March 17, 2020 13:00

licenses, typos, todos

1836c92

uncomment temporarily commented out code

c452e77

Integration test, fix up class name

6b975e5

ignore fixture in analysis

79c0335

dnfield requested a review from alexmarkov March 17, 2020 21:36

alexmarkov approved these changes Mar 17, 2020

View reviewed changes

dnfield added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Mar 17, 2020

dnfield merged commit 034f913 into flutter:master Mar 17, 2020

dnfield deleted the shake_tostring branch March 17, 2020 22:40

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2020

034f913 Teach frontend compiler to replace toString with `super.toS…

1362061

…tring` for selected packages (flutter/engine#17068)

engine-flutter-autoroll mentioned this pull request Mar 18, 2020

Roll engine 083a7c2bcaaf..3a2cc9553346 (7 commits) flutter/flutter#52806

Merged

liyuqian added perf: app size Performance issues related to app size (binary/code size) severe: performance Relates to speed or footprint issues. labels Mar 19, 2020


		final frontend.ProgramTransformer _child;

		/// A set of package URIs to apply this transformer too, e.g. 'dart:ui' and

Conversation

dnfield commented Mar 10, 2020

Uh oh!

dnfield commented Mar 10, 2020

Uh oh!

Uh oh!

jonahwilliams left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 10, 2020

Uh oh!

dnfield commented Mar 10, 2020

Uh oh!

zanderso left a comment

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!

dnfield commented Mar 11, 2020

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!

dnfield commented Mar 11, 2020

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!

dnfield commented Mar 11, 2020

Uh oh!

dnfield commented Mar 11, 2020

Uh oh!

dnfield commented Mar 11, 2020

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!