Bug 71382 - Devirtualize CSSRule.
Summary: Devirtualize CSSRule.
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-02 12:09 PDT by Andreas Kling
Modified: 2011-11-03 05:15 PDT (History)
4 users (show)

See Also:


Attachments
Possibly a patch (10.17 KB, patch)
2011-11-02 12:12 PDT, Andreas Kling
koivisto: review+
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-02 12:09:51 PDT
The only remaining virtual method in CSSRule is the destructor.
If we make it non-virtual, each instance will shrink by one pointer (the vptr.)
Comment 1 Andreas Kling 2011-11-02 12:12:53 PDT
Created attachment 113347 [details]
Possibly a patch
Comment 2 Antti Koivisto 2011-11-02 12:30:14 PDT
Comment on attachment 113347 [details]
Possibly a patch

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

r=me with a few comments.

> Source/WebCore/css/CSSImportRule.h:60
> +    class ImportedStyleSheetClient : public CachedStyleSheetClient {

This could use a comment explaining why the class itself can't implement the interface.

> Source/WebCore/css/CSSRule.cpp:76
> +void CSSRule::deref()
> +{
> +    if (!derefBase())
> +        return;
> +    switch (type()) {
> +    case UNKNOWN_RULE:

The deletion part should be factored into a function. deref() can be inlined.

> Source/WebCore/css/CSSRule.h:40
> +    // Override RefCounted's deref() to ensure operator delete is called on
> +    // the appropriate subclass type.
> +    void deref();

You should explain that this class is non-virtual for a reason.
Comment 3 Darin Adler 2011-11-02 14:46:57 PDT
Comment on attachment 113347 [details]
Possibly a patch

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

> Source/WebCore/css/CSSRule.h:36
> +    ~CSSRule() { }

This base class destructor should be protected.
Comment 4 Antti Koivisto 2011-11-02 16:15:09 PDT
Maybe it would make sense to have RefCounted do deleting by calling

static_cast<T*>(this)->destruct();

instead of calling delete directly (with the default implementation just doing delete)? This way you would only need to override destruct() in cases like this. We may want to devirtualize some more in the future...
Comment 5 Darin Adler 2011-11-02 16:28:25 PDT
(In reply to comment #4)
> Maybe it would make sense to have RefCounted do deleting by calling
> 
> static_cast<T*>(this)->destruct();
> 
> instead of calling delete directly (with the default implementation just doing delete)? This way you would only need to override destruct() in cases like this. We may want to devirtualize some more in the future.

Seems OK.

I would call the function “destroy”, not “destruct”, though.
Comment 6 Andreas Kling 2011-11-03 05:15:52 PDT
Committed r99171: <http://trac.webkit.org/changeset/99171>