Cleanup the IO thread GrContext #14265

liyuqian · 2019-12-09T20:17:09Z

Fixes flutter/flutter#19558

This is tested by the devicelab test fast_scroll_large_images__memory

dnfield · 2019-12-09T20:21:03Z

flow/skia_gpu_object.cc

@@ -39,6 +43,11 @@ void SkiaUnrefQueue::Drain() {
  for (SkRefCnt* skia_object : skia_objects) {
    skia_object->unref();
  }
+
+  if (context_ != nullptr) {
+    TRACE_EVENT0("flutter", "SkiaUnrefQueue::Drain performDeferredCleanup");


Do we want to trace the duration of this?

Did you work out whatever issues were causing this to take 16ms+?

I can remove that. I just figured out that our Drain task has already had a throttling of once every 3 seconds so a dozen milliseconds of cleanup is fine. The speed difference is due to the difference between the precompiled debug engine and my local debug engine (precompiled probably has opt while local one has unopt build). Switching to profile would eliminate the difference...

I'm not concerned with removing it :) But this seems like a nice candidate for TRACE_DURATION rather than TRACE_EVENT0.

The waiting time would give our cleanup more room to finish the job, and thus to make our result more accurate. See also flutter/engine#14265

Fixes flutter/flutter#19558 This is tested by the devicelab test fast_scroll_large_images__memory

chinmaygarde

Minor nit but otherwise LGTM.

chinmaygarde · 2019-12-09T20:40:21Z

flow/skia_gpu_object.cc

@@ -39,6 +43,11 @@ void SkiaUnrefQueue::Drain() {
  for (SkRefCnt* skia_object : skia_objects) {
    skia_object->unref();
  }
+
+  if (context_ != nullptr) {


Nit: The unref queue drains all objects after drain_delay_ but the task is posted on every GPU object unref. So it is possible to for this method to not actually unref any objects because they were cleared on a previous unref. So its good to only perform deferred cleanup if skia_objects.size() > 0.

Do we track SkImage objects in this queue? I thought this was only for GPU context objects?

Yes. SkImages are GPU context related. We also track other objects that may themselves reference SkImages as well (like pictures).

Otherwise, the IOManager may be destructed before the Drain is called and that could cause a crash.

liyuqian · 2019-12-10T00:52:46Z

@chinmaygarde : please check the thread issue that I discussed with you earlier in d536937

flutter/engine@12bf95f...db60ebc git log 12bf95f..db60ebc --first-parent --oneline 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from h4iiT... to otkJA... (flutter/engine#14327) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart bd008dd1e406..d9fa37e85d5c (1 commits) (flutter/engine#14325) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 5afc7b2af854..75799967be60 (2 commits) (flutter/engine#14324) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart bcd18e67dcae..bd008dd1e406 (3 commits) (flutter/engine#14322) 2019-12-11 chinmaygarde@google.com Verify accounting for loop counts in Gif and WebP assets is consistent. (flutter/engine#14321) 2019-12-11 iska.kaushik@gmail.com [fuchsia] Do not Execute paint tasks when there is no vsync (flutter/engine#14298) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/dart c74a8ec2c46e..bcd18e67dcae (9 commits) (flutter/engine#14317) 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from nqJnP... to UdfLO... (flutter/engine#14316) 2019-12-11 skia-flutter-autoroll@skia.org Roll src/third_party/skia 732c49739fa5..5afc7b2af854 (16 commits) (flutter/engine#14315) 2019-12-11 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/mac-amd64 from 9C6UA... to h4iiT... (flutter/engine#14314) 2019-12-11 iska.kaushik@gmail.com [web] Update build_web_compilers to 2.7.1 (flutter/engine#14305) 2019-12-11 liyuqian@google.com Cleanup the IO thread GrContext (flutter/engine#14265) 2019-12-10 50856934+nturgut@users.noreply.github.com Fix for tab not working (flutter/engine#14165) 2019-12-10 jason-simmons@users.noreply.github.com [SkParagraph] Convert the height override flag in text styles (flutter/engine#14283) 2019-12-10 bkonyi@google.com Roll src/third_party/dart 02a8b015ad..98c13ba18f (5 commits) (flutter/engine#14280) 2019-12-10 tvolkert@users.noreply.github.com Remove specificity on Android and iOS (flutter/engine#14282) 2019-12-10 jason-simmons@users.noreply.github.com Create separate objects for isolate state and isolate group state (flutter/engine#14268) 2019-12-10 bkonyi@google.com Roll src/third_party/dart 8b8894648f..02a8b015ad (26 commits) (flutter/engine#14278) 2019-12-10 skia-flutter-autoroll@skia.org Roll fuchsia/sdk/core/linux-amd64 from Zkpa_... to nqJnP... (flutter/engine#14274) 2019-12-10 skia-flutter-autoroll@skia.org Roll src/third_party/skia e56cc054dbae..ab26643258ad (3 commits) (flutter/engine#14273) 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 kaushikiska@google.com on the revert to ensure that a human is aware of the problem. 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/+/master/autoroll/README.md

* 571c999 Roll src/third_party/skia e56cc054dbae..ab26643258ad (3 commits) (flutter/engine#14273) * 140818a Roll fuchsia/sdk/core/linux-amd64 from Zkpa_... to nqJnP... (flutter/engine#14274) * ed6830e Roll src/third_party/dart 8b8894648f..02a8b015ad (26 commits) (flutter/engine#14278) * b7d4278 Create separate objects for isolate state and isolate group state (flutter/engine#14268) * 57afd86 Remove specificity on Android and iOS (flutter/engine#14282) * b7c947d Roll src/third_party/dart 02a8b015ad..98c13ba18f (5 commits) (flutter/engine#14280) * 76d264e [SkParagraph] Convert the height override flag in text styles (flutter/engine#14283) * deb8e57 Fix for tab not working (flutter/engine#14165) * 212fbba Cleanup the IO thread GrContext (flutter/engine#14265) * 3e55f64 [web] Update build_web_compilers to 2.7.1 (flutter/engine#14305) * b6d4fd1 Roll fuchsia/sdk/core/mac-amd64 from 9C6UA... to h4iiT... (flutter/engine#14314) * c0b1dc0 Roll src/third_party/skia 732c49739fa5..5afc7b2af854 (16 commits) (flutter/engine#14315) * 5b5206e Roll fuchsia/sdk/core/linux-amd64 from nqJnP... to UdfLO... (flutter/engine#14316) * 058b4bc Roll src/third_party/dart c74a8ec2c46e..bcd18e67dcae (9 commits) (flutter/engine#14317) * 6430ecf [fuchsia] Do not Execute paint tasks when there is no vsync (flutter/engine#14298) * 49d6552 Verify accounting for loop counts in Gif and WebP assets is consistent. (flutter/engine#14321) * 2ae0d42 Roll src/third_party/dart bcd18e67dcae..bd008dd1e406 (3 commits) (flutter/engine#14322) * ac95536 Roll src/third_party/skia 5afc7b2af854..75799967be60 (2 commits) (flutter/engine#14324) * 02fb9c1 Roll src/third_party/dart bd008dd1e406..d9fa37e85d5c (1 commits) (flutter/engine#14325) * db60ebc Roll fuchsia/sdk/core/mac-amd64 from h4iiT... to otkJA... (flutter/engine#14327)

Fixes flutter/flutter#19558 This is tested by the devicelab test fast_scroll_large_images__memory

liyuqian requested review from chinmaygarde and dnfield December 9, 2019 20:17

auto-assign bot requested a review from stuartmorgan-g December 9, 2019 20:17

googlebot added the cla: yes label Dec 9, 2019

liyuqian force-pushed the purge_io branch from fd63658 to 85ef656 Compare December 9, 2019 20:18

dnfield reviewed Dec 9, 2019

View reviewed changes

liyuqian added a commit to liyuqian/flutter that referenced this pull request Dec 9, 2019

Remove unused observer and raise the waiting time

9bff37d

The waiting time would give our cleanup more room to finish the job, and thus to make our result more accurate. See also flutter/engine#14265

liyuqian mentioned this pull request Dec 9, 2019

Remove unused observer and raise the waiting time flutter/flutter#46626

Merged

Cleanup the IO thread GrContext

bc694af

Fixes flutter/flutter#19558 This is tested by the devicelab test fast_scroll_large_images__memory

chinmaygarde reviewed Dec 9, 2019

View reviewed changes

Use a WeakPtr instead of a raw pointer

5927426

Otherwise, the IOManager may be destructed before the Drain is called and that could cause a crash.

liyuqian force-pushed the purge_io branch from 85ef656 to 5927426 Compare December 9, 2019 23:37

liyuqian added 2 commits December 9, 2019 15:39

Nits

c7e6e90

Fix the thread issue of the unit test

d536937

liyuqian requested a review from chinmaygarde December 10, 2019 00:51

liyuqian merged commit 212fbba into flutter:master Dec 11, 2019

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..3e55f64c4fd2 (10 commits) flutter/flutter#46739

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

cdeabd0

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..b6d4fd164c41 (11 commits) flutter/flutter#46745

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

266cb66

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..5b5206e56da6 (13 commits) flutter/flutter#46747

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

9c581f5

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

1361dd4

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..6430ecfd3902 (15 commits) flutter/flutter#46751

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

8ca0af8

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46754

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

8a02c4a

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46755

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

0fdea29

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

a56bc07

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46772

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

a998417

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46775

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

187f3f7

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46779

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

f694eca

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

018d4f1

This was referenced Dec 11, 2019

Roll engine 12bf95fd49b7..49d6552e76c0 (16 commits) flutter/flutter#46784

Closed

Roll engine 12bf95fd49b7..2ae0d4273b14 (17 commits) flutter/flutter#46788

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

080a68f

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..02fb9c1f9437 (19 commits) flutter/flutter#46791

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

853c253

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

ec55a86

This was referenced Dec 11, 2019

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46795

Closed

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46800

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

5b16ad9

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46804

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

42dc0ee

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

ca6fd6b

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46810

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 11, 2019

212fbba Cleanup the IO thread GrContext (flutter/engine#14265)

d02ed38

engine-flutter-autoroll mentioned this pull request Dec 11, 2019

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46814

Merged

iskakaushik mentioned this pull request Dec 12, 2019

Roll engine 12bf95fd49b7..db60ebc6325c (20 commits) flutter/flutter#46919

Closed

dnfield mentioned this pull request Dec 16, 2019

Memory Leak when animation is playing in iOS flutter/flutter#47108

Closed

liyuqian mentioned this pull request Feb 20, 2020

Flutter abort issue resulted from large memory use of pictures in low-memory iPhones. flutter/flutter#51046

Closed

filmil pushed a commit to filmil/engine that referenced this pull request Mar 13, 2020

Cleanup the IO thread GrContext (flutter#14265)

5fde9bf

Fixes flutter/flutter#19558 This is tested by the devicelab test fast_scroll_large_images__memory

dnfield mentioned this pull request Jun 16, 2020

GrContext::performDeferredCleanup needs to be called on the IO context. flutter/flutter#33087

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup the IO thread GrContext #14265

Cleanup the IO thread GrContext #14265

liyuqian commented Dec 9, 2019 •

edited

Loading

dnfield Dec 9, 2019

liyuqian Dec 9, 2019

dnfield Dec 9, 2019

chinmaygarde left a comment

chinmaygarde Dec 9, 2019

dnfield Dec 9, 2019

chinmaygarde Dec 9, 2019

liyuqian Dec 9, 2019

liyuqian commented Dec 10, 2019

Cleanup the IO thread GrContext #14265

Cleanup the IO thread GrContext #14265

Conversation

liyuqian commented Dec 9, 2019 • edited Loading

dnfield Dec 9, 2019

Choose a reason for hiding this comment

liyuqian Dec 9, 2019

Choose a reason for hiding this comment

dnfield Dec 9, 2019

Choose a reason for hiding this comment

chinmaygarde left a comment

Choose a reason for hiding this comment

chinmaygarde Dec 9, 2019

Choose a reason for hiding this comment

dnfield Dec 9, 2019

Choose a reason for hiding this comment

chinmaygarde Dec 9, 2019

Choose a reason for hiding this comment

liyuqian Dec 9, 2019

Choose a reason for hiding this comment

liyuqian commented Dec 10, 2019

liyuqian commented Dec 9, 2019 •

edited

Loading