RESOLVED FIXED 71292
CSSRule: Devirtualize cssText() 
https://bugs.webkit.org/show_bug.cgi?id=71292
Summary CSSRule: Devirtualize cssText()
Andreas Kling
Reported 2011-11-01 09:43:30 PDT
SSIA
Attachments
Proposed patch (6.82 KB, patch)
2011-11-01 09:45 PDT, Andreas Kling 		dbates: commit-queue-
DetailsFormatted DiffDiff
Proposed patch (7.04 KB, patch)
2011-11-02 06:38 PDT, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Andreas Kling
Comment 1 2011-11-01 09:45:26 PDT
Created attachment 113180 [details] Proposed patch
Luke Macpherson
Comment 2 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.
Andreas Kling
Comment 3 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!
Darin Adler
Comment 4 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.
Andreas Kling
Comment 5 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*).)
Daniel Bates
Comment 6 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
Andreas Kling
Comment 7 2011-11-02 06:38:48 PDT
Created attachment 113311 [details] Proposed patch
Antti Koivisto
Comment 8 2011-11-02 06:40:59 PDT
Comment on attachment 113311 [details] Proposed patch r=me
WebKit Review Bot
Comment 9 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>
WebKit Review Bot
Comment 10 2011-11-02 08:02:23 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.