Flutter Widget Inspector doesn't work with new kernel const format

[vsmenon]

The flutter widget inspector transform expects to be able to access the original const constructors. It can't do that with new consts.

Per @mraleph : Flutter 3xHEAD started failing exactly at the enabling commit. [https://ci.chromium.org/p/dart/builders/ci.sandbox/flutter-engine-linux/2068]

@jacob314 is investigating if there is a way to proceed.

[askeksa-google]

We discussed this on the front-end team, and here's our proposal for fixing it:

• Add a flag to the front-end CompilerOptions which will make the constant evaluator leave constructor invocations unevaluated (as constructor invocations, rather than the InstanceCreation nodes they are usually translated into when unevaluated).
• Enable the flag when compiling code that is to be transformed by the Wdiget transformer.
• Instantiate a constant evaluator in the Widget transformer.
• Make the Widget transformer recurse into unevaluated constants to find (and transform) the constructor invocations inside. When it comes back out, it will call its constant evaluator to evaluate the constant. It needs to do so for all constants (not just the unevaluated ones) to do correct canonicalization of all constants.
• Also explicitly evaluate all new constants created by the Widget transformer (if they are not already inside a constant).

@jacob314 what do you think of this plan? It means the front-end run will need to know whether this transformation is happening later, so those two steps can't run independently. Does this fit how the compilation process in the Widget Inspector works already?

[jacob314]

Can you describe the benefits of this plan relative to running the Widget Inspector transform before any const evaluation?

What do you expect the performance impact of this proposal will be? The kernel transformer tracking widget creation locations needs to be fast as it is run as part of all debugging runs of flutter applications initiated by IDEs.

If I'm understanding it correctly consts like

const [1,2,3]
const {'a': 3, 'b': 5}

would still be evaluated before the widget transformer
but consts such as

const Text('foo')

and

const [Text('foo'), Text('bar)]

would not be evaluated. Is that correct?

[askeksa-google]

The overall issue with running the transformer before constant evaluation is that the CFE is, by design, a black-box translation from Dart to Kernel, and running external transformations in the middle of the translation exposes internal implementation details that are not meant to be part of the public API, which constrains the ability of the CFE to change its implementation in the future.

Specifically, in this case:

• The fact that constant evaluation runs as a separate transformation is just an implementation detail. This is not the most efficient way to do it (mostly for cache reasons), and we would like to optimize this going forward.
• Constant evaluation is responsible for some internal desugarings (e.g. for UI-as-code constructs), which means that before constant evaluation, there will be nodes in the tree that are internal to the CFE and not understandable by any external transformations.
• In a separate compilation scenario (i.e. Flutter Web), some component might not refer to the Widget class at all, which means it does not show up in the ClassHierarchy. As I understand it, this was the issue that was hit during the flag flip just before Easter. It might be possible to work around this somehow, but that will put even more constraints on the internal workings of the CFE.

As for the performance: the constant evaluations themselves will be work that is just shifted from the first constant evaluation, so those on their own should not make much difference.

What could make a difference is that the constants need to be re-canonicalized. The constants produced by the constructor invocations (transformed or not) can never hit constants not containing constructor invocations, but the Widget transformer creates some new constants (representing the positions) that might collide with existing constants. Thus, all constants must be revisited to be considered for canonicalization.

We have plans to make the canonicalization more flexible, for instance to support re-use of the canonicalization cache, which would negate this overhead, canonicalizing every constant just once in any case.

Your examples of evaluated and not evaluated constants are correct. The unevaluated ones will still be wrapped in ConstantExpression nodes with UnevaluatedConstant children. This makes the points at which to perform constant evaluation easy to recognize.

[jacob314]

Ok this seems workable. Let me know when you have it working enough end to end to test the performance. If there are performance issues, another option is to use a simpler scheme for the location ids. For example, I suspect we would get a performance win by switching to integer ids and potentially providing a separate server side API to convert between the integer ids and actual locations instead of storing the complex const Location objects. If need to go that route we will need some lead time as it will require non-trivial changes to Flutter.

[jacob314]

High level, the existing tests in package:flutter will validate correctness and the time delta running

flutter test
vs
flutter test --track-widget-creation

will gate whether the performance is good enough to ship. In general the time running with --track-widget-creation should be no more than 20% slower than without the flag and it would be very valuable if we could reduce the slowdown further.

[jacob314]

To follow up based on offline discussions:
The main blocker to running the transform earlier in the pipline is the ClassHierarchy. The good news is that we want to stop using the ClassHierachy in the Kernel transformer anyway to improve performance.

Prototype CL removing the class hierarchy.
https://dart-review.googlesource.com/c/sdk/+/100408

[kmillikin]

Another blocker for running the widget inspector transformation before constant evaluation is that unevaluated constants contain Fasta-internal syntax that the Kernel transformation APIs can't handle. I've marked this as blocked on solving that isssue.

[askeksa-google]

Support for running the widget transformer before constant evaluation has landed in https://dart-review.googlesource.com/c/sdk/+/101987. It is already enabled for Flutter Web in DDC (and the old calls to the transformer removed).

For Flutter, it is controlled by the new trackWidgetCreation named parameter to the FlutterTarget constructor. This is off by default and currently not enabled by any callers. Flutter should set this flag to true where appropriate and remove existing calls to the transformer.

[devoncarew]

A CL is out for review here: https://dart-review.googlesource.com/c/sdk/+/102920.

@jacob314 may be OOO now; if so, I believe that @terrylucas will take over that CL. I'll confirm.

[jacob314]

That CL will fix the issue with a performance improvement instead of a regression.

…

On Tue, May 21, 2019, 8:52 AM Devon Carew ***@***.***> wrote: A CL is out for review here: https://dart-review.googlesource.com/c/sdk/+/102920. @jacob314 <https://github.com/jacob314> may be OOO now; if so, I believe that @terrylucas <https://github.com/terrylucas> will take over that CL. I'll confirm. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36640?email_source=notifications&email_token=AAJLQPBNTVPSZEW4537ECGLPWQLEFA5CNFSM4HGDS6Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV4LMHA#issuecomment-494450204>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJLQPCLFKGV7NHO24RG5A3PWQLEFANCNFSM4HGDS6QQ> .

[askeksa-google]

This PR enables the new transformation in Flutter: flutter/engine#8994

[aadilmaan]

@terrylucas Assigned this to you for completing the remaining work.

[dgrove]

@terrylucas - can you please update?

[terrylucas]

I committed Jacobs CL and updated with when and what happened.

[dgrove]

Is there anything remaining on this issue, @terrylucas ?

[terrylucas]

Not that I'm aware of Travis tests passed, Flutter engine works and DevTools inspector works as well.

…

On Tue, May 28, 2019 at 8:45 AM Dan Grove ***@***.***> wrote: Is there anything remaining on this issue, @terrylucas <https://github.com/terrylucas> ? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36640?email_source=notifications&email_token=AAPV5QOYSGQ2APXGL72Z5O3PXVHRLA5CNFSM4HGDS6Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWMR2VA#issuecomment-496573780>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPV5QL6QX6I3QEKDENQFW3PXVHRLANCNFSM4HGDS6QQ> .

[terrylucas]

Jacob's CLs are committed to Dart SDK and Flutter.

[askeksa-google]

Reopening since the Flutter change was reverted due to a test failure (most likely an error in the test): flutter/engine#9134

[dgrove]

@terrylucas any updates on this?

[terrylucas]

Aske is there anything in the CFE/kernel transformer AST that might be in the dill file (or something that's not computed until there is a dill file? As I run the failing test (first time the dill file is deleted) the tests will fail. After this failure all tests pass. As I debug the tests, when there's success, the location object is the file name of the widget that was created. Vijay mentioned there a dill dump tool - I'm going to look at the dump. If you think it's interesting to see this dill file I'll attach it too.

…

On Wed, Jun 5, 2019 at 8:24 AM Dan Grove ***@***.***> wrote: @terrylucas <https://github.com/terrylucas> any updates on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#36640?email_source=notifications&email_token=AAPV5QNITPVSG5E7HDAEJV3PY7LBVA5CNFSM4HGDS6Q2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXACFVI#issuecomment-499131093>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAPV5QKCFX6BSIMNRZ3DNW3PY7LBVANCNFSM4HGDS6QQ> .

[terrylucas]

====================================
Run #1

Flutter test run w/o --track-widget-creation
cd flutter/packages/flutter
flutter test

inside of widget_inspector_test.dart the WidgetInspectorService's isWidgetCreationTracked() is false therefore this test is not compiled w/ track-widget-creation from the kernel transformer point of view.

====================================
Run #2

flutter test --track-widget-creation

inside of the widget_inspector_test.dart the WidgetInspectorService's isWidgetCreationTracked() is true so this test should have been compiled w/ track-widget-creation

There is a dill file generated testfile.dill.track.dill however the test fails because no creationLocation entry can be found (at the top-level same level as children) the getSelectedWidget JSON payload is:

  description: Text,
  type: _DiagnosticableTreeNode,
  style: dense,
  hasChildren: true,
  allowWrap: false,
  objectId: inspector-0,
  valueId: inspector-1,
  widgetRuntimeType: Text,
  children: [
    {description: RichText,
    type: _DiagnosticableTreeNode,
    style: dense,
    allowWrap: false,
    objectId: inspector-2,
    valueId: inspector-3,
    widgetRuntimeType: RichText,
    locationId: 1,
    creationLocation: {
      file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/lib/src/widgets/text.dart,
      line: 386,
      column: 21,
      parameterLocations: [
        {file: null, line: 387, column: 7, name: textAlign},
        {file: null, line: 388, column: 7, name: textDirection},
        {file: null, line: 389, column: 7, name: locale},
        {file: null, line: 390, column: 7, name: softWrap},
        {file: null, line: 391, column: 7, name: overflow},
        {file: null, line: 392, column: 7, name: textScaleFactor},
        {file: null, line: 393, column: 7, name: maxLines},
        {file: null, line: 394, column: 7, name: strutStyle},
        {file: null, line: 395, column: 7, name: textWidthBasis},
        {file: null, line: 396, column: 7, name: text}]},
        children: []}
      ]}

====================================
Run #3

When the dill file is generated all run with --track-widget-creation works, a dump of the dill file shows that creationLocation field exist so the tests all run successfully e.g.,

  final wid::_Location creationLocation = wid::_getCreationLocation(value);

Notice after the dill is generated there is a creationLocation (at the same level as children prefix with **)

description: Text,
type: _DiagnosticableTreeNode,
style: dense,
hasChildren: true,
allowWrap: false,
objectId: inspector-0, 
valueId: inspector-1, 
widgetRuntimeType: Text, 
locationId: 10, 

** creationLocation: {file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/test/widgets/widget_inspector_test.dart,
  line: 803,
  column: 15, 
  parameterLocations: [
    {file: null,
     line: 803, 
     column: 20, 
     name: data}
  ]
},

children: [
  {description: RichText, 
  type: _DiagnosticableTreeNode,
  style: dense,
  allowWrap: false,
  objectId: inspector-2,
  valueId: inspector-3,
  widgetRuntimeType: RichText,
  locationId: 4,
  creationLocation: {
    file: file:///Users/terry/flutter-engine2-git/flutter/packages/flutter/lib/src/widgets/text.dart,
    line: 386,
    column: 21,
    parameterLocations: [
      {file: null, line: 387, column: 7, name: textAlign},
      {file: null, line: 388, column: 7, name: textDirection},
      {file: null, line: 389, column: 7, name: locale},
      {file: null, line: 390, column: 7, name: softWrap},
      {file: null, line: 391, column: 7, name: overflow},
      {file: null, line: 392, column: 7, name: textScaleFactor},
      {file: null, line: 393, column: 7, name: maxLines},
      {file: null, line: 394, column: 7, name: strutStyle},
      {file: null, line: 395, column: 7, name: textWidthBasis},
      {file: null, line: 396, column: 7, name: text}]}, children: []}
    ]}

[terrylucas]

Sent mail to Aske seems like an issue with the CFE either race condition of some fixup of the in memory data is not the same until after a dill is generated. The ** creationLocation doesn't exist first time test if run.

[terrylucas]

Talked to Aske he suggested - ” Dmitry found a bug in the transformer which resulted in an invalid override. It showed in connection with the new top-level type inference that he is working on landing, but it could have implications for the old one as well. Try patching in this CL (without its dependencies) and see if that changes anything: https://dart-review.googlesource.com/c/sdk/+/105245 ”.

Terry will try patching Dart and rolling a new Flutter with the patched Dart (will bring in Ben on how to do).

[askeksa-google]

@stefantsov's transformer fix has now landed i the SDK, so it should be sufficient to just use the newest SDK head to test it against the Flutter change.

[aadilmaan]

I presume Terry will verify this transformer fix.

[terrylucas]

Tried Dmitry's Dart PR re-built the flutter engine This didn't fix the problem same results. Sent mail to Aske on other suggestions or areas where I should look? I’ll drill into Widget Inspector Transformer in kernel to see if I can locate the problem.

[terrylucas]

Found the problem, with Jacob's help, CL
Test still run essentially the same with or without --track-widget-creation, the transformer overhead negligible.

[aadilmaan]

We are picking up the current dev release as release candidate to begin align with Flutter. We are out of time for D24. This will have to move to D25.

[terrylucas]

Fix is checked into tip of Dart. Locally rebuilding Flutter engine, applying Jacob's engine change. If all works will have Ben roll Dart into Flutter and create an engine build. Then I can check Jacob's original PR into tip of Flutter's engine and the Flutter's bot should stay green.

[terrylucas]

Flutter transformer PR is checked in to use the new kernel transformer and the Flutter bots remained green.