Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Optimizing performance by avoiding multiple GC operations caused by multiple surface destruction notifications #43587

Merged
merged 1 commit into from
Jul 15, 2023
Merged

Optimizing performance by avoiding multiple GC operations caused by multiple surface destruction notifications #43587

merged 1 commit into from
Jul 15, 2023

Conversation

0xZOne
Copy link
Member

@0xZOne 0xZOne commented Jul 12, 2023 •
edited
Loading

Fixes: flutter/flutter#130379

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
  • I listed at least one issue that this PR fixes in the description above.
  • I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on writing and running engine tests.
  • I updated/added relevant documentation (doc comments with ///).
  • I signed the CLA.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@0xZOne 0xZOne marked this pull request as draft July 12, 2023 03:46
@0xZOne 0xZOne force-pushed the task/two_gc branch 8 times, most recently from cc6e0a6 to ec7f080 Compare July 12, 2023 09:08
@0xZOne 0xZOne marked this pull request as ready for review July 12, 2023 09:08
dnfield
dnfield approved these changes Jul 13, 2023
View reviewed changes
Copy link
Contributor

@dnfield dnfield left a comment

Choose a reason for hiding this comment

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

LGTM - it woul dbe good to have a general mechanism to debounce Dart_NotifyLowMemory and Dart_NotifyDestroyed

@chinmaygarde
Copy link
Member

From Triage: This is a sound fix but we want to debounce spammy calls to Dart_NotifyDestroyed as a whole in the runtime controller. I suggest patching RuntimeController::NotifyDestroyed in addition to this.

@0xZOne 0xZOne added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 15, 2023
@auto-submit auto-submit bot merged commit 9d78f0a into flutter:main Jul 15, 2023
27 checks passed
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 15, 2023
Closed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2023
@engine-flutter-autoroll
9d78f0a9e Optimizing performance by avoiding multiple GC operations c…
a65afa3 
…aused by multiple surface destruction notifications (flutter/engine#43587)
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Jul 15, 2023
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 15, 2023
@engine-flutter-autoroll
9d78f0a9e Optimizing performance by avoiding multiple GC operations c…
869b8b8 
…aused by multiple surface destruction notifications (flutter/engine#43587)
zanderso pushed a commit to flutter/flutter that referenced this pull request Jul 15, 2023
@engine-flutter-autoroll
Manual roll Flutter Engine from 403866d16137 to 683087731feb (16 revi…
4f437d4 
…sions) (#130666)

Manual roll requested by zra@google.com

flutter/engine@403866d...6830877

2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
DEENqWMCYI1SMYsYH... to rmzZN2ZAgpbjAFi5_... (flutter/engine#43722)
2023-07-15 zanderso@users.noreply.github.com Revert "Reland "add
non-rendering operation culling to DisplayListBuilder" (#41463)"
(flutter/engine#43721)
2023-07-15 skia-flutter-autoroll@skia.org Roll Clang from 6d667d4b261e
to ebd0b8a0472b (flutter/engine#43673)
2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from
671bbdf6c542 to 0bd185c282d2 (1 revision) (flutter/engine#43719)
2023-07-15 26625149+0xZOne@users.noreply.github.com Optimizing
performance by avoiding multiple GC operations caused by multiple
surface destruction notifications (flutter/engine#43587)
2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 975eb1250431 to
6fb535aede4e (1 revision) (flutter/engine#43716)
2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from
d1fcadf22aad to 671bbdf6c542 (2 revisions) (flutter/engine#43715)
2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
Z-1lzZAOYHvVrdjQ8... to oeYLDNShuD-FTgGwU... (flutter/engine#43714)
2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 271b2b6d5aaa to
975eb1250431 (2 revisions) (flutter/engine#43712)
2023-07-15 goderbauer@google.com Move ViewConfiguration ownership to
FlutterView (flutter/engine#43701)
2023-07-14 yjbanov@google.com [web] always add secondary role managers
(flutter/engine#43663)
2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again,
this time with perspective when a save layer is involved
(flutter/engine#43695)
2023-07-14 flar@google.com Reland "add non-rendering operation culling
to DisplayListBuilder" (#41463) (flutter/engine#43698)
2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to
271b2b6d5aaa (2 revisions) (flutter/engine#43706)
2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color
attachment 0u does not cause crash in embedder API
(flutter/engine#43705)
2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to
5f6578398870 (4 revisions) (flutter/engine#43704)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from DEENqWMCYI1S to rmzZN2ZAgpbj
  fuchsia/sdk/core/mac-amd64 from Z-1lzZAOYHvV to oeYLDNShuD-F

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@0xZOne
Copy link
Member Author

0xZOne commented Jul 16, 2023

From Triage: This is a sound fix but we want to debounce spammy calls to Dart_NotifyDestroyed as a whole in the runtime controller. I suggest patching RuntimeController::NotifyDestroyed in addition to this.

Well, the strategy of performing GC when a FlutterView detaches from the engine is not friendly for scenarios where multiple FlutterViews reuse the same engine, which may slow down the initial display of pages. It would be even better if there is an interface provided to control whether this GC is enabled. :)

@yeran1212 yeran1212 mentioned this pull request Jul 19, 2023
Closed
harryterkelsen pushed a commit to harryterkelsen/engine that referenced this pull request Jul 20, 2023
@0xZOne @harryterkelsen
Optimizing performance by avoiding multiple GC operations caused by m…
fc5b32b 
…ultiple surface destruction notifications (flutter#43587)

Fixes: flutter/flutter#130379

[C++, Objective-C, Java style guides]: https://github.com/flutter/engine/blob/main/CONTRIBUTING.md#style
LouiseHsu pushed a commit to LouiseHsu/flutter that referenced this pull request Jul 31, 2023
@engine-flutter-autoroll @LouiseHsu
Manual roll Flutter Engine from 403866d16137 to 683087731feb (16 revi…
522e358 
…sions) (flutter#130666)

Manual roll requested by zra@google.com

flutter/engine@403866d...6830877

2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Linux SDK from
DEENqWMCYI1SMYsYH... to rmzZN2ZAgpbjAFi5_... (flutter/engine#43722)
2023-07-15 zanderso@users.noreply.github.com Revert "Reland "add
non-rendering operation culling to DisplayListBuilder" (flutter#41463)"
(flutter/engine#43721)
2023-07-15 skia-flutter-autoroll@skia.org Roll Clang from 6d667d4b261e
to ebd0b8a0472b (flutter/engine#43673)
2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from
671bbdf6c542 to 0bd185c282d2 (1 revision) (flutter/engine#43719)
2023-07-15 26625149+0xZOne@users.noreply.github.com Optimizing
performance by avoiding multiple GC operations caused by multiple
surface destruction notifications (flutter/engine#43587)
2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 975eb1250431 to
6fb535aede4e (1 revision) (flutter/engine#43716)
2023-07-15 skia-flutter-autoroll@skia.org Roll Dart SDK from
d1fcadf22aad to 671bbdf6c542 (2 revisions) (flutter/engine#43715)
2023-07-15 skia-flutter-autoroll@skia.org Roll Fuchsia Mac SDK from
Z-1lzZAOYHvVrdjQ8... to oeYLDNShuD-FTgGwU... (flutter/engine#43714)
2023-07-15 skia-flutter-autoroll@skia.org Roll Skia from 271b2b6d5aaa to
975eb1250431 (2 revisions) (flutter/engine#43712)
2023-07-15 goderbauer@google.com Move ViewConfiguration ownership to
FlutterView (flutter/engine#43701)
2023-07-14 yjbanov@google.com [web] always add secondary role managers
(flutter/engine#43663)
2023-07-14 dnfield@google.com [Impeller] Fix text scaling issues again,
this time with perspective when a save layer is involved
(flutter/engine#43695)
2023-07-14 flar@google.com Reland "add non-rendering operation culling
to DisplayListBuilder" (flutter#41463) (flutter/engine#43698)
2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 5f6578398870 to
271b2b6d5aaa (2 revisions) (flutter/engine#43706)
2023-07-14 jonahwilliams@google.com [Impeller] Ensure that missing color
attachment 0u does not cause crash in embedder API
(flutter/engine#43705)
2023-07-14 skia-flutter-autoroll@skia.org Roll Skia from 315c7f08c731 to
5f6578398870 (4 revisions) (flutter/engine#43704)

Also rolling transitive DEPS:
  fuchsia/sdk/core/linux-amd64 from DEENqWMCYI1S to rmzZN2ZAgpbj
  fuchsia/sdk/core/mac-amd64 from Z-1lzZAOYHvV to oeYLDNShuD-F

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC rmistry@google.com,zra@google.com on the revert to ensure that
a human
is aware of the problem.

To file a bug in Flutter:
https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://bugs.chromium.org/p/skia/issues/entry?template=Autoroller+Bug

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnfield dnfield dnfield approved these changes

@jason-simmons jason-simmons jason-simmons approved these changes

Assignees
No one assigned
Labels
autosubmit Merge PR when tree becomes green via auto submit App platform-android
Projects
None yet
Milestone
No milestone
4 participants
@0xZOne @chinmaygarde @dnfield @jason-simmons