[flutter_tools] default tree-shake-icons to enabled and improve performance by jonahwilliams · Pull Request #54923 · flutter/flutter

jonahwilliams · 2020-04-16T03:26:08Z

Description

Enable tree-shake-icons by default in profile/release mode. Fix performance by ensuring that only a single instance of const finder runs, and only on ttf fonts

Fixes #55370

dnfield

LGTM

dnfield · 2020-04-17T02:08:05Z

(once tests pass)

jonahwilliams · 2020-04-17T19:30:05Z

So the deploy gallery shard has failed twice in a row with OOM. I'm going to mark this a WIP, I need to investigate whether we're using drastically more memory with this on by default.

jonahwilliams · 2020-04-17T21:35:02Z

Running this locally I'm able to repro a large spike in memory pressure and my computer also locked up for a few seconds while running this (it doesn't for normal builds).

dnfield · 2020-04-17T21:36:40Z

Which command? Const_finder?

jonahwilliams · 2020-04-17T21:37:43Z

just by running flutter build apk --tree-shake-icons

Zazo032 · 2020-04-18T18:40:00Z

I also experience this when using this flag. For a few seconds, my machine becomes unresponsive with 100% CPU usage and a spike in used memory. Both for APK and appbundle (didn't test iOS or web).

…ms/flutter into flip_tree_shake_icon_default

jonahwilliams · 2020-04-20T15:50:05Z

+          // Only tree shake MaterialIcons-Regular.ttf font.
+          final bool isMaterialIcons = globals.fs.path
+            .basename(content.file.path) == kMaterialIconsFontName;
+          if (!isMaterialIcons || !await iconTreeShaker.subsetFont(


@dnfield fyi

But... Why? What about Cupertino icons or font awesome icins?

The intention of this routine was to only do work if the font file was used by an IconData instance. Is that not happening?

As written this was spinning up a const finder instance for every single asset. Does this actually work with arbitrary icon sets? If so we could use something like the mime package to limit it to font files (ttf only?)

The other change this would need is to limit the number of instances to 2 or 3, because each one seems to be quite heavy

Ahh ok. Yes, this should work with any font that you refernece a glyph in using IconData .. or at least any TTF.

Limiting this to 2 or 3 instances should be fine. And it wasn't meant to run on non-ttf assets.

The expectation is that this code

flutter/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart

Line 166 in 3f30b61

return false;

would return quickly, after the initial run of the const finder here:

flutter/packages/flutter_tools/lib/src/build_system/targets/icon_tree_shaker.dart

Line 161 in 3f30b61

_iconData ??= await _getIconData(_environment);

Why is the const finder running more than once? :\

I figured it out, getIconData is fairly expensive, and we might be copying multiple assets at the same time. each one that hits this line before the first instance has finished will spawn a new const finder

Ahh, yes. Ok. We should just trigger that to happen before the loop. I'm still a little confused about how multiple places are reaching this code before the first one finishes -are we missing an await somewhere?

Because the out loop is a Future.wait, not a for loop

dnfield · 2020-04-20T18:22:46Z

+    if (input.lengthSync() < 12) {
+      return false;
+    }
+    final String mimeType = mime.lookupMimeType(


How confident are you that this won't cause new problems?

I'm not confident about anything

Maybe we can just move the mime chekc to a separate pr

We might as well resolve this now if you have concerns :)

Are there non-ttf fonts that the icon_tree_shaker works with? Or does the hb subset utility have a list of supported formats?

I guess this should be fairly safe. Mime sniffing can get funny, and we could get broken if the mime package changes the spelling of a type

I'll add some more tests around this logic

More tests added. If the spelling changes there was an existing test that fails, but I also added cases to verify we don't subset invalid fonts or non-ttf mime types

jonahwilliams · 2020-04-20T18:28:55Z

I enabled this for web too, but it doesn't actually work since --cfe-only does not do any const evaluation. Instead we'll need to add a dart2js target to the frontend server and produce kernel that way.

…ake_icon_default

dnfield

LGTM

jonahwilliams · 2020-04-22T19:25:24Z

One test failure, with a really nice error message too :D

…ake_icon_default

…ve performance (#54923)" This reverts commit 0b93a92.

…ve performance (#54923)" (#55413) This reverts commit 0b93a92.

[flutter_tools] default tree-shake-icons to enabled for non-web

a2744b4

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

googlebot added the cla: yes label Apr 16, 2020

jonahwilliams added 2 commits April 16, 2020 10:57

update command tests

551d1e9

Fix build aot test

2272b97

jonahwilliams removed the work in progress; do not review label Apr 16, 2020

jonahwilliams marked this pull request as ready for review April 16, 2020 21:44

Merge branch 'master' into flip_tree_shake_icon_default

01db89c

dnfield approved these changes Apr 17, 2020

View reviewed changes

fix build aot test

b9a5111

jonahwilliams added the work in progress; do not review label Apr 17, 2020

jonahwilliams added 2 commits April 20, 2020 01:49

only tree shake material icons

4355fab

Merge branch 'flip_tree_shake_icon_default' of github.com:jonahwillia…

24fd4d9

…ms/flutter into flip_tree_shake_icon_default

jonahwilliams commented Apr 20, 2020

View reviewed changes

jonahwilliams and others added 3 commits April 20, 2020 11:10

only run const finder once, use mime type

4efb694

Delete example.dart

10d218a

Update assets.dart

4d88be2

dnfield reviewed Apr 20, 2020

View reviewed changes

jonahwilliams changed the title ~~[flutter_tools] default tree-shake-icons to enabled for non-web~~ [flutter_tools] default tree-shake-icons to enabled Apr 20, 2020

jonahwilliams mentioned this pull request Apr 22, 2020

--shrink --tree-shake-icons causes huge system load, causing CI Runners to fail #55370

Closed

willlarche mentioned this pull request Apr 22, 2020

How to use alternative themes of material icons #18384

Closed

jonahwilliams changed the title ~~[flutter_tools] default tree-shake-icons to enabled~~ [flutter_tools] default tree-shake-icons to enabled and improve performance Apr 22, 2020

jonahwilliams added 2 commits April 22, 2020 11:46

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

ca68d9a

…ake_icon_default

add more testing

3d9b929

jonahwilliams removed the work in progress; do not review label Apr 22, 2020

jonahwilliams requested a review from dnfield April 22, 2020 19:15

dnfield approved these changes Apr 22, 2020

View reviewed changes

jonahwilliams added 2 commits April 22, 2020 12:28

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

8c72011

…ake_icon_default

add missing icon dependency

3cf4c0b

jonahwilliams merged commit 0b93a92 into flutter:master Apr 22, 2020

jonahwilliams deleted the flip_tree_shake_icon_default branch April 22, 2020 20:17

jonahwilliams pushed a commit that referenced this pull request Apr 22, 2020

Revert "[flutter_tools] default tree-shake-icons to enabled and impro…

eb02a35

…ve performance (#54923)" This reverts commit 0b93a92.

jonahwilliams mentioned this pull request Apr 22, 2020

Revert "[flutter_tools] default tree-shake-icons to enabled and improve performance" #55413

Merged

jonahwilliams pushed a commit that referenced this pull request Apr 22, 2020

Revert "[flutter_tools] default tree-shake-icons to enabled and impro…

27d11da

…ve performance (#54923)" (#55413) This reverts commit 0b93a92.

jonahwilliams restored the flip_tree_shake_icon_default branch April 22, 2020 23:08

github-actions Bot locked as resolved and limited conversation to collaborators Jul 31, 2021

Conversation

jonahwilliams commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Apr 17, 2020

Uh oh!

jonahwilliams commented Apr 17, 2020

Uh oh!

jonahwilliams commented Apr 17, 2020

Uh oh!

dnfield commented Apr 17, 2020

Uh oh!

jonahwilliams commented Apr 17, 2020

Uh oh!

Zazo032 commented Apr 18, 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!

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!

jonahwilliams commented Apr 20, 2020

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

jonahwilliams commented Apr 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jonahwilliams commented Apr 16, 2020 •

edited

Loading