Const finder missing `static const` list/map/set fields. #16896

dnfield · 2020-03-02T22:31:30Z

dnfield · 2020-03-02T22:31:49Z

tools/const_finder/lib/const_finder.dart


  bool _matches(Class node) {
-    return node.enclosingLibrary.canonicalName.name == classLibraryUri &&
+    return node.enclosingLibrary.importUri.toString() == classLibraryUri &&


This is more correct than using canonicalName, per @alexmarkov offline discussion.

dnfield · 2020-03-02T22:32:06Z

tools/const_finder/lib/const_finder.dart

+    if (_cache.add(node)) {
+      super.defaultConstant(node);
+    }


Avoids combinatorial explosion when viewing deeply nested constants.

dnfield · 2020-03-02T22:32:20Z

tools/const_finder/lib/const_finder.dart

+
+  @override
+  void defaultConstantReference(Constant node) {
+    defaultConstant(node);


This is the actual critical change for this fix.

Putting some comments here as it looks like github doesn't allow me to attach them to the corresponding lines.

We should probably also visit all children recursively in visitInstanceConstantReference, e.g. consider replacing L71 with

super.visitInstanceConstantReference(node);

and removing L73. This will cover cases like an InstanceConstant which has ListConstant in its field which has InstanceConstant in its element.

Nit: typo in _visitedInstnaces

Consider replacing kvp.key.canonicalName.name at L82 with kvp.key.asField.name.name.

zanderso · 2020-03-02T23:03:03Z

tools/const_finder/test/fixtures/lib/consts.dart

 }

+class StaticConstInitializer {
+  static const List<Target> targets = <Target>[


Consider adding a test with deeper nesting to test the caching optimization.

@alexmarkov - can you help suggest a test for this, or point me to one in the Kernel compiler?

I'm not entirely sure what we'd test here either - we don't have benchmarks for this and I'm not sure we want to set them up, and otherwise I guess we could expose number of visits done, but that puts us at the mercy up upstream changes in the kernel pretty tightly...

We have Dart tests for such cases:

https://github.com/dart-lang/sdk/blob/master/tests/language_2/canonicalize/hashing_memoize_array_test.dart
https://github.com/dart-lang/sdk/blob/master/tests/language_2/canonicalize/hashing_memoize_instance_test.dart

Traversal of such constants without caching will time out for any reasonable time limit.

Perfect, thanks - I took one of those and basically copied it into here with slight modifications for Flutter style.

alexmarkov

LGTM w/nits.

alexmarkov · 2020-03-02T22:52:39Z

tools/const_finder/lib/const_finder.dart

+
+  @override
+  void defaultConstantReference(Constant node) {
+    defaultConstant(node);


Putting some comments here as it looks like github doesn't allow me to attach them to the corresponding lines.

We should probably also visit all children recursively in visitInstanceConstantReference, e.g. consider replacing L71 with

super.visitInstanceConstantReference(node);

and removing L73. This will cover cases like an InstanceConstant which has ListConstant in its field which has InstanceConstant in its element.

Nit: typo in _visitedInstnaces

Consider replacing kvp.key.canonicalName.name at L82 with kvp.key.asField.name.name.

dnfield · 2020-03-03T00:46:23Z

Going to land this on red to kick infra that flaked on a gsutil upload

Also, the web failure has nothing to do with this change - /cc @yjbanov FYI, felt tests failed on this change.

dnfield · 2020-03-03T00:48:21Z

(I will wait for the build_and_test_linux_unopt_debug to pass, that one is relevant to this change)

yjbanov · 2020-03-03T00:59:03Z

Thanks for the heads-up. build_and_test_linux_unopt_debug runs all of the same tests as the failing Linux Web Engine does. The problem must be with LUCI infra. The 10-second timeout is probably too tight for screenshot tests given that we have to clone the goldens repo. We'll bump that up to a couple of minutes (I'll file an issue). The golden comparison failure looks more concerning. We'll have to look into it too, if it happens again without the timeout.

…tter/engine#16896)

* 38f497c Roll src/third_party/skia 9dd0bd78b2d7..470f0637aeea (11 commits) (flutter/engine#16887) * 5073cc7 Roll src/third_party/dart fbe9f6115d2f..0b819161d778 (3 commits) (flutter/engine#16888) * df94213 Revert "Try rasterizing images and layers only once , even when their rasterization fails. Further enforce the same access threshold on layers as on Pictures. Previously layers would always be cached. The latter is a semantic change. (#16545)" (flutter/engine#16889) * b230adb Roll src/third_party/skia 470f0637aeea..ded437003d0e (6 commits) (flutter/engine#16891) * 63bdf3e Roll fuchsia/sdk/core/mac-amd64 from q2DAy... to WmA2M... (flutter/engine#16892) * e54f2c8 Const finder missing `static const` list/map/set fields. (flutter/engine#16896) * 10275fe Roll fuchsia/sdk/core/linux-amd64 from 9NHsJ... to uiAI5... (flutter/engine#16893) * 8d046fa Roll src/third_party/skia ded437003d0e..b43cfa4d3f96 (7 commits) (flutter/engine#16894) * 9c53993 Roll src/third_party/dart 0b819161d778..ca3ad264a649 (18 commits) (flutter/engine#16898) * 6991dd9 Roll src/third_party/skia b43cfa4d3f96..8121d27b297c (10 commits) (flutter/engine#16899)

dnfield added 10 commits January 21, 2020 18:53

Fix issues in const_finder

09240e4

test

6764323

void return

2f7c065

more

b3a6d4a

test more cases

3f869e2

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

61b695b

test pkg

0d6040d

..

340710f

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

15e58b5

Fix const finder for static const map/list/set

038221c

dnfield requested review from alexmarkov and zanderso March 2, 2020 22:31

auto-assign bot requested a review from gw280 March 2, 2020 22:31

googlebot added the cla: yes label Mar 2, 2020

dnfield commented Mar 2, 2020

View reviewed changes

zanderso reviewed Mar 2, 2020

View reviewed changes

alexmarkov approved these changes Mar 2, 2020

View reviewed changes

dnfield added 2 commits March 2, 2020 15:45

review

76e00a2

test for insane recursion

e2d4980

dnfield mentioned this pull request Mar 3, 2020

[flutter_tools] supports tree-shake-icons for web builds flutter/flutter#51808

Merged

dnfield merged commit e54f2c8 into flutter:master Mar 3, 2020

dnfield deleted the const_finder_fixes branch March 3, 2020 00:56

yjbanov mentioned this pull request Mar 3, 2020

Increase Web engine screenshot test timeout flutter/flutter#51811

Closed

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

e54f2c8 Const finder missing static const list/map/set fields. (flu…

22df039

…tter/engine#16896)

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

Roll engine 5e474ee860a3..e54f2c8c1c3d (6 commits) flutter/flutter#51813

Closed

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

e54f2c8 Const finder missing static const list/map/set fields. (flu…

326f2d1

…tter/engine#16896)

This was referenced Mar 3, 2020

Roll engine 5e474ee860a3..8d046faf95b3 (8 commits) flutter/flutter#51818

Closed

Roll engine 5e474ee860a3..6991dd981bf7 (10 commits) flutter/flutter#51821

Merged

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

e54f2c8 Const finder missing static const list/map/set fields. (flu…

2739f93

…tter/engine#16896)

Const finder missing static const list/map/set fields. #16896

Const finder missing static const list/map/set fields. #16896

Uh oh!

Conversation

dnfield commented Mar 2, 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!

alexmarkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield commented Mar 3, 2020

Uh oh!

dnfield commented Mar 3, 2020

Uh oh!

yjbanov commented Mar 3, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Const finder missing `static const` list/map/set fields. #16896

Const finder missing `static const` list/map/set fields. #16896