Bug 42073 – CSS color parsing optimizations

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Andreas Kling]

Currently, parseColor() creates a CSSParser every time it's called. This is not always necessary.

Also, the fast path for #RRGGBB (or #RGB) colors doesn't work for color strings actually starting with '#'

[Andreas Kling]

Created attachment 61214 [details]
Proposed patch

[Oliver Hunt]

Comment on attachment 61214 [details]
Proposed patch

Do you have perf measurements for this? There was a canvas perf test from hixie that the #color optimisation was made for, are we sure it doesn't work? or was it regressed at some point?

[Andreas Kling]

(In reply to comment #2)
> was it regressed at some point?

This regressed with https://bugs.webkit.org/show_bug.cgi?id=38845 where I unintentionally removed your optimization from https://bugs.webkit.org/show_bug.cgi?id=3781

[Andreas Kling]

Hixies test: http://hixie.ch/tests/adhoc/perf/video/002.html

Without this patch (current trunk):

  Elapsed wall-clock time: 1325ms (ideal: 640ms).
  Elapsed non-idle time: 685ms (ideal: 0ms).
  Speed: 11.32fps (ideal: 25.00fps).

With this patch:

  Elapsed wall-clock time: 1011ms (ideal: 640ms).
  Elapsed non-idle time: 371ms (ideal: 0ms).
  Speed: 14.84fps (ideal: 25.00fps).

[Oliver Hunt]

Comment on attachment 61214 [details]
Proposed patch

I would prefer it if you had a separate patch to revert the #color parsing regression

[Andreas Kling]

(In reply to comment #5)
> I would prefer it if you had a separate patch to revert the #color parsing regression

I'm not sure we want to revert that. Condensing the color parsing to one call (CSSParser::parseColor()) from the Canvas side is still desirable IMO.

Better to fix parseColor() to take the appropriate fast-path instead of doing the Color(string) trick in a bunch of places.

[Andreas Kling]

Created attachment 61243 [details]
Proposed patch v2

Updated patch to also avoid allocating a new string when getting the RRGGBB (or RGB) part of a '#' color.

  Elapsed wall-clock time: 996ms (ideal: 640ms).
  Elapsed non-idle time: 356ms (ideal: 0ms).
  Speed: 15.06fps (ideal: 25.00fps).

[WebKit Commit Bot]

Comment on attachment 61243 [details]
Proposed patch v2

Clearing flags on attachment: 61243

Committed r63099: <http://trac.webkit.org/changeset/63099>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 61243 [details]
Proposed patch v2

Adding an include of PlatformString.h to Color.h is not correct. Why was that done?

[Andreas Kling]

(In reply to comment #10)
> Adding an include of PlatformString.h to Color.h is not correct. Why was that done?

For UChar. What is the correct way?

[Eric Seidel (no email)]

wtf/unicode/Unicode.h is how you get UChar.

[Andreas Kling]

Right, thanks. Filed bug 42109 with a fix.