1186546 - DrawTargetCG::FillRect allocates an unnecessary surface when tiling device-pixel aligned surfaces

Status: RESOLVED WONTFIX

(Core :: Graphics, defect)

Description

[Markus Stange [:mstange]]

Attachment: patch

This shows up as build_tile in profiles; it's called by CG when drawing repeated surfaces. However, when we're device-pixel aligned, it's not necessary to allocate an intermediate surface, so CG is just doing unnecessary work in that case.

We can work around this by calling DrawSurface in a loop on our side. This won't give us repeat sampling at the tile edges, but CG doesn't give us that anyway.

Comment 1

[Markus Stange [:mstange]]

I forgot to push a clip in this patch. It needs a PushClipRect(deviceRect) after the first SetTransform call and a PopClip() before the second SetTransform call.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Just did some profiling and measurements, and this patch doesn't help much :(. Some timings while scrolling bug 1172841.

Without patch:

// Refreshing the page
Draw Scaled Image time: 66.852088
Draw Scaled Image time: 69.202409
Draw Scaled Image time: 67.071342
Draw Scaled Image time: 67.323160

Scrolling to bottom: // Very noisy due to human hand scrolling
690.590693
632.590209
681.383573

Comment 3

[Mason Chang [Inactive] [:mchang]]

With patch
// Refresh
Draw Scaled Image time: 79.320642
Draw Scaled Image time: 68.486225
Draw Scaled Image time: 72.478783
Draw Scaled Image time: 67.550486

// Scrolling
701.223834
660.411152
819.329177

Comment 4

[Mason Chang [Inactive] [:mchang]]

With CGContextDrawTiledImage from bug 1154311.

// Refresh 
Draw Scaled Image time: 69.213240
Draw Scaled Image time: 70.717759
Draw Scaled Image time: 65.031977
Draw Scaled Image time: 65.165359

// Scrolling
615.527478
654.345563
664.044687

Comment 5

[Mason Chang [Inactive] [:mchang]]

I think the problem is that in profiles, build_tile doesn't show up according to instruments. This is what I'm getting from instruments with an inverted call tree:

Inverted call tree
Running Time	Self (ms)		Symbol Name
360.0ms   22.2%	360.0	 	 argb32_mark
360.0ms   22.2%	0.0	 	  RIPLayerBltShape
360.0ms   22.2%	0.0	 	   ripc_Render
360.0ms   22.2%	0.0	 	    ripc_DrawRects
360.0ms   22.2%	0.0	 	     CGContextFillRects
360.0ms   22.2%	0.0	 	      mozilla::gfx::DrawTargetCG::FillRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&)
360.0ms   22.2%	0.0	 	       mozilla::gfx::DrawTargetTiled::FillRect(mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::Pattern const&, mozilla::gfx::DrawOptions const&)
360.0ms   22.2%	0.0	 	        DrawScaledImage(gfxDrawable*, gfxContext*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits>, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits>, GraphicsFilter const&, mozilla::gfx::SurfaceFormat, double)

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Fixed patch

From comment 1.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

Can you get a profile with the patch?

Comment 8

[Mason Chang [Inactive] [:mchang]]

http://people.mozilla.org/~bgirard/cleopatra/#report=5e472a081cc94e1b9600ba62053d5a0180c826f0

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(In reply to Mason Chang [:mchang] from comment #8)
> http://people.mozilla.org/~bgirard/cleopatra/
> #report=5e472a081cc94e1b9600ba62053d5a0180c826f0

As usual, your CoreGraphics symbols are garbage. Can you do what you did with instruments?

Comment 10

[Jeff Muizelaar [:jrmuizel]]

Or maybe it's good enough. It doesn't look like mstange's fast path is being hit. Are the patches you're testing up some place?

Comment 11

[Mason Chang [Inactive] [:mchang]]

Ahh, yeah mstange's fast path wasn't being hit because of:

if (aPattern.mExtendMode != ExtendMode::REPEAT ||
    !aPattern.mSamplingRect.IsEmpty() || // We're not using a sampling rect here
    aPattern.mMatrix.IsSingular() ||
    !mTransform.IsRectilinear()) {                                                                
  return false;                                                                          
}

But the numbers didn't move much.

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: WIP patch w/ this and scale temp surface

Ahh should say, because we are using a sampling rect here, that's just the size of the whole scaled image surface. But even using the fast path, the numbers didn't move much.

Comment 13

[Markus Stange [:mstange]]

Attachment: patch

This one might be better.

Comment 14

[Markus Stange [:mstange]]

We're not using DrawTargetCG any more so this optimization is not worth putting effort into.