Skip to content

Inline some element methods for better stack overflow affinity #78269

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 29, 2021

Conversation

goderbauer
Copy link
Member

@goderbauer goderbauer commented Mar 16, 2021

Fixes #73730.

Locally, this reduced the stack_size benchmark by 30% in release mode at the expense of slightly higher binary size. Example binary increase for the Gallery app on android-arm64:

  • libapp.so (the Dart AOT) increased by 53.9 KB (+0.2%) uncompressed
  • the overall APK increased by 15.0 KB (+0.02%) compressed

A local spot check with the gallery transition perf benchmark did not reveal any notable changes in build performance.

Last, but not least, this allows developers to nest more widget before facing a stack overflow. In a test setup, an additional ~900 containers could be nested before a stack overflow occurred.

See also #73730 for details on the points discussed above.

@flutter-dashboard flutter-dashboard bot added the framework flutter/packages/flutter repository. See also f: labels. label Mar 16, 2021
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@google-cla google-cla bot added the cla: yes label Mar 16, 2021
@goderbauer
Copy link
Member Author

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

The test for this is the stack_size benchmark: https://flutter-flutter-perf.skia.org/e/?queries=sub_result%3Dstack_size_per_nesting_level.

@goderbauer goderbauer requested review from tvolkert and Hixie March 16, 2021 00:41
@Hixie
Copy link
Contributor

Hixie commented Mar 16, 2021

Does this affect the stack traces in error messages in any significant way?
What about the logic for folding stack traces?

@goderbauer
Copy link
Member Author

This change is transparent to stack traces. The VM already inlines methods based on internal heuristics without affecting stack traces and we just tell it to be more aggressive in inlining these annotated methods.

@goderbauer
Copy link
Member Author

Updated PR description with some more information on the size increase.

Copy link
Contributor

@tvolkert tvolkert left a comment

Choose a reason for hiding this comment

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

LGTM

@fluttergithubbot fluttergithubbot merged commit 5231a7d into flutter:master Mar 29, 2021
@goderbauer goderbauer deleted the inline branch March 3, 2023 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider inlining render pipeline methods to reduce stack overflow affinity
4 participants