Bug 75714 - Remove inheritance of CSSValue from RefCounted.
Summary: Remove inheritance of CSSValue from RefCounted.
Status: RESOLVED WONTFIX
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexis Menard (darktears)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-06 10:38 PST by Alexis Menard (darktears)
Modified: 2012-02-02 21:35 PST (History)
7 users (show)

See Also:


Attachments
Patch (3.18 KB, patch)
2012-01-06 10:40 PST, Alexis Menard (darktears)
no flags Details | Formatted Diff | Diff
get rid of SHADOW_ROOT_NODE (21.43 KB, patch)
2012-02-02 21:31 PST, Hayato Ito
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexis Menard (darktears) 2012-01-06 10:38:33 PST
Remove inheritance of CSSValue from RefCounted.
Comment 1 Alexis Menard (darktears) 2012-01-06 10:40:14 PST
Created attachment 121446 [details]
Patch
Comment 2 Alexis Menard (darktears) 2012-01-06 10:42:19 PST
This bug is linked to the discussion of https://bugs.webkit.org/show_bug.cgi?id=75630
Comment 3 Alexis Menard (darktears) 2012-01-06 10:43:44 PST
More than 65536 references of the same CSSValue is unlikely to happen I would assume.
Comment 4 Luke Macpherson 2012-01-08 15:30:54 PST
Looks like a good idea to me. How about adding some ASSERTs to ensure that this doesn't overflow in practice?
Comment 5 Luke Macpherson 2012-01-08 15:57:31 PST
Comment on attachment 121446 [details]
Patch

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

> Source/WebCore/css/CSSValue.cpp:62
> +class SameSizeAsCSSValue {

I don't think this struct is needed now - it was primarily needed to account for the size added by extending RefCounted.

> Source/WebCore/css/CSSValue.h:49
> +        ++m_refCount;

ASSERT(m_refCount) here.

> Source/WebCore/css/CSSValue.h:-50
> -        if (derefBase())

ASSERT(m_refCount) here.

> Source/WebCore/css/CSSValue.h:189
>      unsigned char m_primitiveUnitType : 7; // CSSPrimitiveValue::UnitTypes

unsigned char and bool may need to be changed to unsigned so that MS compiler will pack them correctly.
Comment 6 Mike Lawther 2012-01-08 16:52:49 PST
(In reply to comment #3)
> More than 65536 references of the same CSSValue is unlikely to happen I would assume.

I tested this myself a couple of months ago. Unfortunately, it does happen.  From memory, I tested on tbe single-page HTML5 spec, and it did overflow 65536 references.
Comment 7 Mike Lawther 2012-01-08 17:12:49 PST
(In reply to comment #6)
> I tested this myself a couple of months ago [...]

Sorry - Luke reminded me that I actually tested dropping RefCounted itself's counter down to 16 bits in an attempt to reduce the size of CSSValue. *something* in WebKit often has more than 65536 references, but I cannot categorically state that it was a CSSValue that did.
Comment 8 Andreas Kling 2012-01-09 02:39:42 PST
Comment on attachment 121446 [details]
Patch

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

> Source/WebCore/css/CSSValue.h:186
> +    unsigned m_refCount : 16;

This can be an unsigned short, no need for it to be a bitfield.
Comment 9 Andreas Kling 2012-01-09 02:40:31 PST
(In reply to comment #7)
> (In reply to comment #6)
> > I tested this myself a couple of months ago [...]
> 
> Sorry - Luke reminded me that I actually tested dropping RefCounted itself's counter down to 16 bits in an attempt to reduce the size of CSSValue. *something* in WebKit often has more than 65536 references, but I cannot categorically state that it was a CSSValue that did.

Yeah, I'm not too sure this is safe. With CSSValuePool reusing the same CSSValue, I could see a popular value (0 or 1 perhaps) having a *lot* of refs.
Comment 10 Alexis Menard (darktears) 2012-01-09 02:43:13 PST
(In reply to comment #9)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > I tested this myself a couple of months ago [...]
> > 
> > Sorry - Luke reminded me that I actually tested dropping RefCounted itself's counter down to 16 bits in an attempt to reduce the size of CSSValue. *something* in WebKit often has more than 65536 references, but I cannot categorically state that it was a CSSValue that did.
> 
> Yeah, I'm not too sure this is safe. With CSSValuePool reusing the same CSSValue, I could see a popular value (0 or 1 perhaps) having a *lot* of refs.

Tony proposed https://bugs.webkit.org/show_bug.cgi?id=75725

I would say let's close that one.
Comment 11 Luke Macpherson 2012-01-09 15:13:10 PST
(In reply to comment #10)
> (In reply to comment #9)
> > (In reply to comment #7)
> > > (In reply to comment #6)
> > > > I tested this myself a couple of months ago [...]
> > > 
> > > Sorry - Luke reminded me that I actually tested dropping RefCounted itself's counter down to 16 bits in an attempt to reduce the size of CSSValue. *something* in WebKit often has more than 65536 references, but I cannot categorically state that it was a CSSValue that did.
> > 
> > Yeah, I'm not too sure this is safe. With CSSValuePool reusing the same CSSValue, I could see a popular value (0 or 1 perhaps) having a *lot* of refs.
> 
> Tony proposed https://bugs.webkit.org/show_bug.cgi?id=75725
> 
> I would say let's close that one.

It is unclear to me how that bug is related to this one. Reducing the size of the reference count would still be useful so long as the reference count does not overflow in practice. We need data to support how many references there actually are in practice to make that call.

I might pick this idea up and play with it some more.
Comment 12 Luke Macpherson 2012-01-09 17:59:19 PST
I added some instrumentation and did some surfing, the worst case I saw was 5200 references to a single CSSValue while using gmail.
Comment 13 Hayato Ito 2012-02-02 21:31:09 PST
Reopening to attach new patch.
Comment 14 Hayato Ito 2012-02-02 21:31:13 PST
Created attachment 125256 [details]
get rid of SHADOW_ROOT_NODE
Comment 15 Hayato Ito 2012-02-02 21:35:52 PST
I posted a patch at wrong issue. I meant 77514, not 75714.

I changed this but's status to 'WONTFIX', I guess this is the previous status. If this is wrong, please fix.(In reply to comment #14)

> Created an attachment (id=125256) [details]
> get rid of SHADOW_ROOT_NODE