search

flutter / flutter

Flutter makes it easy and fast to build beautiful apps for mobile and beyond
https://flutter.dev
BSD 3-Clause "New" or "Revised" License
165.17k stars 27.25k forks source link

Share the GrDirectContext across engine instances on Android #90398

Closed gaaclarke closed 3 years ago

gaaclarke commented 3 years ago

This feature previously was introduced in https://github.com/flutter/engine/pull/23798, but was reverted in https://github.com/flutter/engine/pull/28694. This revert can lead to a 5x to 50x increase of memory usage of secondary engines from a FlutterEngineGroup on Android. The revert happened because the GrDirectContext was unsafely being cleaned up on the platform thread. That threading issue needs to be addressed and tests are needed to bolster the approach.

See also: 1) https://github.com/flutter/flutter/issues/87937

chinmaygarde commented 3 years ago

"5x to 50x increase of memory usage of secondary engines from a FlutterEngineGroup on Android". Can you cross-reference the benchmark you are referring to so the sheriff can monitor?

gaaclarke commented 3 years ago

@chinmaygarde I added a source for those numbers in a link. I can't remember if we created automated performance tests for memory usage for FlutterEngineGroup. I think so.

chinmaygarde commented 3 years ago

The action items from our discussion the other day were:

  1. [Jason] Disable the buggy feature till we can figure out how to safely re-enable it.
  2. [Aaron] Try to recreate the failure in the shell unit-tests harness.
  3. [Aaron] Try to figure out if we are running into the same issue on iOS 9 and disable the feature on iOS in the meantime as well.
  4. [Chinmay] Figure out how SwiftShader can be configured to make GL calls without a context throw fatal errors.
  5. [All] Based on the test and its implications for all platforms, reconvene and decide how to safely re-enable the feature everywhere.

So we are using this as meta-bug for all tasks right?

gaaclarke commented 3 years ago

That sounds good to me. I updated the link to the source of 5x-50x (it was broken). Also here is a link to the benchmarks: https://github.com/flutter/flutter/tree/master/dev/benchmarks/multiple_flutters/android/app/src/main/java/dev/flutter/multipleflutters

gaaclarke commented 3 years ago

Here's the skia perf that seems related to that test. It doesn't seem to display anything: https://flutter-flutter-perf.skia.org/e/?begin=1631314922&end=1632168783&queries=test%3DLinux_android_flutter_engine_group_performance%26test&requestType=0 Considering our manual memory audits, I suspect it's not a good test. Probably the increase in size of the secondary engine is minuscule compared to the total memory usage.

gaaclarke commented 3 years ago

I've recreated the leak in the test harness introduced in https://github.com/flutter/engine/pull/28784. There was some confusion about the original issue where it reported a crash and a leak. I clarified with @jason-simmons that we didn't see the crash. The revert was predicated on the opengl leak.

gaaclarke commented 3 years ago

Here is the umbrella issue to get the tests implemented and running on ci: https://github.com/flutter/flutter/issues/90903

github-actions[bot] commented 2 years ago

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.