Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

Disable LTO in builds of CanvasKit to reduce binary size #40733

Merged
merged 1 commit into from
Mar 29, 2023
Merged

Disable LTO in builds of CanvasKit to reduce binary size #40733

merged 1 commit into from
Mar 29, 2023

Conversation

jason-simmons
Copy link
Member

No description provided.

harryterkelsen
harryterkelsen approved these changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@harryterkelsen harryterkelsen left a comment

Choose a reason for hiding this comment

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

LGTM

mdebbar
mdebbar approved these changes Mar 29, 2023
View reviewed changes
third_party/canvaskit/BUILD.gn
@@ -15,6 +15,9 @@ wasm_toolchain("canvaskit") {
skia_use_libjpeg_turbo_decode = true
skia_use_libpng_decode = true
skia_use_libwebp_decode = true

# Disable LTO.
enable_lto = false
Copy link
Contributor

Choose a reason for hiding this comment

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

If this is going to be the same for all CanvasKit builds, you can also add it to tools/gn. But this is good too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Adding it to tools/gn would conflict with skwasm, which must always be built with LTO enabled

eyebrowsoffire
eyebrowsoffire approved these changes Mar 29, 2023
View reviewed changes
Copy link
Contributor

@eyebrowsoffire eyebrowsoffire left a comment

Choose a reason for hiding this comment

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

LGTM!

@kevmoo
Copy link
Contributor

kevmoo commented Mar 29, 2023

@jason-simmons – we're are super confident that

  1. this is a solid size win (how much?)
  2. there are ~zero perf regressions?

@jason-simmons
Copy link
Member Author

After disabling LTO I saw a 13% reduction in the size of canvaskit.wasm in my local wasm_release build. I did not notice significant performance differences in local runs of web_benchmarks_canvaskit when disabling LTO.

I can wait a day before landing this PR in order to get more runs of the benchmarks on the Skia dashboard with -O3 and LTO. After that we can check if the dashboard reports any change from disabling LTO.

@jason-simmons
Copy link
Member Author

Also, the builds of CanvasKit that we had previously obtained from Skia were not built with LTO.

@jason-simmons jason-simmons mentioned this pull request Mar 29, 2023
Closed
@kevmoo kevmoo added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 29, 2023
@auto-submit auto-submit bot merged commit 4a25940 into flutter:main Mar 29, 2023
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Mar 29, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 29, 2023
@engine-flutter-autoroll
4a25940de Disable LTO in builds of CanvasKit to reduce binary size (f…
9765ebb 
…lutter/engine#40733)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mdebbar mdebbar mdebbar approved these changes

@harryterkelsen harryterkelsen harryterkelsen approved these changes

@eyebrowsoffire eyebrowsoffire eyebrowsoffire approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jason-simmons @kevmoo @mdebbar @harryterkelsen @eyebrowsoffire