769993 - 20% performance regression on IE Maze If gfx.content.azure.enabled=true

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/f08d285b63b0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/16.0 Firefox/16.0 ID:20120630030532

20% performance regression on IE Maze If gfx.content.azure.enabled=true

gfx.content.azure.enabled=true  : 136sec
gfx.content.azure.enabled=false : 111sec

Steps to Reproduce:
1. Prepare a new profile and make sure setting of gfx.content.azure.enabled
2. Open http://ie.microsoft.com/testdrive/Performance/MazeSolver/Default.html
3. Click "Start"

Actual Results:
 20% slow, If gfx.content.azure.enabled=true

Expected Results:
 Not so.

Comment 1

[Alice0775 White]

Graphics
Adapter Description : ATI Radeon HD 4300/4500 Series
Vendor ID : 0x1002
Device ID : 0x954f
Adapter RAM : 512
Adapter Driver : saticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
Driver Version : 8.961.0.0
Driver Date : 4-5-2012
Direct2D Enabled : true
DirectWrite Enabled : true (6.1.7601.17789)
ClearType Parameters : Gamma: 2200 Pixel Structure: RGB ClearType Level: 50 Enhanced Contrast: 50 
WebGL Renderer : Google Inc. -- ANGLE (ATI Radeon HD 4300/4500 Series) -- OpenGL ES 2.0 (ANGLE 1.0.0.1041)
GPU Accelerated Windows : 1/1 Direct3D 10
AzureBackend : direct2d

Comment 2

[Bas Schouten (:bas.schouten)]

Although I've found a fix to this specific regression. It's revealed a couple of more serious issues with the Azure-thebes wrapper. They're not too hard to fix, however for now I suggest we switch off the wrapper for 15. We've gotten enough value from its testing and right now it doesn't offer any big wins (as expected).

Comment 3

[Bas Schouten (:bas.schouten)]

Attachment: Switch off Azure content in Beta

This is turning off a new feature to fix some regressions we're still working on fixing.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 647400 [details] [diff] [review]
Switch off Azure content in Beta

Approving for disabling this on beta, please set status flags to reflect this status once it lands.

Comment 5

[Bas Schouten (:bas.schouten)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5f2f7027b65b

Accidentally put the wrong person for approval in the commit message.

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Bas, what's happening here?

Comment 7

[Bas Schouten (:bas.schouten)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #6)
> Bas, what's happening here?

So, the main reason Azure-content is slower here is that Layout is saving. pushing a clip, and then restoring, without actually doing any drawing. In Cairo this would result in nothing actually happening, in Azure-content the clip is naively pushed/popped causing a significant performance penalty. I've got a patch that 'fixes' this problem, it really just works around it from layout land (i.e. it doesn't clip if it can easily determine it won't be drawing anything).

Additionally, after the benchmark finishes we suffer considerably with Azure-content due to a very small clip being applied and than a PushGroup/PopGroup occurring numerous times. With cairo the intermediate blend would be limited to the bounds of the clip, with current Azure-content it's not. I've uploaded a patch for this in another bug (See bug 778367).

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Bas Schouten (:bas) from comment #7)
> So, the main reason Azure-content is slower here is that Layout is saving.
> pushing a clip, and then restoring, without actually doing any drawing. In
> Cairo this would result in nothing actually happening, in Azure-content the
> clip is naively pushed/popped causing a significant performance penalty.
> I've got a patch that 'fixes' this problem, it really just works around it
> from layout land (i.e. it doesn't clip if it can easily determine it won't
> be drawing anything).

Avoiding it in layout is OK, but this could also be triggered by scripts using canvas, right?

> Additionally, after the benchmark finishes we suffer considerably with
> Azure-content due to a very small clip being applied and than a
> PushGroup/PopGroup occurring numerous times. With cairo the intermediate
> blend would be limited to the bounds of the clip, with current Azure-content
> it's not. I've uploaded a patch for this in another bug (See bug 778367).

Great, thanks.

Comment 9

[Alex Keybl [:akeybl]]

Are we planning to perform the same disable in FF16 that we did in FF15? Or are we still trying to uplift bug 778367.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Are we planning to perform the same disable in FF16 that we did in FF15? Or
> are we still trying to uplift bug 778367.

I would like to uplift bug 778367, but I'm not 100% sure we should. Unless we think this bug is a big issue on beta I'd like to pref off later in the cycle so we get some more test coverage for the next cycle, where we'll actually start seeing real improvements from it being preffed on.

Comment 11

[Alex Keybl [:akeybl]]

(In reply to Bas Schouten (:bas) from comment #10)
> I would like to uplift bug 778367, but I'm not 100% sure we should. Unless
> we think this bug is a big issue on beta I'd like to pref off later in the
> cycle so we get some more test coverage for the next cycle, where we'll
> actually start seeing real improvements from it being preffed on.

Will wait to hear your risk vs reward of bug 778367. If that doesn't pan out, we should land the same preference change 9/18 (b4's go to build).

Comment 12

[Alex Keybl [:akeybl]]

Spoke with Bas - we agreed to again disable azure for FF16 to prevent new perf regressions. Let's land this before Tuesday.

Comment 13

[Alex Keybl [:akeybl]]

Comment on attachment 647400 [details] [diff] [review]
Switch off Azure content in Beta

Approving for beta again. a=akeybl

Comment 14

[Scoobidiver (away)]

Landed in Beta: http://hg.mozilla.org/releases/mozilla-beta/rev/d1bdd265730b

Comment 15

[Bas Schouten (:bas.schouten)]

I discussed this with Lukas a little on IRC. There's a fairly simple, safe 'hack' we can do to 'fix' this regression. But I personally don't think it's worth it. This is a 10-20% regression in a benchmark where we're already 400% slower than the competition. If we value this benchmark we should probably focus on improving the actual layout algorithm here which is eating all of the time, that will automatically take care of the regression.

Comment 16

[Dão Gottwald [:dao]]

If we value this benchmark, then probably because it's a good proxy for real applications, where a 10-20% regression might be significant regardless of how much worse we are than our competitors. Can we rule this out? Would the hack you mention only affect this benchmark or real-world use cases too?

Comment 17

[Bas Schouten (:bas.schouten)]

(In reply to Dão Gottwald [:dao] from comment #16)
> If we value this benchmark, then probably because it's a good proxy for real
> applications, where a 10-20% regression might be significant regardless of
> how much worse we are than our competitors. Can we rule this out? Would the
> hack you mention only affect this benchmark or real-world use cases too?

I don't imagine that this would ever happen in the real world. But the layout people would know better than I do. If something like this would happen in the real world performance would be pretty bad anyway and it would probably be somewhere on our radar. I'm happy to being convinced otherwise, if we can find a real-world example.

Comment 18

[JP Rosevear [:jpr]]

Jeff had a real world example at one point.

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Beta 4 is going out later this week with all of Bas's recent work in it, we'll be meeting next Monday (Nov 5) to assess if we'll need to do the disabling again in 17 based on feedback and testing over the weekend.

Comment 20

[Lukas Blakk [:lsblakk] use ?needinfo]

The fixes have landed, and are looking good with no regressions.  However, for the perf hit - as Bas mentioned in comment 15, we're looking at a slight perf hit on something we're already not performing well on. It's not worth pushing further on this right now for release tracking purposes.

Comment 21

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Tha patches landed. Is there some more work to do here or we can close is as RESOLVED?

Comment 22

[Bas Schouten (:bas.schouten)]

Don't think so.