Bug 75714 – Remove inheritance of CSSValue from RefCounted.

• Component: CSS
• Status: RESOLVED WONTFIX

[Alexis Menard (darktears)]

Remove inheritance of CSSValue from RefCounted.

[Alexis Menard (darktears)]

Created attachment 121446 [details]
Patch

[Alexis Menard (darktears)]

This bug is linked to the discussion of https://bugs.webkit.org/show_bug.cgi?id=75630

[Alexis Menard (darktears)]

More than 65536 references of the same CSSValue is unlikely to happen I would assume.

[Luke Macpherson]

Looks like a good idea to me. How about adding some ASSERTs to ensure that this doesn't overflow in practice?

[Luke Macpherson]

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.

[Mike Lawther]

(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.

[Mike Lawther]

(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.

[Andreas Kling]

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.

[Andreas Kling]

(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.

[Alexis Menard (darktears)]

(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.

[Luke Macpherson]

(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.

[Luke Macpherson]

I added some instrumentation and did some surfing, the worst case I saw was 5200 references to a single CSSValue while using gmail.

[Hayato Ito]

Reopening to attach new patch.

[Hayato Ito]

Created attachment 125256 [details]
get rid of SHADOW_ROOT_NODE

[Hayato Ito]

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