[Webkit-unassigned] [Bug 43783] Make CSS Style Selector non-recursive

[bugzilla-daemon at webkit.org]

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





--- Comment #11 from Hayato Ito <hayato at chromium.org>  2010-09-01 05:07:09 PST ---
Thank you for the review!

(In reply to comment #9)
> (From update of attachment 64069 [details])
> This looks like a good idea. We need to measure if it makes things faster or slower. Also, can we construct a test case demonstrating that the old algorithm runs out of stack space if nested too deeply, or is that not practical?

I am glad to hear that this is a step in the right direction.

I've tried i-Bench and run the performance tests against with-this-patch and without-this-patch.
But my impression about this benchmark is that the score of the performance test is unstable. So it looks difficult to me to judge this patch is good for the performance. Hmm. My understanding and/or usage of i-Bench might be wrong.

I'll attach the result of performance tests for reference.

As for a test case, I've added a testcase in this patch. That test contains deeply nested HTML and long CSS selector. That should not happen in practical, I guess. If we increate a nest level in the test case, WebKit without this patch will crash.
The bad thing about this test case is that it takes too much time, over 1 minute (!), until the test runs out of stack space. So I am afraid that it is inappropriate to add this test to LayoutTest directory as is.


> 
> > +class CSSStyleSelector::SelectorChecker::CallState {
> 
> If this is going to have public data members, lets have it be a struct and not use the "m_" prefix. Or make the data members private.

Thank you. I've changed it to a struct and removed 'm_' prefix from the member variables.
Note that I've appended an underscore, '_', to each formal parameter of the constructor to avoid naming conflicts with member variables. I am nor sure that that there is any established naming convention for formal parameters of such a constructor.


> 
> > +    State m_state;
> > +    CSSSelector* m_sel;
> 
> Lets spell it out: selector, not sel.

Done.

> 
> > +    Element* m_e;
> 
> Lets spell it out: element, not e.


Done.

> 
> > +    CallState(State state, CSSSelector* sel, Element* e, bool isSubSelector, bool encounteredLink, RenderStyle* elementStyle, RenderStyle* elementParentStyle)
> > +            : m_state(state)
> 
> Not sure why this is indented 8 spaces.

Done. Thank you!

> 
> > +    void push(CallState state)
> > +    {
> > +        m_stack.append(state);
> > +    }
> 
> Cuts down on copying if this takes a const CallState&.

Done.

> 
> > +private:
> > +    Vector<CallState> m_stack;
> 
> It may improve performance to use Vector<CallState, 20> here or some other number that's usually larger than the typical depth of the stack.

Done. I think so.

> 
> Is there a way to make this patch have a smaller diff, by doing enough of this to change the indentation level separately? All those lines of code marked different that are not different make the patch too hard to read.

I agree that this patch is hard to read. I am sorry for that. Though I am not sure that I understand your suggestion correctly, I don't have any good ideas to make this patch have a smaller diff. You mean that it might be okay to change indent level of lines, breaking code styles temporarily, for easy reviewing?
I'd like to make this patch easy to read as possible as I can. Any comments are welcome.


> 
> Normally, we do not use "const bool".

Done.

> 
> I'm going to say review- for this patch, but it looks like a step in the right direction.

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