1559437 - Drawing performance regression after merging bug 1464032

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Kacper Michajłow [:kasper93]]

Attachment: about_support.txt

Hi,

Since few day Firefox is unusable slow to draw. Here is profile https://perfht.ml/2RkocNR (from few seconds on vsynctester.com) I am attaching also about_support.txt with information about my config. I am currently on a trip so can't really test with other platforms, but on this Intel UHD 620 it is slideshow... instead of motion. I checked also on some very low end NVIDIA OEM GPU and result was similar. But I didn't compare to higher end hardware.

I bisected recent builds and here is offending change:
Bug 1464032 Part 15: Spread the playback of canvas recordings across multiple threads in the GPU process. r=mattwoodrow

I also had some GPU driver crashes while using FF recently, but I will report it in another bug. Need to reproduce first.

Comment 1

[Bob Owen (:bobowen) - PTO until 3rd April]

I'm fairly sure this is because I am getting a DataSourceSurface from the snapshot, when I shouldn't be.

Could you test the build at:
https://queue.taskcluster.net/v1/task/IW5RSWLKRZW72cH5mzGtLw/runs/0/artifacts/public/build/install/sea/target.installer.exe
to see if it fixes the problem, thanks.

You might want to run that with the following to create a profile for testing:
firefox -no-remote -P

Comment 2

[Kacper Michajłow [:kasper93]]

Yes, your new build resolves the main issue. Here is the profile if you are interested https://perfht.ml/2RhF1cc

Do you have some reference benchmark you would like me to test against? To see if there are maybe more areas to improve?

Comment 3

[Bob Owen (:bobowen) - PTO until 3rd April]

Attachment: Bug 1559437: Don't explicitly return DataSourceSurface from CanvasRenderingContext2D::GetSurfaceSnapshot.

Comment 5

[Bob Owen (:bobowen) - PTO until 3rd April]

(In reply to Kacper Michajłow [:kasper93] from comment #2)

Yes, your new build resolves the main issue. Here is the profile if you are interested https://perfht.ml/2RhF1cc

Do you have some reference benchmark you would like me to test against? To see if there are maybe more areas to improve?

Thanks for testing this, it's also been confirmed in another bug.

I don't have anything specific benchmark-wise, we have some of our own benchmark testing, but they didn't seem to pick this up.

The changes for bug 1464032 are actually for moving the accelerated canvas drawing to the GPU thread and are mainly behind a pref.
Obviously they do touch some existing code-paths and I mistakenly introduced this bug in a fairly late refactor during reviews.
Part of that was to avoid getting data back from the GPU process off the content main-thread, so I'll have to come up with a different way of doing that.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5fc7054fc05d
Don't explicitly return DataSourceSurface from CanvasRenderingContext2D::GetSurfaceSnapshot. r=mattwoodrow

Comment 7

[Kacper Michajłow [:kasper93]]

we have some of our own benchmark testing, but they didn't seem to pick this up.

This bothers me, I would expect a test to track this performance. I know there are a lot of automated tests, but it seems that none is triggered by this workload which make it easy to sneak regressions. Severe regression can easily be detected manually like this bug, but if it weren't so obvious it would probably be not detected for long time.

Anyhow, Thanks for the fix.

Comment 8

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5fc7054fc05d

Comment 10

[Cristian Fogel [:cfogel]]

Can confirm the reported issue & catastrphic vsync failure reported on https://www.vsynctester.com/ when using 69.0a1 (2019-06-14) - Windows 10.
Fix verified with 69.0b13, 70.0a1 (2019-08-12) on Windows 10, macOS 10.13, Ubuntu 18.04.