If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Add storage for slots in nsGenericDOMDataNode

RESOLVED FIXED

Status

()

Core
DOM
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: sicking, Assigned: sicking)

Tracking

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
Created attachment 218490 [details] [diff] [review]
Patch to fix
Assignee: general → bugmail
Status: NEW → ASSIGNED
Attachment #218490 - Flags: superreview?(jst)
Attachment #218490 - Flags: review?(jst)
Created attachment 218504 [details] [diff] [review]
Sync to tip

This just merges to trunk after the checkin for bug 333942
Attachment #218490 - Attachment is obsolete: true
Attachment #218504 - Flags: superreview?(jst)
Attachment #218504 - Flags: review?(jst)
Attachment #218490 - Flags: superreview?(jst)
Attachment #218490 - Flags: review?(jst)

Updated

12 years ago
Blocks: 329122

Comment 3

12 years ago
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).
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 on attachment 218504 [details] [diff] [review]
Sync to tip

Got a better patch comming up in a sec
Attachment #218504 - Flags: superreview?(jst)
Attachment #218504 - Flags: review?(jst)
Created attachment 221392 [details] [diff] [review]
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.
Attachment #218504 - Attachment is obsolete: true
Attachment #221392 - Flags: superreview?(jst)
Attachment #221392 - Flags: review?(jst)
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
Attachment #221392 - Flags: review?(jst) → review+

Updated

12 years ago
Blocks: 329112
Comment on attachment 221392 [details] [diff] [review]
Patch v2

Reshuffeling my review requests a bit
Attachment #221392 - Flags: superreview?(jst) → superreview?(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!
Attachment #221392 - Flags: superreview?(bzbarsky) → superreview+
Created attachment 221676 [details] [diff] [review]
Fix review comments
Attachment #221392 - Attachment is obsolete: true
Attachment #221676 - Flags: superreview+
Attachment #221676 - Flags: review+
Comment on attachment 221676 [details] [diff] [review]
Fix review comments

This will be landed when the trunk opens
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.
So I actually have a question...

>+// Three bits are node type specific.

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

Comment 14

12 years ago
(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.
Yeah, that comment is pretty outdated now that we don't have content-ids any more. I can remove it if you want
Please do!
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.
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.
Yeah, separate bug is good.  I can give you a brain-dump on how it's used if you want.  ;)
After a lot of platform specific bustages, compiler bugs, and heroic effort from jag, this is now in the tree.
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Depends on: 338595
You need to log in before you can comment on or make changes to this bug.