Bug 238654

<rdar://problem/91147450>

Created attachment 456326 [details] Patch

Created attachment 456327 [details] Patch

Created attachment 456435 [details] Patch

Comment on attachment 456435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456435&action=review > Source/WebCore/dom/EventTarget.h:146 > + mutable uint32_t m_nodeRefCountAndParentBit; It is really not pretty to have a Node-specific data member on EventTarget. Not only is it weird encapsulation but it also makes every subclass object that is not a Node unnecessarily larger. Maybe an alternative would be to have a boolean data member indicating that this EventTarget is a Node and then use downcast<Node>(*this).ref() / deref() ? It would look less odd IMHO.

(In reply to Chris Dumez from comment #5) > Maybe an alternative would be to have a boolean data member indicating that > this EventTarget is a Node and then use downcast<Node>(*this).ref() / > deref() ? It would look less odd IMHO. This also makes non-Node EventTargets larger, although adding just a boolean does mean that there is some opportunity for non-Node EventTargets to pack some of their own data up against the boolean. Node doesn't have any 1 or 2 byte members to pack in there, though, so this would make all Nodes bigger. I think we want to avoid that. Another alternative could be instead to move the flags word up to EventTarget. Only one flag bit would be relevant for EventTarget, and others would be relevant for Node and descendants. Similar to how NodeFlags has Element etc. specific flags on it. Would that be acceptable?

(In reply to Cameron McCormack (:heycam) from comment #6) > (In reply to Chris Dumez from comment #5) > > Maybe an alternative would be to have a boolean data member indicating that > > this EventTarget is a Node and then use downcast<Node>(*this).ref() / > > deref() ? It would look less odd IMHO. > > This also makes non-Node EventTargets larger, although adding just a boolean > does mean that there is some opportunity for non-Node EventTargets to pack > some of their own data up against the boolean. Node doesn't have any 1 or 2 > byte members to pack in there, though, so this would make all Nodes bigger. > I think we want to avoid that. > > Another alternative could be instead to move the flags word up to > EventTarget. Only one flag bit would be relevant for EventTarget, and > others would be relevant for Node and descendants. Similar to how NodeFlags > has Element etc. specific flags on it. > > Would that be acceptable? Hmm, I don't think moving the node flags up to EventTarget is any better factoring than moving the NodeRefCount. In my opinion, It is just too confusing to have Node-specific data members on EventTarget. It is true that increasing the size of Node would be unfortunate. That said, we did so recently when we started subclassing CanMakeWeakPtr<> and no-one noticed. For now, I do not have a better proposal than the one I made but I cc'd a couple of people who might. A 0.4% progression is nice but we have to weight it against the how bad it makes the code look.

(In reply to Chris Dumez from comment #7) > Hmm, I don't think moving the node flags up to EventTarget is any better > factoring than moving the NodeRefCount. In my opinion, It is just too > confusing to have Node-specific data members on EventTarget. I don't want to belabor the point too much, but that's kind of the situation that NodeFlag is already in. It has a bunch of flags relevant to all Nodes, but also ones that are only relevant to subclasses. Maybe conceptually the distance between EventTarget and Node is greater than between Node and {Element,Text,DocumentFragment}, which makes it seem worse. FWIW if the flags word lived on EventTarget, we could move EventTargetData::isFiringEventListeners there, so there would be two flags relevant to all EventTargets. That would also shrink all non-Node EventTargets (they all inherit from EventTargetWithInlineData). > It is true that increasing the size of Node would be unfortunate. That said, > we did so recently when we started subclassing CanMakeWeakPtr<> and no-one > noticed. If it's truly not an issue to grow all EventTargets by another word, then I will go with that. Interested to hear others' opinions too.

(In reply to Cameron McCormack (:heycam) from comment #8) > (In reply to Chris Dumez from comment #7) > > Hmm, I don't think moving the node flags up to EventTarget is any better > > factoring than moving the NodeRefCount. In my opinion, It is just too > > confusing to have Node-specific data members on EventTarget. > > I don't want to belabor the point too much, but that's kind of the situation > that NodeFlag is already in. It has a bunch of flags relevant to all Nodes, > but also ones that are only relevant to subclasses. Maybe conceptually the > distance between EventTarget and Node is greater than between Node and > {Element,Text,DocumentFragment}, which makes it seem worse. > > FWIW if the flags word lived on EventTarget, we could move > EventTargetData::isFiringEventListeners there, so there would be two flags > relevant to all EventTargets. That would also shrink all non-Node > EventTargets (they all inherit from EventTargetWithInlineData). > > > It is true that increasing the size of Node would be unfortunate. That said, > > we did so recently when we started subclassing CanMakeWeakPtr<> and no-one > > noticed. > > If it's truly not an issue to grow all EventTargets by another word, then I > will go with that. Interested to hear others' opinions too. I think we should avoid increasing the size. WeakPtr one can fix a lot of bugs, but in this case, benefit is readability. I think reducing these object size and keeping them small are very important and worth doing.

Comment on attachment 456435 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=456435&action=review >> Source/WebCore/dom/EventTarget.h:146 >> + mutable uint32_t m_nodeRefCountAndParentBit; > > It is really not pretty to have a Node-specific data member on EventTarget. Not only is it weird encapsulation but it also makes every subclass object that is not a Node unnecessarily larger. > > Maybe an alternative would be to have a boolean data member indicating that this EventTarget is a Node and then use downcast<Node>(*this).ref() / deref() ? It would look less odd IMHO. Especially given the existing complexity of node refcounting, I agree with the goal of retaining encapsulation. Checking a bit and down casting sounds good to me. That's still pretty well encapsulated. The only thing that leaks out is knowing that we have a subclass, but not any of its implementation details.

Created attachment 456501 [details] Patch (v2) Here's a version that moves the flags word up to EventTarget, but keeps all of the Node-specific flag definitions on Node. I like this a lot more than the previous patch.

Created attachment 456502 [details] Patch (v2)

I thought of a way to put an "isNode" bit in EventTarget, without needing to move all the other bits for space. 1. EventTarget removes its CanMakeWeakPtr base class 2. EventTarget adds a WeakPtrFactory data member and re-exports the createWeakPtr() function 3. WeakPtrFactory uses PackedRefPtr<T> or CompactPointerTuple or whatever to make the low bits of its RefPtr usable, and offers an interface for getting/setting those bits, and EventTarget uses one bit to store the "isNode" state.

That sounds good. I will probably still want to cram those three bits my v2 patch adds in there. But we avoid the funky "reinterpret EventTargetFlags and NodeFlags".

Created attachment 460191 [details] Patch (v3.1) This is the latest refinement of the patch I tried. But it didn't seem to help much.

*** Bug 241542 has been marked as a duplicate of this bug. ***

I started a new A/B test of Speedometer 2 with a rebased/tweaked version of Cameron’s latest attempt.

Devirtualizing ref() and deref() was done in bug 254113. Devirtualizing eventTargetData() was done in bug 244710.