77941 - nsLookAndFeel::GetColor() inefficient

Status: RESOLVED FIXED

(Core :: XUL, defect, P4)

Description

[Chris Waterson]

There are two problems here:

1. The implementation of nsXPLookAndFeel::GetColor() is atrocious: it scans a
   list of two or three dozen colors that the user may have over-ridden in
   his/her prefs file. Most likely, the user will *not* have over-ridden them
   at all, so we have a worst-case lookup scenario.

   This should simply be a table keyed off the color ID so lookup is O(1).

2. The implementation of the window's version of nsLookAndFeel::GetColor()
   should cache the colors that it gets from the OS, and be smart about nuking
   the cache (e.g., respond to WM_SYSTEMPALETTECHANGED, or whatever).

   Typically, after we miss looking up the color in nsXPLookAndFeel, we call
   into the toolkit and burn an OS call!

Doing (2) may subsume doing (1). Discovered this while profiling long pages, cf.
bug 56854.

Comment 1

[Chris Waterson]

See also bug 77939.

Comment 2

[Brian Ryner (not reading)]

I can take this one.

Comment 3

[Brian Ryner (not reading)]

Just to complete the OS picture:

- On linux, we never need to make system calls from GetColor().  All of the
colors are pre-cached in mStyle or a static nscolor that we initialize the first
time an nsLookAndFeel is created.

- On Mac, it's a mixed bag.  Some colors are hardcoded, a few result in one or
two OS calls, and the one that's probably called the most during layout
(TextForeground), goes off into XPCOM-land and asks the nsIInternetConfigService
(which isn't cached), which asks the OS for the color (and doesn't cache it).

Setting to All/All.

Comment 4

[Brian Ryner (not reading)]

Patch coming up... this is basically a reorganization of how nsLookAndFeel
objects work. Instead of each platform nsLookAndFeel having a member variable
nsXPLookAndFeel, they each inherit from nsXPLookAndFeel.  nsXPLookAndFeel
implements caching in GetColor, which is done in a static array (note that the
color count is small, and this is definitely taking less space than
nsLookAndFeelColorPref array is currently).  Cache misses call a new virtual
method, NativeGetColor... all this needs to do is call through to the OS.

The one thing I didn't do is hook up system color-changed notifications to clear
the color cache.  This is a bigger problem because we would need to re-resolve
style on anything using a native system color.  I'm inclined to leave this out
for now... people rarely change system colors, and I don't think requiring an
app restart for that to take effect is a big deal.

I haven't been able to get good perf data for how much this actually helps (the
test page for 56584 seems to be permanently hanging mozilla on my machine if I
have the page saved locally).

Comment 5

[Brian Ryner (not reading)]

Attachment: patch

Comment 6

[Akkana Peck]

Good idea, and the changes look fine.  Inheriting is much better than including
in this case.

I was concerned about the lack of NS_DECL_ISUPPORTS_INHERITED and
NS_IMPL_{ADDREF,RELEASE}_INHERITED, but bryner pointed out that nsISupportsImpl
says that's only needed for classes that implement a new interface, which these
don't.

r=akkana

Comment 7

[Chris Waterson]

rs=waterson

Comment 8

[Brian Ryner (not reading)]

patch checked in. GetColor() should now be quite fast in most cases.

Comment 9

[Hixie (not reading bugmail)]

Is there a bug about responding to system notifications regarding colour changes
to nuke our cache and repaint everything?