Bug 71292 - CSSRule: Devirtualize cssText()
Summary: CSSRule: Devirtualize cssText()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andreas Kling
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-11-01 09:43 PDT by Andreas Kling
Modified: 2011-11-02 08:02 PDT (History)
4 users (show)

See Also:


Attachments
Proposed patch (6.82 KB, patch)
2011-11-01 09:45 PDT, Andreas Kling
dbates: commit-queue-
Details | Formatted Diff | Diff
Proposed patch (7.04 KB, patch)
2011-11-02 06:38 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andreas Kling 2011-11-01 09:43:30 PDT
SSIA
Comment 1 Andreas Kling 2011-11-01 09:45:26 PDT
Created attachment 113180 [details]
Proposed patch
Comment 2 Luke Macpherson 2011-11-01 09:53:17 PDT
Comment on attachment 113180 [details]
Proposed patch

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

> Source/WebCore/css/CSSRule.cpp:66
> +        ASSERT_NOT_REACHED();

If you remove the default case and put the ASSERT_NOT_REACHED() after the switch the compiler should produce a warning if any cases are missed.
Comment 3 Andreas Kling 2011-11-01 10:02:34 PDT
(In reply to comment #2)
> (From update of attachment 113180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113180&action=review
> 
> > Source/WebCore/css/CSSRule.cpp:66
> > +        ASSERT_NOT_REACHED();
> 
> If you remove the default case and put the ASSERT_NOT_REACHED() after the switch the compiler should produce a warning if any cases are missed.

Fair point!
Comment 4 Darin Adler 2011-11-01 10:44:29 PDT
Comment on attachment 113180 [details]
Proposed patch

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

> Source/WebCore/ChangeLog:8
> +        Have CSSRule::cssText() redirect to the appropriate subclass based on type().

This patch needs a “why” comment. I think the reason is that a switch statement based on bits is faster than a virtual function call? If so, normally we want performance measurement to go along with the patch.
Comment 5 Andreas Kling 2011-11-01 10:49:12 PDT
(In reply to comment #4)
> (From update of attachment 113180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=113180&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Have CSSRule::cssText() redirect to the appropriate subclass based on type().
> 
> This patch needs a “why” comment. I think the reason is that a switch statement based on bits is faster than a virtual function call? If so, normally we want performance measurement to go along with the patch.

Ah right. I'll gladly add one. (The primary reason is that I'm devirtualizing CSSRule completely to remove the vtable pointer, shrinking each instance by sizeof(void*).)
Comment 6 Daniel Bates 2011-11-01 12:46:38 PDT
Comment on attachment 113180 [details]
Proposed patch

Attachment 113180 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/10256028
Comment 7 Andreas Kling 2011-11-02 06:38:48 PDT
Created attachment 113311 [details]
Proposed patch
Comment 8 Antti Koivisto 2011-11-02 06:40:59 PDT
Comment on attachment 113311 [details]
Proposed patch

r=me
Comment 9 WebKit Review Bot 2011-11-02 08:02:19 PDT
Comment on attachment 113311 [details]
Proposed patch

Clearing flags on attachment: 113311

Committed r99059: <http://trac.webkit.org/changeset/99059>
Comment 10 WebKit Review Bot 2011-11-02 08:02:23 PDT
All reviewed patches have been landed.  Closing bug.