Closed Bug 585817 Opened 9 years ago Closed 9 years ago

Avoid surface allocations on GetThebesSurface

Categories

(Core :: Widget, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- beta5+

People

(Reporter: bas.schouten, Assigned: roc)

References

Details

Attachments

(5 files)

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.
blocking2.0: --- → ?
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.
This provides an API to a shared single-pixel surface that we can use to create gfxContexts for measuring stuff on the screen.
Assignee: nobody → roc
Attachment #464296 - Flags: review?(vladimir)
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...).
Attachment #464299 - Flags: superreview?(dbaron)
Attachment #464299 - Flags: review?(matspal)
The new gfxPlatform::ScreenReferenceSurface is basically the same as nsSVGUtils's ThebesComputationalSurface, so remove the latter and make all callers use gfxPlatform.
Attachment #464301 - Flags: review?
Attachment #464301 - Flags: review? → review?(jwatt)
On Mac, with these patches, I seem to be able to create new windows without hitting nsIWidget::GetThebesSurface at all.
blocking2.0: ? → beta4+
With these patches, is it possible to remove nsIWidget::GetThebesSurface entirely?
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 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.
Attachment #464301 - Flags: review?(jwatt) → review+
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?
This doesn't need to block beta4. However, it's ready to go if I get reviews.
blocking2.0: beta4+ → beta5+
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.
Attachment #464299 - Flags: review?(matspal) → review+
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 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.)
Attachment #464299 - Flags: superreview?(dbaron) → superreview+
Whiteboard: [needs landing]
(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.
Either this or bug 582057 failed a win7 reftest that didn't seem to show up on tryserver.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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 |
Huh ... tried a few more times and 7b3726c3a580 is bad too.  Maybe I was in the wrong objdir.
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).
Attachment #467649 - Flags: review?(roc)
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.
Attachment #467649 - Flags: review?(roc) → review+
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?
Attachment #467657 - Flags: review?(roc) → review+
Whiteboard: [needs landing]
You need to log in before you can comment on or make changes to this bug.