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).
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.
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.
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.
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.
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!
landed in r18480