Closed Bug 989858 Opened 10 years ago Closed 10 years ago

Convert BasicLayers to Moz2D

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
firefox31 - ---

People

(Reporter: mattwoodrow, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: perf, regression, Whiteboard: [leave open][talos_regression])

Attachments

(6 files)

      No description provided.
Attachment #8399228 - Flags: review?(roc)
Attachment #8399229 - Flags: review?(roc)
Attachment #8399230 - Flags: review?(roc)
I don't know why this was added, partially implemented and never used. It didn't have a mask layer transform parameter, so it couldn't be correct anyway.

My patches replace this entirely, so it can go away.
Attachment #8399231 - Flags: review?(roc)
Comment on attachment 8399227 [details] [diff] [review]
Part 1: Add Moz2D helpers to BasicLayersImpl

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

::: gfx/layers/basic/BasicImplData.h
@@ +121,5 @@
>     */
>    virtual bool GetAsSurface(gfxASurface** aSurface,
>                              SurfaceDescriptor* aDescriptor)
>    { return false; }
> +  virtual bool GetAsSurface(RefPtr<gfx::SourceSurface>& aSurface) { return false; }

Why not just return a TemporaryRef<gfx::SourceSurface> directly, returning null on failure?

::: gfx/layers/basic/BasicLayersImpl.cpp
@@ +84,5 @@
> +
> +bool
> +AutoMoz2DMaskData::IsConstructed()
> +{
> +  return !!mSurface;

Put all these methods inline in the class.
Attachment #8399227 - Flags: review?(roc) → review+
Blocks: 990338
it appears that the canvasmark benchmark on linux (I suspect linux64, winxp, and win7 also) has regressed about 2.3% as a side effect of this patch set:
http://graphs.mozilla.org/graph.html#tests=[[297,131,33],[297,131,35],[297,131,37],[297,131,25]]&sel=1395770878537,1396375678537&displayrange=7&datatype=running

Here is more about canvasmark:
https://wiki.mozilla.org/Buildbot/Talos/Tests#CanvasMark

datazilla doesn't seem to have data for the last day, so i am not sure which subtest is the problem.
Blocks: 990085
here is a reference to some retriggers I did:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=Ubuntu%20HW%2012.04%20mozilla-inbound%20talos%20chromez&fromchange=b4edb59d3db5&tochange=ecda58bb90a7

you can see prior to this push we were in the 6500 range, and after this push in the 6350 range.
Blocks: 960524
Blocks: 982427
The following changesets are now in Firefox Nightly:

> 71db5845ea4d Bug 989858 - Part 1: Add Moz2D helpers to BasicLayersImpl. r=roc
> 25306d89bded Bug 989858 - Part 2: Convert BasicCanvasLayer. r=roc
> 433b95e605bf Bug 989858 - Part 3: Convert BasicImageLayer. r=roc
> 696938af99e5 Bug 989858 - Part 4: Convert BasicColorLayer. r=roc
> ad933a27ebb5 Bug 989858 - Part 5: Remove partially implemented moz2d paint function. r=roc
> f94c69dfca91 Bug 989858 - Part 6: Rename DeprecatedPaint and stop passing a gfxContext in. r=roc

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central
Depends on: 991067
as per comment 10
Keywords: perf, regression
Whiteboard: [leave open] → [leave open][talos_regression]
Depends on: 993784
No longer blocks: 982427
No longer blocks: 960524
I wanted to make sure we were looking into the talos regression, we will be uplifting this code in about 2 weeks.
Depends on: 996378
Depends on: 997336
this is on aurora- we still see the regression on linux32
Joel, are you still considering an uplift of this to 31 or the problem is still unfixed? thanks
Flags: needinfo?(jmaher)
(In reply to Sylvestre Ledru [:sylvestre] from comment #16)
> Joel, are you still considering an uplift of this to 31 or the problem is
> still unfixed? thanks

I haven't been following this, but if this 2.3% regression is the only thing standing between us and uplifting, then it shouldn't stop us from uplifting IMO.

Moz2D is new and important enough and some regressions are expected, especially when it brings other improvements elsewhere which are not mentioned here because we only track regressions and not improvements.

On top of this, we're not blocking on perf regressions unless they're really bad (and this isn't), and the threshold over which we file regression bugs was decided to be too low, and therefore starting next week we'll only be filing regression bugs for regressions over ~4% rather than over 2%.

To summarize, this regression shouldn't stop us from doing anything we think we should.
this regressions still exists.  It is disappointing when we spend resources for every changeset that detect a regression and there is no real comment about why we should accept the regression or what work is being done to accept it.  Now that this is going on 10 weeks and a couple uplifts, we really are starting to run out of time.

On Aurora and Inbound the regression exists and for the most part no other canvasmark regressions have been seen.  This is a <3% regression (on multiple platforms), so there isn't much we can do.
Flags: needinfo?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #18)
> ... It is disappointing when we spend resources
> for every changeset that detect a regression and there is no real comment
> about why we should accept the regression or what work is being done to
> accept it.

IMO it shouldn't be disappointing. The effort to reliably detect regressions is a useful one also without devs justifying or commenting in general on every 2% (or 20%) regression.

It allows us to track values over time, it gives feedback to devs and managers on where priorities might need to be, and, in case management or a group or a dev feels that it should be addressed, provide feedback while making changes to the code.

I'm not saying that commenting on a regression completely lacks value, but when the regression is small, the ground shifts much all over (as with OMTC or Moz2D, etc), taking the time and focus to be able to provide a meaningful explanation or justification for a 2% regression will just cost more than it gives back.

Besides, I believe that if anyone thought it's worth a comment or that they have anything useful to say here, you'd have seen a comment by now, as we've seen already on many other cases.
(In reply to Avi Halachmi (:avih) from comment #17)
> ... we're not blocking on perf regressions unless they're really bad.

That's actually a lie. We don't block on perf regressions, period.

We can help in understanding the numbers better, or help in evaluating the importance of a specific regression, but the decision if it's a blocker or not is affected by so many factors (product, marketing, deadlines, etc, including performance/regressions in other areas of the product or tradeoff) that unless it's a disastrous one to a degree that it will block itself by whomever owns the code, we can't claim that some X% regression is worth blocking just because it exists or is above some Y threshold.
This bug will probably requires a refactoring which would not be accepted in 31. Untracking it.
this is almost 3 months old, a lot has changed, can we close this bug?  is there work we should be tracking?
Flags: needinfo?(matt.woodrow)
yes \o/
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: needinfo?(matt.woodrow)
Resolution: --- → FIXED
Part 6 replaced a call to gfxContext::UserToDevicePixelSnapped() with a call to the new Moz2D helper of the same name. These aren't equivalent though since the Moz2D version doesn't (isn't able to) check for the presence of the FLAG_DISABLE_SNAPPING flag. Should we just stop using FLAG_DISABLE_SNAPPING (it's only used for printing, and it's comments suggest that's only to skip seam avoidance work that isn't necessary for printing), or do we need to replace the gfxContext FLAG_DISABLE_SNAPPING flag with a UserData entry on the DrawTarget (more expensive to look-up than a flag with good cache locality). If you don't know the answer, Matt, do you know who would?
Flags: needinfo?(matt.woodrow)
(In reply to Jonathan Watt [:jwatt] (away until Tuesday 14th) from comment #25)
> Part 6 replaced a call to gfxContext::UserToDevicePixelSnapped() with a call
> to the new Moz2D helper of the same name. These aren't equivalent though
> since the Moz2D version doesn't (isn't able to) check for the presence of
> the FLAG_DISABLE_SNAPPING flag. Should we just stop using
> FLAG_DISABLE_SNAPPING (it's only used for printing, and it's comments
> suggest that's only to skip seam avoidance work that isn't necessary for
> printing), or do we need to replace the gfxContext FLAG_DISABLE_SNAPPING
> flag with a UserData entry on the DrawTarget (more expensive to look-up than
> a flag with good cache locality). If you don't know the answer, Matt, do you
> know who would?

I think we probably still do need it, though the lack of complaints would suggest otherwise. I think roc would be the best person to answer this though.

We don't check the flags *that* often, making it user data should be ok. Otherwise we can just add a flag to DrawTarget (SetSubpixelAAEnabled is basically that).
Flags: needinfo?(matt.woodrow) → needinfo?(roc)
(In reply to Matt Woodrow (:mattwoodrow) from comment #26)
> I think we probably still do need it, though the lack of complaints would
> suggest otherwise. I think roc would be the best person to answer this
> though.
> 
> We don't check the flags *that* often, making it user data should be ok.
> Otherwise we can just add a flag to DrawTarget (SetSubpixelAAEnabled is
> basically that).

I think we should keep on not snapping for printing. It artificially reduces the accuracy of the printed output.
Flags: needinfo?(roc)
I've put patches up in bug 1082530.
You need to log in before you can comment on or make changes to this bug.