[Webkit-unassigned] [Bug 54707] Introduce lookup-table based approach for applying CSS properties.

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=54707





--- Comment #67 from Antti Koivisto <koivisto at iki.fi>  2011-09-22 01:33:49 PST ---
(In reply to comment #62)
> I very much believe you.  This risk was discussed in this and other bugs.  Do you have a particular favorite tool which shows you this result, or is this derived from other data and your profiling experience?  If so, could you list the tools you used so that others (most importantly Luke) could also reproduce your results?

I was using Instruments to look the profile of page load of http://www.whatwg.org/specs/web-apps/current-work/. Very large sample counts show up in the virtual function call entry points. Not that that page does nothing special in terms styling, it is just big and complex enough to make style resolve performance easily observable on desktop computers. On ARM pretty much any complex page displays similar characteristics, observable by sampling.

> > Stylistically CSSStyleApplyProperty is pretty appalling jumble of jumps tables, virtual calls, function pointers and templates, a clear regression from the clarity of the original switch statement. The switch statement still exists in any case as this work was left in half-done state.
> 
> I'm not sure this stone is worth throwing.

Let me put it this way.

Style applying is analogous to HTML tree building. We have a stream of property tokens as input and we parse them to build up a RenderStyle object as output. You worked on HTML5 parser. The tree builder is implemented as large switch statements. Would you refactor it to use a lookup table (one slot per token) of instances of virtual classes?

Probably not. Yet the code here is often more important for performance than the HTML parser (> 2s  vs < 0.5s over the spec load).

I think architecturally this work has moved us to wrong direction and this is an important aspect of the discussion. Refactoring is costly (including the cost of discussion we are now having) and should not be attempted unless benefits are clear.

> What I hear you saying is that this performance regression needs to be adressed.  Revert is obviously one course of action, possibly the best one.  It seems a slightly premature conclusion unless you've spoken with the patch author? (I have not, I only know him through IRC.)
> 
> Thank you for providing a concrete test case.  On possible addressing of said regression would be to revert all this work of course.  I do not have enough data (besides of course your trusted -- but possibly hasty? -- recommendation) to know if that is the best course of action.

This is currently blocking us from doing the sensible refactoring (moving the style applying to a separate class and file as was done for SelectorChecker) as that would make any future reverting more complicated. Going back to switch looks like one of the easiest to achieve major performance progressions we have in the engine right now (purely mechanical work essentially) and would be worth doing on those ground alone.

> Antti:  Have you seen any slowdown in other benchmarks which your team may run more regularly, like the PLT?

Not that I know of. There have been progressions in selector matching performance big enough to easily mask out any regressions here.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.