[Webkit-unassigned] [Bug 46592] Convert CSSStyleSelector::applyProperty to use function pointers

[bugzilla-daemon at webkit.org]

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





--- Comment #20 from Luke Macpherson <macpherson at chromium.org>  2011-01-19 16:03:43 PST ---
1) Shouldn't this be done using a code generator?

The header file contents and switch statement can easily be autogenerated from the list of CSS properties. I intend to do that shortly.

It is not clear to me that using a code generator for the implementations would be beneficial in this case. I spent a few days implementing a generator before becoming convinced that it wasn't the right approach. The problem is the sheer amount of special case code that is contained here. The CSS property to RenderStyle mapping is not simple, and must be encoded somewhere. There are a substantial number of cases that must be hand-written individually.

2) Macros are hard to debug.

Yes and no - there is always a tradeoff between code reuse as enabled by macros and difficulty of understanding the generated output, however using a modern IDE with inline macro expansion means that this isn't as hard as it may first appear.

In any case, the existing code has at least as many macros as this code does, so it cannot be described as a net negative.

3) Are switch statements are faster on some architectures?

This isn't really an important distinction - if a switch statement is genuinely faster one can always rewrite a function pointer array access in the form of a switch statement. 
This CL as it stands is still using a switch statement, even though it would now be trivial to switch to a function pointer array.

http://software.intel.com/en-us/articles/branch-and-loop-reorganization-to-prevent-mispredicts/
"Since the BTB is only 16 entries long in the Pentium 4 processor, the prediction will eventually fail for loops that are longer than 16 iterations. This limitation can be avoided by unrolling a loop until it is only 16 iterations long. When this is done, a loop conditional will always fit into the BTB, and a branch misprediction will not occur on loop exit."

Therefore I am somewhat doubtful that branch prediction will have a measurable impact on a switch statement with more than 300 cases, and that neglecting the many branches contained within.

It also seems unlikely that ARM will see any difference in performance - it is a CPU with a much shorter pipeline and most use only static branch prediction.

4) There is no readability / maintainability / hackability gains in this CL.

applyProperty() has a very complicated control flow contained in a 2500 line switch statement that is exceedingly difficult to follow. It has hundreds of fall-through cases, return statements hidden inside macros, lots of unreachable code, and lots of nested switch statements where the parent and child variables are shared.

By splitting out each of the cases into individual functions it is possible to greatly clarify the flow and separation of this code, and this improves the readability, maintainability and hackability of this code.

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