Closed Bug 1087530 Opened 5 years ago Closed 5 years ago

Retain container's intermediate surfaces for up to one frame

Categories

(Core :: Graphics, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: BenWa, Assigned: BenWa)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: perf)

Attachments

(2 files, 7 obsolete files)

+++ This bug was initially created as a clone of Bug #1037220 +++

Right now hitting group opacity is a large performance cliff. This bug is about avoiding alloc/free of a render target for each draw where a intermediate container layer is required.
Attached patch WIP (missing d3d implementation) (obsolete) — Splinter Review
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch WIP (missing d3d implementation) (obsolete) — Splinter Review
Attachment #8509718 - Attachment is obsolete: true
Profiling results on mac:
We spend less time flushing under glDeleteTexture.

I did more in depth profiling b2g:
Default: http://people.mozilla.org/~bgirard/cleopatra/#report=f4a7e7d25c39d7e7a3890737d818473c77d73cf5&select=3632,4320
With WIP: http://people.mozilla.org/~bgirard/cleopatra/#report=9059fc993257ce438b90614b3cc609c95d674ff5&select=93603,94456

Differences:
Total GPU time: 9ms -> 7.5ms (GPU profiling already coming in handy)
CPU Frame times: Bimodal frame times, half at 20ms and half near 28ms -> Most frames at 16.6ms, two frames at ~28ms

Risk:
Reusing a surface that's still in use from the previous frame might cause a hazard that some GPU drivers might fallback to flushing. We need to keep an eye out to make sure this doesn't regress on any platforms.
Attached patch WIP (obsolete) — Splinter Review
Needs testing on windows
Attachment #8509721 - Attachment is obsolete: true
Attached patch patch v1 (obsolete) — Splinter Review
Tested d3d9/d3d11/ogl(mac,b2g)/basic(windows)
Attachment #8509780 - Attachment is obsolete: true
Attachment #8509834 - Flags: review?(jmuizelaar)
OS: Linux → All
Hardware: x86_64 → All
Comment on attachment 8509834 [details] [diff] [review]
patch v1

Review of attachment 8509834 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/composite/TextureHost.h
@@ +671,5 @@
>  
> +  /**
> +   * Perform a clear when recycling a non opaque surface.
> +   * This must be call before binding.
> +   */

I don't like the semantics of this that much, but don't have suggestions for improvement.

@@ +680,3 @@
>  
>  private:
> +  gfx::IntRect mRect;

It seems like this will store the size twice. Let's not.
Attachment #8509834 - Flags: review?(jmuizelaar) → review-
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #8509834 - Attachment is obsolete: true
Attachment #8510390 - Flags: review?(jmuizelaar)
This is a profile of a testcase that uses group opacity. Jeff wanted a profile showing deleting a texture being costly:

http://people.mozilla.org/~bgirard/cleopatra/#report=c4a756c160c0c35286b2b6db472df4337ceb6eda&search=delete
Attached patch patch v3 (obsolete) — Splinter Review
windows fixed
Attachment #8510390 - Attachment is obsolete: true
Attachment #8510390 - Flags: review?(jmuizelaar)
Blocks: 945082
Depends on: 1092360
with this patch and bug 1092360 my testcase goes from ~40ms to ~23ms frame time on the flame:
http://people.mozilla.org/~bgirard/many_framebuffer.html
Attached patch patch v4Splinter Review
Attachment #8510433 - Attachment is obsolete: true
Attachment #8511215 - Attachment is obsolete: true
Attachment #8516291 - Flags: review?(jmuizelaar)
Comment on attachment 8516291 [details] [diff] [review]
patch v4

Review of attachment 8516291 [details] [diff] [review]:
-----------------------------------------------------------------

Sure.
Attachment #8516291 - Flags: review?(jmuizelaar) → review+
Blocks: 1098495
Improvement: Mozilla-Inbound - Session Restore no Auto Restore Test - MacOSX 10.6 (rev4) - 2.25% decrease
https://hg.mozilla.org/mozilla-central/rev/1608ca65d899
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Depends on: 1101650
Attachment #8525491 - Flags: review?(jmuizelaar)
Attachment #8525491 - Flags: review?(jmuizelaar) → review+
You need to log in before you can comment on or make changes to this bug.