Remove inheritance of CSSValue from RefCounted.
Created attachment 121446 [details] Patch
This bug is linked to the discussion of https://bugs.webkit.org/show_bug.cgi?id=75630
More than 65536 references of the same CSSValue is unlikely to happen I would assume.
Looks like a good idea to me. How about adding some ASSERTs to ensure that this doesn't overflow in practice?
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.
(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.
(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 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.
(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.
(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.
(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.
I added some instrumentation and did some surfing, the worst case I saw was 5200 references to a single CSSValue while using gmail.
Reopening to attach new patch.
Created attachment 125256 [details] get rid of SHADOW_ROOT_NODE
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