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

Conversation

freiling
Copy link
Contributor

Description

This is a reland of #22984 because I accidentally submitted without code review and reverted.

This PR contains fixes for a number of issues that came up around shader warmup

  • Correctly handling the Embedder API ifdefs
  • Decouple Shader warmup from embedder API so they can be enabled independently
  • Reduce in flight memory usage to avoid OOM'ing the device
  • Fix a resource lifetime issue uncovered by the fix for the OOM

Tests

These issues are not within the scope of unit tests and need to be covered by an E2E integration test that runs on fuchsia infrastructure for the following reasons:

  • Embedder API not present in unit test harness
  • Memory pressure during testing is lower due to using beefier hardware and not running as much stuff
  • resource lifetime issue is covered by existing unittests now that the OOM fix is in

This fixes an issue with the shader warmup where gpu resources could end
up deleted before the gpu work that needed them was complete, leading to
GPU page faults. This was because although the sk_sp<SkSurface> will normally
keep resources alive throughout its lifetime, the SurfaceProducerSurface will
call VkDestroyMemory on the memory backing the SkSurface when it is deleted,
even if the SkSurface wrapping that VkMemory is still alive.

This change also deletes some related but unused code from
CompositorContext that I noticed while refactoring.
@freiling freiling requested a review from arbreng December 10, 2020 23:38
@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.

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

Copy link
Contributor

@arbreng arbreng left a comment

Choose a reason for hiding this comment

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

+2 w/ the suggestions given

@arbreng arbreng added the waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land. label Dec 12, 2020
@fluttergithubbot fluttergithubbot merged commit bbed7c6 into flutter:master Dec 14, 2020
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 15, 2020
gspencergoog pushed a commit to gspencergoog/engine that referenced this pull request Jan 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes waiting for tree to go green This PR is approved and tested, but waiting for the tree to be green to land.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants