Closed Bug 1335149 Opened 3 years ago Closed 3 years ago

Slow drawing on this canvas example

Categories

(Core :: Graphics, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
platform-rel --- +
relnote-firefox --- -
firefox51 --- wontfix
firefox52 --- verified
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: overholt, Assigned: jrmuizel, NeedInfo)

Details

(Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets][gfx-noted])

Attachments

(3 files)

Attached file 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.
(Note that the performance is a bit difference when the grid() function is in a separate js file)
Attached image unnecessaryflush.PNG
Jeff Muizelaar said we're doing a flush when we shouldn't be (see profile screenshot attached).

Profile: https://clptr.io/2kk4W4s
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:+)
platform-rel: --- → +
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets]
(In reply to Jeff Muizelaar [:jrmuizel] from comment #4)
> Does this build help?
> https://archive.mozilla.org/pub/firefox/try-builds/jmuizelaar@mozilla.com-
> 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.
Bas, does this seem sane to you?
Attachment #8832496 - Flags: review?(bas)
(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/jmuizelaar@mozilla.com-
> > 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.
Edgard, see comment 7 above from Jeff. What do you think?
Flags: needinfo?(edgard)
(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)?
Priority: -- → P3
Whiteboard: [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets] → [platform-rel-Google][platform-rel-GoogleSuite][platform-rel-GoogleSheets][gfx-noted]
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.
Attachment #8832496 - Flags: review?(bas) → review+
Pushed by jmuizelaar@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/90df369672f6
Remove unnecessary flush() calls from canvas implementation.
https://hg.mozilla.org/mozilla-central/rev/90df369672f6
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Jeff, do you think this flush() fix is worth uplifting to Aurora or Beta?
Assignee: nobody → jmuizelaar
Flags: needinfo?(jmuizelaar)
Sure, let's up lift it. It's easy to back out if we find problems.
Flags: needinfo?(jmuizelaar)
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
Attachment #8832496 - Flags: approval-mozilla-beta?
Attachment #8832496 - Flags: approval-mozilla-aurora?
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+.
Attachment #8832496 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Is the canvas retained mode (or whatever it's called) kicking in for Chrome?
(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.
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)]:
relnote-firefox: --- → ?
Comment on attachment 8832496 [details] [diff] [review]
Remove flushes that I don't think we need

fix a perf issue affecting google sheets, beta52+
Attachment #8832496 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hoping QE can do some testing around google sheets with 52.0b9 on Friday, so ni?avaida.
Flags: needinfo?(andrei.vaida)
(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.
Flags: qe-verify+
(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.
Flags: qe-verify+
Flags: needinfo?(andrei.vaida)
This fix shipped in 52, so I don't think it should go into 52 release notes.
You need to log in before you can comment on or make changes to this bug.