1335149 - Slow drawing on this canvas example

Status: RESOLVED FIXED

(Core :: Graphics, defect, P3)

Description

[Andrew Overholt [:overholt]]

Attachment: index.html

Firefox Nightly on Windows 10 (Surface Book) takes a lot more time than Chrome (something like 55 ms vs 6 ms) to render the canvas in the test case I'll attach.

Comment 1

[Andrew Overholt [:overholt]]

(Note that the performance is a bit difference when the grid() function is in a separate js file)

Comment 2

[Andrew Overholt [:overholt]]

Attachment: unnecessaryflush.PNG

Jeff Muizelaar said we're doing a flush when we shouldn't be (see profile screenshot attached).

Profile: https://clptr.io/2kk4W4s

Comment 3

[Andrew Overholt [:overholt]]

Edgard now has a Bugzilla account so I should point out that the excellent reduced test case came from him; thanks in public, Edgard :)

(Since this is related to Google Sheets performance, I'm setting platform-rel:+)

Comment 4

[Jeff Muizelaar [:jrmuizel]]

Does this build help?
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-606f383fb5cd72971519f6bfd1e31733580767cf/try-win32/

Comment 5

[Andrew Overholt [:overholt]]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Does this build help?
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 606f383fb5cd72971519f6bfd1e31733580767cf/try-win32/

Yes! With it I get 25-30 ms render times vs > 70 ms in today's nightly build (this is all Windows 10 on a Surface Book).
I still get < 10 ms in Chrome and ~45-50 ms in Edge.

Comment 6

[Jeff Muizelaar [:jrmuizel]]

Attachment: Remove flushes that I don't think we need

Bas, does this seem sane to you?

Comment 7

[Jeff Muizelaar [:jrmuizel]]

(In reply to Andrew Overholt [:overholt] from comment #5)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> > Does this build help?
> > https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> > 606f383fb5cd72971519f6bfd1e31733580767cf/try-win32/
> 
> Yes! With it I get 25-30 ms render times vs > 70 ms in today's nightly build
> (this is all Windows 10 on a Surface Book).
> I still get < 10 ms in Chrome and ~45-50 ms in Edge.

Chrome, afaik, defers canvas rendering so the Chrome times don't actually include the rendering time.

This means to do a fair comparison, the benchmark should actually be painting in a loop or at least trying to maintain 60fps and seeing how close it can get.

Comment 8

[Andrew Overholt [:overholt]]

Edgard, see comment 7 above from Jeff. What do you think?

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Jeff Muizelaar [:jrmuizel] from comment #6)
> Created attachment 8832496 [details] [diff] [review]
> Remove flushes that I don't think we need
> 
> Bas, does this seem sane to you?

Is this change OK with SkiaGL (Mac/Android)?

Comment 10

[Bas Schouten (:bas.schouten)]

Comment on attachment 8832496 [details] [diff] [review]
Remove flushes that I don't think we need

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

I don't see a problem with this.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90df369672f6
Remove unnecessary flush() calls from canvas implementation.

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/90df369672f6

Comment 13

[Chris Peterson [:cpeterson]]

Jeff, do you think this flush() fix is worth uplifting to Aurora or Beta?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Sure, let's up lift it. It's easy to back out if we find problems.

Comment 15

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8832496 [details] [diff] [review]
Remove flushes that I don't think we need

Approval Request Comment
[Feature/Bug causing the regression]: None particular
[User impact if declined]: Slower Google Sheets performance
[Is this code covered by automated tests?]: Yes it should be.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: Nope
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: It could potentially result in some wrong rendering in some situations but I don't know what they would be.
[Why is the change risky/not risky?]: If we're somehow depending on the flushes for some reason.
[String changes made/needed]: None

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8832496 [details] [diff] [review]
Remove flushes that I don't think we need

Fix slow Google Sheets performance. Take it in aurora first. Aurora53+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c73bb9843d79

Comment 18

[Milan Sreckovic [:milan] (needinfo for best results)]

Is the canvas retained mode (or whatever it's called) kicking in for Chrome?

Comment 19

[Jeff Muizelaar [:jrmuizel]]

(In reply to Milan Sreckovic [:milan] from comment #18)
> Is the canvas retained mode (or whatever it's called) kicking in for Chrome?

I think this is always the case.

Comment 20

[Jeff Muizelaar [:jrmuizel]]

Release Note Request (optional, but appreciated)
[Why is this notable]: Has a significant impact on Google Sheets performance
[Affects Firefox for Android]: Possibly at little bit
[Suggested wording]: Improve canvas text drawing performance.
[Links (documentation, blog post, etc)]:

Comment 21

[Julien Cristau [:jcristau]]

Comment on attachment 8832496 [details] [diff] [review]
Remove flushes that I don't think we need

fix a perf issue affecting google sheets, beta52+

Comment 22

[Julien Cristau [:jcristau]]

Hoping QE can do some testing around google sheets with 52.0b9 on Friday, so ni?avaida.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e2a6d2b4f1b4b8e0c77d057bc012fb20a18df2f5

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/e2a6d2b4f1b4

Comment 25

[Andrei Vaida [:avaida]]

(In reply to Julien Cristau [:jcristau] from comment #22)
> Hoping QE can do some testing around google sheets with 52.0b9 on Friday, so
> ni?avaida.

Will be covered as part of 52.0b9-build2 sign off, today. We'll follow up with test results as soon as possible -- leaving the ni? in place until this is done.

Comment 26

[Andrei Vaida [:avaida]]

(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #25)
> (In reply to Julien Cristau [:jcristau] from comment #22)
> > Hoping QE can do some testing around google sheets with 52.0b9 on Friday, so
> > ni?avaida.
> 
> Will be covered as part of 52.0b9-build2 sign off, today. We'll follow up
> with test results as soon as possible -- leaving the ni? in place until this
> is done.

This is done, Regression and Exploratory Testing around Google Spreadsheets on 52.0b9-build2 uncovered no issues.

Platforms covered: Windows 10 x86, Windows 8.1 x86, macOS 10.12.3 and Ubuntu 16.04 x64.

Comment 27

[Liz Henry (:lizzard) (relman/hg->git project)]

This fix shipped in 52, so I don't think it should go into 52 release notes.