593812 - <tree> causes CPU load spikes when D2D is enabled

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Nils Maier [:nmaier]]

Attachment: simple test-case

<tree>/nsITree performance regressed on Windows7 when D2D is enabled.
Whenever the tree gets repainted (cell/row invalidated by e.g. scrolling, selection changes or by the program), especially if there are progressmeter cells, CPU usage spikes, leaving the UI thread to consume an entire CPU core for up to 2 seconds.

Steps:
* Open testcase or Seamonkey download manager listing some downloads
* Scroll a bit around, select some rows
* See CPU load spikes consuming (almost) an entire core

Before regression (no D2D):
* No major CPU load spikes and smooth user interaction possible.

This is not related to full DOM trees, but also affects virtual ones (implementing nsITree).

We're affected by this in our DownThemAll! extension, which uses nsITree all over the place.
The Seamonkey Download manager is affected too, as it uses <tree> + progressmeters as well.
The Places bookmarks library is affected as well to some extend, but showing less serious load spikes, probably because it does not use progressmeter cells. But there is clearly a perf regression as well.

D3D (as enabled in recent nightlies by layers.accelerate-all) does NOT mitigate the CPU load spikes.


I initially determined the regression range:
Nightly good d5e211bdd793 (20100813041308)
Nightly BAD 656d99ca089c (20100814040443)
And then hg bisect'ed it to changeset 536ac334d335, part of bug 573229 to enable D2D by default.

Might be related to bug 593700, but this one regressed a few weeks before already.

Machine
Core 2 Quad, 8GB RAM
Windows 7 x64, fully patched

Graphics
Adapter Description
	ATI Radeon HD 3600 Series
Vendor ID
	1002
Device ID
	9598
Adapter RAM
	512
Adapter Drivers
	aticfx64 aticfx64 aticfx32 aticfx32 atiumd64 atidxx64 atiumdag atidxx32 atiumdva atiumd6a atitmm64
Driver Version
	8.732.0.0
Driver Date
	5-4-2010
Direct2D Enabled
	true
DirectWrite Enabled
	true
GPU Accelerated Layers Enabled (as most recent nightly)
	3/3 Direct3D 9

Comment 1

[Joe Drew (not getting mail)]

This might be border related, says Bas. Hopefully fixes to border rendering will help this out. However, I don't think Firefox uses this, so for now we won't block on it.

Comment 2

[Boris Zbarsky [:bzbarsky]]

I don't think that's reasonable.  Thunderbird _does_ use trees, and you're making a Gecko blocking decision here, not a Firefox one.

And for that matter, comment 0 mentions a part of Firefox where performance has also regressed due to this bug....

Renominating; if you think this shouldn't block that's your decision, but I want to make sure we're not making a decision based on incorrect premises here.

Comment 3

[Nils Maier [:nmaier]]

(In reply to comment #1)
> This might be border related, says Bas. Hopefully fixes to border rendering
> will help this out. However, I don't think Firefox uses this, so for now we
> won't block on it.

Firefox might not use trees in the main UI, but in Places.
Thunderbird (main window) and Seamonkey (Downloads, Places) use trees as well.
Both Adblock Plus (Filter selection, Preferences/Main window) and DownThemAll! (download selection, download manager) use trees, which have have 11+ and 2+ million users respectively alone. And likely a lot of more extensions use trees as well.
So this will have a direct impact on millions of users if not fixed.

There is no workaround, except turning off all hardware acceleration.

Comment 4

[Joe Drew (not getting mail)]

I guess we need to look into border rendering sooner than later.

Comment 5

[Bas Schouten (:bas.schouten)]

This is likely all borders. Need to verify this.

Comment 6

[Bas Schouten (:bas.schouten)]

I've confirmed this is due completely to border drawing (in this testcase the progressmeter borders). My current border rendering patches do not catch these borders. With those borders using a (still incorrect) fast path is scrolls smoothly, scrolling through the testcase still uses full CPU when actively done, but this is the same for 3.6 when scrolling actively.

Comment 7

[Bas Schouten (:bas.schouten)]

I'm making this test case fast by adding a fast path for moz-border-colors without rounded corners(which is not a bad idea in any case). However I'm not sure why these progress meters are using moz-border-colors at all, they're drawing a 2 pixel width border, and the second moz-border-color is the background color, essentially the second pixel is doing nothing as far as I can see (from inside the border renderer, maybe there's something situation in which this has a function).

Comment 8

[Nils Maier [:nmaier]]

(In reply to comment #7)
> I'm making this test case fast by adding a fast path for moz-border-colors
> without rounded corners(which is not a bad idea in any case). However I'm not
> sure why these progress meters are using moz-border-colors at all, they're
> drawing a 2 pixel width border, and the second moz-border-color is the
> background color, essentially the second pixel is doing nothing as far as I can
> see (from inside the border renderer, maybe there's something situation in
> which this has a function).

Dropping the inner border color in favor of a simple 1px padding will work for winstripe/gnomestripe, but not on pinestripe, where the inner color is black, and the background color changes for even and odd rows.
Is this worth pursuing for win/gnomestripe or will your work to add fast path for regular shaped borders be enough?

Comment 9

[Bas Schouten (:bas.schouten)]

My patch will certainly improve things, but there's still be some win in dropping it. For all rendering systems btw, not just D2D.

Comment 10

[Nils Maier [:nmaier]]

Ok, filed bug 611073 (with patch)

Comment 11

[Bas Schouten (:bas.schouten)]

This should be fixed with the landing of 588271.