585817 - Avoid surface allocations on GetThebesSurface

Status: RESOLVED FIXED

(Core :: Widget, defect)

Description

[Bas Schouten (:bas.schouten)]

Right now we call GetThebesSurface during rendering. This, especially when using D2D, causes complex allocations of resources. D2D actually caches these and this is bad. In any case we should probably find a way of changing this when we have retained layer managers which don't render to a window surface.

Comment 1

[Bas Schouten (:bas.schouten)]

My former comment was not completely accurate, when these are called without a BasicLayerManager these surfaces actually aren't cached, which overhead-wise is even worse.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: add gfxPlatform::ScreenReferenceSurface

This provides an API to a shared single-pixel surface that we can use to create gfxContexts for measuring stuff on the screen.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 2: Change nsIPresShell::CreateRenderingContext to GetReferenceRenderingContext

Almost all the callers of nsIPresShell::CreateRenderingContext (all except inFlasher and nsSimplePageSequence) just want a context that they can used to measure text and similar stuff, they don't want to draw. So this patch renames the API, reimplements it to use gfxPlatform::ScreenReferenceSurface for any screen device, and changes the callers to use the new API. inFlasher and nsSimplePageSequence just create the reference context using essentially an inlined and specialized version of the old nsIPresShell::CreateRenderingContext (for nsSimplePageSequence this boils down to just one line...).

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 3: remove nsSVGUtils::GetThebesComputationalSurface in favour of gfxPlatform::ScreenReferenceSurface

The new gfxPlatform::ScreenReferenceSurface is basically the same as nsSVGUtils's ThebesComputationalSurface, so remove the latter and make all callers use gfxPlatform.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

On Mac, with these patches, I seem to be able to create new windows without hitting nsIWidget::GetThebesSurface at all.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

With these patches, is it possible to remove nsIWidget::GetThebesSurface entirely?

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Not quite, there's still dribs and drabs like inFlasher.cpp and maybe other places that want to just draw into a widget. But we should definitely be aiming for that.

Comment 8

[Jonathan Watt [:jwatt]]

Comment on attachment 464301 [details] [diff] [review]
Part 3: remove nsSVGUtils::GetThebesComputationalSurface in favour of gfxPlatform::ScreenReferenceSurface

I don't really understand the choice of words "Screen" and "Reference" in ScreenReferenceSurface (it sounds like a reference to the screen rather than a measuring surface), but I guess you're just keeping the name consistent with an existing member.

Comment 9

[Shawn Wilsher :sdwilsh]

Is this needed to turn on d2d (it's not set to blocking the bug to do that, so it is unclear)?  If not, why does this need to block beta4 as opposed to final or betaN?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This doesn't need to block beta4. However, it's ready to go if I get reviews.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 464299 [details] [diff] [review]
Part 2: Change nsIPresShell::CreateRenderingContext to GetReferenceRenderingContext

> In layout/base/nsIPresShell.h
-  virtual NS_HIDDEN_(nsresult) CreateRenderingContext(nsIFrame *aFrame,
-                                                      nsIRenderingContext** aContext) = 0;
+  virtual already_AddRefed<nsIRenderingContext> GetReferenceRenderingContext() = 0;
 
Did you drop NS_HIDDEN_ intentionally?

> In layout/base/nsPresShell.cpp
+    devCtx->CreateRenderingContextInstance(*getter_AddRefs(rc));
+    rc->Init(devCtx, gfxPlatform::GetPlatform()->ScreenReferenceSurface());

Check NS_SUCCEEDED(return value) before using 'rc'.

> In layout/generic/nsSimplePageSequence.cpp
+      dc->CreateRenderingContext(*getter_AddRefs(renderingContext));

Add NS_ENSURE_TRUE(renderingContext, NS_ERROR_OUT_OF_MEMORY)

> In layout/svg/base/src/nsSVGForeignObjectFrame.cpp
-  if (!renderingContext)
-    return;

Please keep the null check.
nsHTMLReflowState::nsHTMLReflowState has a PRECONDITION for that.

> In layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

We should null check 'rc' for all these calls.

Comment 12

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Rob: just fyi, this was clean on tryserver below the refactoring patches in bug 582057, in cf8300790325, a36b9fcc41ff, 6d630e8e9743, and 01e851d8285f (kept getting bad slaves :|).

Comment 13

[David Baron :dbaron:]

Comment on attachment 464299 [details] [diff] [review]
Part 2: Change nsIPresShell::CreateRenderingContext to GetReferenceRenderingContext

sr=dbaron, but we should probably decide whether we should be null-checking the result of this function.  (We do in some places, don't in others, and you changed it in the SVG case.)

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #11)
> > In layout/base/nsIPresShell.h
> -  virtual NS_HIDDEN_(nsresult) CreateRenderingContext(nsIFrame *aFrame,
> -                                                      nsIRenderingContext**
> aContext) = 0;
> +  virtual already_AddRefed<nsIRenderingContext> GetReferenceRenderingContext()
> = 0;
> 
> Did you drop NS_HIDDEN_ intentionally?

Yes, I believe it's a noop with libxul.

> > In layout/base/nsPresShell.cpp
> +    devCtx->CreateRenderingContextInstance(*getter_AddRefs(rc));
> +    rc->Init(devCtx, gfxPlatform::GetPlatform()->ScreenReferenceSurface());
> 
> Check NS_SUCCEEDED(return value) before using 'rc'.

I added "if (rc)" instead.

> > In layout/generic/nsSimplePageSequence.cpp
> +      dc->CreateRenderingContext(*getter_AddRefs(renderingContext));
> 
> Add NS_ENSURE_TRUE(renderingContext, NS_ERROR_OUT_OF_MEMORY)

Done

> > In layout/svg/base/src/nsSVGForeignObjectFrame.cpp
> -  if (!renderingContext)
> -    return;
> 
> Please keep the null check.

Done

> > In layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp
> 
> We should null check 'rc' for all these calls.

Done.

Comment 15

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/b3e968d831ec
http://hg.mozilla.org/mozilla-central/rev/7b3726c3a580
http://hg.mozilla.org/mozilla-central/rev/cebb111fbfc4

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Either this or bug 582057 failed a win7 reftest that didn't seem to show up on tryserver.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Bisection says that http://hg.mozilla.org/mozilla-central/rev/7b3726c3a580 (part 2) is OK and http://hg.mozilla.org/mozilla-central/rev/cebb111fbfc4 (part 3) triggers
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/mozilla-central_win7_test-reftest/build/reftest/tests/layout/reftests/bugs/373381-1.html |
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/mozilla-central_win7_test-reftest/build/reftest/tests/layout/reftests/forms/checkbox-radio-stretched.html |

Comment 18

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Huh ... tried a few more times and 7b3726c3a580 is bad too.  Maybe I was in the wrong objdir.

Comment 19

[Bas Schouten (:bas.schouten)]

Attachment: Create and expose a scratch DC from gfxWindowsPlatform

This exposes a ScratchDC that can be used to select fonts in and such whenever API's require such a surface (for example the theme APIs).

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 467649 [details] [diff] [review]
Create and expose a scratch DC from gfxWindowsPlatform

Let's call it GetScreenDC since that's exactly what it is.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: Use gfxWindowsPlatform::GetScreenDC to compute the minimum widget size

Comment 22

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 467657 [details] [diff] [review]
Use gfxWindowsPlatform::GetScreenDC to compute the minimum widget size

Fine but can we rename GetScratchDC to GetScreenDC in both patches pleeeeease?

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

http://hg.mozilla.org/mozilla-central/rev/3608833562ed
http://hg.mozilla.org/mozilla-central/rev/65cd9d78d0a8
http://hg.mozilla.org/mozilla-central/rev/6e78abd54201
http://hg.mozilla.org/mozilla-central/rev/80cea1587c03
http://hg.mozilla.org/mozilla-central/rev/64fa66e599c8