Bug 12007 – SVGColor::setRGBColor color creates/deletes many strings, slowing down parsing

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

SVGColor::setRGBColor color creates/deletes many strings, slowing down parsing (for bamboo.svg)

Now that some of the other parsers are fixed, SVGColor::setRGBColor is one of the next hot-spots appearing in Shark when running the SVG PLT.  setRGBColor creates and deletes strings needlessly while parsing the color value.

Rob has already coded a fix:
http://paste.lisp.org/display/33499

Which looks fine to me.  r=me on the code change.

We still need some additional tests before he lands (e.g. to make sure that the change in behavior to not change the color to invalid when the parse fails).

http://paste.lisp.org/display/33500

was his first stab at a test case.  However we came up with a few improvements via IRC:
1.  call setRGBColor('green') first (to make sure setRGBColor actually works)
2.  wrap each call in some sort of try/catch block and check the exception (perhaps an expectsException() wrapper?)
3.  Test paints with leading spaces, commas, and spacing between digits and % (which should be invalid).

[Eric Seidel (no email)]

Thinking about this more, it isn't at all surprising that color parsing is our current hot spot.  What do SVG parsers/renderers do?  Parse lots of colors and lots of paths.  I expect even with Rob's improvements, this area will get hot again soon.

One of the major flaws in the current paint parsing/color parsing is how all parsing is done in the constructors.  That means you have to call new SVGPaint just to figure out if the string you have even has a valid color/paint in it.  That's silly.  We should parse the string first, then wrap it in a paint if it happens to be valid.

Ideally we would unify HTML and SVG color parsing.  There is little reason why 'aliceblue' should work in SVG and not in HTML for instance.  I believe the set of named colors is the only difference between SVG and HTML colors.  We could pass a bool into the common color parsing function to disable certain colors when parsing HTML if deemed necessary.

Rob's fix will work.  A long term solution will involve removing all the SVGPaint allocations and only allocating after parsing once we've determined we have a valid Color/SVGColor.

[Rob Buis]

Created attachment 12089 [details]
First attempt

This patch goes a long way in refactoring. It can go further, but that will have to wait until tomorrow :)
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 12089 [details]
First attempt

The patch looks pretty good.

The test case is busted.  You never actually use "value".

I don't think you need to init RGBA32 to transparent (at least not outside the parse function).

It's still unfortunate that we're creating new SVGPaints even for invalid colors.

[Rob Buis]

Created attachment 12100 [details]
Better patch

This should address most of the issues. Unfortunately we still seem to need the "empty" SVGPaint, since
we need to be able to differ between no fill specified at all or invalid fill specified. In the former case the
default or inherited value should be kept, in the latter case it should use black paint.
The testcase should be much better now.
Cheers,

Rob.

[Eric Seidel (no email)]

Comment on attachment 12100 [details]
Better patch

Looks great.

It appears you didn't change any of the HTML Color parsing code paths, which is good.

expectsValid needs to check the set value (make sure it set correctly).  And I think you coulud add a few more valid tests (for leading/tailing spaces, commas, etc.)

With a few more tests and the expectedValid fix, feel free to land!

[Eric Seidel (no email)]

landed in r18480