Closed Bug 973976 Opened 6 years ago Closed 6 years ago

tcanvasmark regression detected on osx and windows platforms starting feb 12, 2014

Categories

(Testing :: Talos, defect)

All
macOS
defect
Not set

Tracking

(firefox29 unaffected, firefox30 affected)

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- unaffected
firefox30 --- affected

People

(Reporter: jmaher, Assigned: torarvid)

References

Details

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

Attachments

(2 files, 3 obsolete files)

with this revision landing:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&startdate=2014-02-12&enddate=2014-02-14&rev=4e9b713f435a

We have an almost 10% regression on canvasmark for osx 10.6 and 10.8!  Check the graphs here:
http://graphs.mozilla.org/graph.html#tests=[[297,63,24],[297,63,21]]&sel=1392139823821,1392744623821&displayrange=7&datatype=running

there are 4 bugs that landed with that commit, bug 963023, bug 960382, bug 956646, and bug 948785.

I like to think it is bug 948785 due to the nature of the bug.
Yeah, definitely looks like bug 948765.
Depends on: 948765
:nical, can you take a look at this?
Flags: needinfo?(nical.bugzilla)
(In reply to Joel Maher (:jmaher) from comment #2)
> :nical, can you take a look at this?

Yes. I hope that bug 973892 will fix this. If not I'll have to see if how soon I can dedicate time to this, otherwise I'll find someone to investigate, or backout bug 948765.
Assignee: nobody → nical.bugzilla
Flags: needinfo?(nical.bugzilla)
I believe Bug 956646 should have nothing to do with this regression. That bug adds a method to nsIFile, but hasn't been used yet :)
My investigation so far have found some small means for improvement; A call to DrawTarget::Flush() can be removed in CopyableCanvasLayer::UpdateTarget, and the creation of a DrawTarget in CanvasClient2D can be changed so it is not created on every frame (but rather whenever the mBuffer is set).

However, after doing these changes locally, it did not seem to yield much improvement (if any at all) on the tcanvasmark :-/

What I *have* seen though, is that when we go through the old thebes path, we eventually call FillWithMask, while in the Moz2D path, we call DrawTarget::FillRect instead.

When trying a simple canvas page:
http://jonobr1.github.io/two.js/examples/particle-sandbox.html?type=canvas&shapes=circle&operations=translation&count=1

... I've benchmarked the call to FillWithMask at around 0.8-1 ms, and DrawTargetCG::FillRect at around 4 ms.

So - at this point at least, it seems like DrawTargetCG::FillRect needs to accept some blame (and me for calling it ;))

Any CoreGraphics experts around to help with this?
thanks for looking into this!
This operator was already initialized to OP_SOURCE, so no need to do it
again.
Attachment #8379732 - Flags: review?(nical.bugzilla)
Every time ::Update was called, we instantiated an instance of DrawTarget
from the mBuffer. This patch creates a member variable mDrawTarget so that
it will only be created when mBuffer changes.
Attachment #8379733 - Flags: review?(nical.bugzilla)
This patch *may* be an idea to speed up Canvas rendering on the Mac
platform.

Previously, this code would often create a separate CGLayer and draw into
that. At the end of FillRect, the CGLayer would be stamped onto the main
CGContext... This patch removes the creation of this layer, and preliminary
tests (running talos tcanvasmark on my laptop) seem to increase performance
by about 3.5 % for me (rMBP with OS X 10.9.1).

But will some other stuff break or become slower because of this? Need
input from others here ;)
Attachment #8379735 - Flags: review?(jmuizelaar)
(Yeah, the getTimeMillis() method in the last patch is just my little debug-benchmark-thingy and I'll remove it if we want to merge something like this change..)
Comment on attachment 8379733 [details] [diff] [review]
Reduce instantiations of DrawTarget in CanvasClient2D

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

TextureClient is not guaranteed to return the same DrawTarget for each frame (MemoryTextureClient used a lot on mac will, but GrallocTextureClientOGL won't, for instance). This patch may work on mac on will probably break b2g because it retains a DrawTarget pointing to invalid (unpapped) memory. If there is DrawTarget creation/destruction work to be saved by caching the DrawTarget, it is already done (or should be done) by the TextureClient itself, and users of TextureClient should not keep references to the DrawTarget outside of the interval between Lock and Unlock.
Attachment #8379733 - Flags: review?(nical.bugzilla) → review-
Comment on attachment 8379732 [details] [diff] [review]
Remove redundant work in CopyableCanvasLayer

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

I am not a good reviewer for this, passing on to Jeff.
Attachment #8379732 - Flags: review?(nical.bugzilla) → review?(jmuizelaar)
I am not going to be available for this bug in the next few weeks, so I am passing this to Tor. If we don't manage to get this fixed we'll have to prepare a backout of bug 948765 at some point.
Assignee: nical.bugzilla → torarvid
Comment on attachment 8379735 [details] [diff] [review]
[RFC] Simplify DrawTargetCG::FillRect

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

I don't think this will work. The UnboundnessFixer is need for proper behaviour for certain operators. Does this pass the same number of canvas tests on http://philip.html5.org/tests/canvas/suite/tests/index.2d.composite.html?

What operator is being used by tcanvasmark?
Attachment #8379735 - Flags: review?(jmuizelaar) → review-
Attachment #8379732 - Flags: review?(jmuizelaar) → review+
This operator was already initialized to OP_SOURCE, so no need to do it
again.
Attachment #8379732 - Attachment is obsolete: true
Attachment #8382904 - Flags: review?(jmuizelaar)
The performance on the Mac platform degraded after porting the code to
Moz2D in Bug 948765. This patch chooses the old thebes path instead of the
Moz2D path, so that performance is unaffected. This way we can easily
revert this patch at a later time when the perf issue has been fixed.
Attachment #8379733 - Attachment is obsolete: true
Attachment #8379735 - Attachment is obsolete: true
Attachment #8382905 - Flags: review?(jmuizelaar)
I need help from someone else to figure out how to do this faster in Moz2D on Mac with CoreGraphics.

If nobody wants to do this soon'ish, the last patch I uploaded is a way to make the code take the old Thebes path instead of the Moz2D path (so that we don't have to revert everything from 948765). That should restore the performance to what it was before Bug 948765.
Assignee: torarvid → nobody
Attachment #8382905 - Flags: review?(jmuizelaar) → review+
Attachment #8382904 - Flags: review?(jmuizelaar) → review+
I filed bug 983548 for a pdf.js regression that points to the same pushlog as this bug does. My bug might be a duplicate of this bug, but has different STR and a much bigger performance regression (6x vs 1.1x).
Blocks: 948765
No longer depends on: 948765
I have verified that the performance issue on bug 983548 is very real. I have also verified that the patches in this bug fixes the issue.

Even though I think this is a generic performance issue in DrawTargetCG (and long-term fixes are needed there), it seems that these patches are an OK solution in the imminent future.

So could someone with access tag this with checkin-needed (or assign me to this bug again, so I can do it myself)?
Assignee: nobody → torarvid
thanks for working on this, I am glad to see a solution to this regression!
https://hg.mozilla.org/mozilla-central/rev/b1e0e1576f38
https://hg.mozilla.org/mozilla-central/rev/c737dc096ba0
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.