Bug 88222 - Improve the performance of pushScope in StyleResolver
Summary: Improve the performance of pushScope in StyleResolver
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-04 04:00 PDT by Takashi Sakamoto
Modified: 2012-06-07 19:06 PDT (History)
6 users (show)

See Also:


Attachments
Patch (2.90 KB, patch)
2012-06-04 04:24 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff
Patch (2.76 KB, patch)
2012-06-07 04:29 PDT, Takashi Sakamoto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Takashi Sakamoto 2012-06-04 04:00:22 PDT
As StyleResolver::setupScopeStack has a bug that m_scopeStackParent is always set to be NULL, setupScopeStack is always invoked if scoped author styles exist.
Comment 1 Takashi Sakamoto 2012-06-04 04:24:11 PDT
Created attachment 145558 [details]
Patch
Comment 2 Hajime Morrita 2012-06-04 18:10:40 PDT
Comment on attachment 145558 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review

> Source/WebCore/ChangeLog:10
> +        Reviewed by NOBODY (OOPS!).

The "Reviewed By" line should be jut after the bug URL. See others.

> Source/WebCore/css/StyleResolver.cpp:563
>      for (; parent; parent = parent->parentOrHostNode()) {

It would be better to introduce new local variable for this loop instead of reusing the parameter.
It's generally a bad style to reuse it and that is a source of this bug.

> Source/WebCore/css/StyleResolver.cpp:597
> +            const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1];

Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause.
Also, why do we need isEmpty() check?
Comment 3 Takashi Sakamoto 2012-06-07 04:29:04 PDT
Created attachment 146251 [details]
Patch
Comment 4 Takashi Sakamoto 2012-06-07 04:34:57 PDT
(In reply to comment #2)
> (From update of attachment 145558 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=145558&action=review
> 
> > Source/WebCore/ChangeLog:10
> > +        Reviewed by NOBODY (OOPS!).
> 
> The "Reviewed By" line should be jut after the bug URL. See others.

Done. However, I think, this is the default order generated by prepare-ChangeLog script.

> > Source/WebCore/css/StyleResolver.cpp:563
> >      for (; parent; parent = parent->parentOrHostNode()) {
> 
> It would be better to introduce new local variable for this loop instead of reusing the parameter.
> It's generally a bad style to reuse it and that is a source of this bug.

I see. Done.

> > Source/WebCore/css/StyleResolver.cpp:597
> > +            const ScopeStackFrame& lastFrame = m_scopeStack[m_scopeStack.size() - 1];
> 
> Vector::last() is preferred. Then it's easy to pack the conditional into the single if clause.

Done.

> Also, why do we need isEmpty() check?

Yes, we need. Because pushScope doesn't add any ruleset if a scope doesn't have any rules, but m_scopeStackParent is always updated to avoid ruleset recalculation.

And I also tested whether we could remove isEmpty() check or not and obtained many crash logs.

Best regards,
Takashi Sakamoto
Comment 5 Hajime Morrita 2012-06-07 18:34:01 PDT
Comment on attachment 146251 [details]
Patch

Okay, let's land this.
Comment 6 WebKit Review Bot 2012-06-07 19:06:32 PDT
Comment on attachment 146251 [details]
Patch

Clearing flags on attachment: 146251

Committed r119782: <http://trac.webkit.org/changeset/119782>
Comment 7 WebKit Review Bot 2012-06-07 19:06:38 PDT
All reviewed patches have been landed.  Closing bug.