334075 - Add storage for slots in nsGenericDOMDataNode

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So here's a somewhat controversial patch. The idea is to give nsGenericDOMDataNode the ability to store extra information in a slots struct just like nsGenericElement can.

One big reason I want to do this is that I want to be able to store a bindingParent for textnodes. Right now the fact that we just get the bindingparent from the parent actually causes some bugs when you have textnodes in the root of a binding.

Another good thing about this is that we for free get 32 fresh new flags to store information in and won't have to cram a bunch of bits into the single bit we have left over in mParentPtrBits. In fact, this makes it possible to move mFlagsOrSlots into nsIContent and potentially inline a bunch of stuff.

The downside with all this is of course that we add 4 bytes to every textnode. However, when I recently implemented bug 329974 which reduced the amount of memory textnodes consume in a lot of cases it didn't show up in any tinderbox graphs. While this partially probably is due to us having bad tests, I think it's safe to say that another 4 bytes is not going to add a whole lot of bloat. And given bug 329974 we should still use a good deal less then any current release.

Of course, we could do the old stick-it-in-a-hash trick, but I'd be worried about performance for that. Especially given that some of these flags there to avoid hash-lookups.

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Sync to tip

This just merges to trunk after the checkin for bug 333942

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 218504 [details] [diff] [review]
Sync to tip


>   /**
>    * Get whether this content is C++-generated anonymous content
>    * @see nsIAnonymousContentCreator
>    * @return whether this content is anonymous
>    */
>-  virtual PRBool IsNativeAnonymous() const = 0;
>+  PRBool IsNativeAnonymous() const
>+  {
>+    return !!HasFlag(CONTENT_IS_ANONYMOUS);
>+  }

Shouldn't IsNativeAnonymous be virtual? nsXULElement has a special version
of it (not SetNativeAnonymous).

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I made SetNativeAnonymous be virtual instead. The result should be the same and we're most likly calling IsNativeAnonymous a lot more often than SetNativeAnonymous.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 218504 [details] [diff] [review]
Sync to tip

Got a better patch comming up in a sec

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch v2

So I decided to take things a little further and push the flags all the way to nsINode. This also ment that GetListenerManager and GetEventListenerManager lives on the same interface and so I decided to kill GetEventListenerManager since there were only two callers of it.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 221392 [details] [diff] [review]
Patch v2

>Index: content/base/public/nsIContent.h
>+  PRBool IsNativeAnonymous() const
>+  {
>+    return !!HasFlag(NODE_IS_ANONYMOUS);
>+  }

I'll let you get away with the double-negation. But this double-double negation is just too much ;-).

>Index: content/base/src/nsGenericElement.cpp
>+  // pop any enclosed ranges out
>+  // nsRange::OwnerGone(mContent); not used for now

This needs capitals and full sentences (or as MS Word would tell you: this is a sentence fragment).

Seems reasonable otherwise. r=mrbkap

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 221392 [details] [diff] [review]
Patch v2

Reshuffeling my review requests a bit

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 221392 [details] [diff] [review]
Patch v2

>Index: content/base/public/nsContentUtils.h

>-  static nsresult AddToRangeList(nsIContent *aContent, nsIDOMRange *aRange,
>+  static nsresult AddToRangeList(nsINode *aNode, nsIDOMRange *aRange,

s/aContent/aNode/ in the comments too?  Same for the other methods you're changing here.

>Index: content/base/public/nsIContent.h

>+    return !!HasFlag(NODE_IS_ANONYMOUS);

HasFlag() returns a boolean; no need for the !!.

>+   * This virtual and non-inlined due to nsXULElement::SetNativeAnonymous

"This is ..."

>Index: content/base/public/nsINode.h

>+ * This bit will be set if the content doesn't have nsDOMSlots

s/content/node/

Similar for the other comments here.  I'd like us to move whatever flags don't make sense for nsINode (NODE_MAY_HAVE_FRAME, NODE_IS_ANONYMOUS, maybe NODE_HAS_PROPERTIES if it's not relevant) into nsIContent instead, I think.

Alternately, document what flags apply to what IsNodeOfType()s?  And maybe add some relevant asserts in SetFlags()/UnsetFlags() to catch people setting bogus flags?

>   /**
>+   * Inform content of range ownership changes.  This allows content

s/content/node/?  Throughout these comments.

>+  nsSlots* FlagsAsSlots() const

Maybe just name this Slots()?

>+  void SetSlots(nsSlots* aSlots)
>+  {
>+    mFlagsOrSlots = NS_REINTERPRET_CAST(PtrBits, aSlots);

Should we assert that !HasSlots() when we enter that method?

>Index: content/base/src/nsDocument.cpp
>+  // We can't rely on the nsINode dtor doing this for us since
>+  // by the time it runs GetOwnerDoc will return null

Could we document that this is because we've called mNodeInfoManager->DropReference() by then or something?

>Index: content/base/src/nsGenericElement.cpp

>+nsINode::~nsINode()

Could this assert that !HasSlots(), since it doesn't know how to destroy them if they do exist?

>+nsGenericElement::nsDOMSlots::nsDOMSlots(PtrBits aFlags)
>+  : nsIContent::nsSlots(aFlags),

nsINode::nsSlots?  Or alternately nsGenericElement::nsSlots?

sr=bzbarsky with those nits.  This is great stuff!

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Fix review comments

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 221676 [details] [diff] [review]
Fix review comments

This will be landed when the trunk opens

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I didn't rename FlagsAsSlots() to Slots() since it felt like the former is more clear on that it will cast the member, rather than create slots if they're not there.

Comment 13

[Boris Zbarsky [:bzbarsky]]

So I actually have a question...

>+// Three bits are node type specific.

Why three?  Isn't it 32-6 = 26?  Or something?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> So I actually have a question...
> 
> >+// Three bits are node type specific.
> 
> Why three?  Isn't it 32-6 = 26?  Or something?
> 

Yeah, that looks like an old comment from xul to dom merge.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yeah, that comment is pretty outdated now that we don't have content-ids any more. I can remove it if you want

Comment 16

[Boris Zbarsky [:bzbarsky]]

Please do!

Comment 17

[Boris Zbarsky [:bzbarsky]]

As a followup, could we make SetMayHaveFrame work for generic DOM data nodes too (so make it non-virtual, etc)?  That would actually help in some perf testcases, I bet.

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Yes, I'm wure we could do some stuff to MayHaveFrame (such as push it up the classchain and inline it). However I wanted to do that in a separate bug as I don't really know how that code is used.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yeah, separate bug is good.  I can give you a brain-dump on how it's used if you want.  ;)

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

After a lot of platform specific bustages, compiler bugs, and heroic effort from jag, this is now in the tree.