Closed Bug 996929 Opened 6 years ago Closed 6 years ago

Windows XP non-PGO CanvasMark regression on inbound from bug 950372


(Core :: Graphics, defect)

Windows XP
Not set





(Reporter: jwatt, Assigned: mattwoodrow)




(2 files)

Regression: Mozilla-Inbound-Non-PGO - CanvasMark - WINNT 5.1 (ix) - 24% decrease
    Previous: avg 6980.042 stddev 28.433 of 12 runs up to revision e134b3aa718f
    New     : avg 5303.083 stddev 728.320 of 12 runs since revision a54345bfd338
    Change  : -1676.958 (24% / z=58.979)
    Graph   :
From IRC:

mattwoodrow	GetSourceSurfaceForSurface(aTarget, ..) was in SFE, now it’s GetSourceSurfaceForSurface(nullptr)
mattwoodrow	in RasterImage
mattwoodrow	and it’s on XP, so the canvas backend isn’t the same as the content backend
mattwoodrow	I think calling OptimizeSourceSurface in SurfaceFromElement would fix the regression
mattwoodrow	the problem is that we’re returning a SourceSurfaceCairo that wraps a gdi surface, and we put that into the canvas image cache
mattwoodrow	then every time we try draw with skia we call GetDataSurface() and that makes a copy
mattwoodrow	repeat for every draw of that image
mattwoodrow	OptimizeSourceSurface will do the same, but then we put that version into the cache
Attachment #8407213 - Flags: review?(jwatt)
Assignee: jwatt → matt.woodrow
Comment on attachment 8407213 [details] [diff] [review]
Optimize the source surface

Can you make this:

    // The surface we return is likely to be cached. We don't want to have to
    // convert to a surface that's compatible with aTarget each time it's used
    // (that would result in bad performance), so we convert once here upfront.
    RefPtr<SourceSurface> optSurface =
    if (optSurface) {
      result.mSourceSurface = optSurface;

At least add the comment.
Attachment #8407213 - Flags: review?(jwatt) → review+
Well, it would work that way, if OptimizeSourceSurface was actually implemented.
Whiteboard: [leave open]
This should actually fix the regression.
Attachment #8407301 - Flags: review?(bas)
Blocks: 997014
Comment on attachment 8407301 [details] [diff] [review]

Review of attachment 8407301 [details] [diff] [review]:

::: gfx/2d/DrawTargetD2D.cpp
@@ +1196,5 @@
> +  }
> +
> +  RefPtr<SourceSurfaceD2D> newSurf = new SourceSurfaceD2D();
> +
> +  if (!newSurf->InitFromData(map.mData, data->GetSize(), map.mStride, data->GetFormat(), mRT)) {

nit: I'd prefer:

bool success = newSurf->InitFromData(map.mData, data->GetSize(), map.mStride, data->GetFormat(), mRT);

if (!success) {
  return nullptr;

return newSurf;

::: gfx/2d/DrawTargetD2D1.cpp
@@ +936,5 @@
> +  }
> +
> +  RefPtr<ID2D1Bitmap1> bitmap;
> +  HRESULT hr = mDC->CreateBitmap(D2DIntSize(data->GetSize()), map.mData, map.mStride,
> +                                 D2D1::BitmapProperties1(D2D1_BITMAP_OPTIONS_NONE, D2DPixelFormat(data->GetFormat())),

nit: whitespace
Attachment #8407301 - Flags: review?(bas) → review+
Duplicate of this bug: 997369
Looks like this fixed the regression \o/
Whiteboard: [leave open]
very nice!
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in before you can comment on or make changes to this bug.