156364 - Add storage for flags in nsGenericElement

Status: RESOLVED FIXED

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

Description

[Johnny Stenback (:jst)]

nsGenericElement needs flags so that we can start optimizing what's stored in
the elements and what's stored elsewhere, and we'll also need a flag that says
if the element is anonymous or not (mContentID is used for that now, but that's
going away). This would also let the editor people fix bug 155475, and who knows
what we'll need flags for in the future...

Comment 1

[Matthias Versen [:Matti]]

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

Comment 2

[Johnny Stenback (:jst)]

Attachment: Proposed fix.

Add mFlagsOrSlots to nsGenericElement, this member is shared for storing either
flags or the pointer to the nsDOMSlots, if we have slots, the flags are stored
in the slots. Also sqeeze mContentID into mFlagsOrSlots (if it fits, if it
doesn't, allocate slots and store the content id in the slots). Push
mRangeLists and mListenerManager from the slots into hash tables (and share the
hash tables with nsGenericDOMDataNode) and use a flag to say if there's a event
listener manager or a range list in the hash for an element.

This patch saves us 4 bytes per element, woohoo! :-)

Comment 3

[Johnny Stenback (:jst)]

A diff -w doesn't really make reviewing this any easier, so I didn't see a point
in attaching a diff -w...

Comment 4

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

+  if (slots->IsEmpty()) {
+    mFlagsOrSlots = slots->mFlags | GENERIC_ELEMENT_DOESNT_HAVE_DOMSLOTS;
+
+    delete slots;
   }

Don't you have to add slots->mContentID to mFlagsOrSlots?


 nsGenericElement::GetTagName(nsAString& aTagName)
 {
   aTagName.Truncate();
-  if (mNodeInfo) {
-    mNodeInfo->GetName(aTagName);
-  }
+
+  mNodeInfo->GetName(aTagName);
+
   return NS_OK;
 }
you can remove the aTagName.Truncate() too now.


in nsGenericElement::SetAttributeNode:
+  nsCOMPtr<nsIDOMNode> node(do_QueryInterface(aAttribute, &rv)), returnNode;

please simply use aAttribute instead of creating the |node| variable. Same in
nsGenericElement::SetAttributeNodeNS


That's it for tonight, still have some stuff left to look at

Comment 5

[Johnny Stenback (:jst)]

Thanks, made those changes in my local tree.

Comment 6

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

+typedef unsigned long PtrBits;

IIRC we should use PRWord for bitfields that are the same size of a void*


+  } else {
+    SetFlags(aID << GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET);
+  }

you need to unset the bits first, otherwise the new id and the old one will be
or'ed together.


 nsGenericElement::RangeAdd(nsIDOMRange* aRange)
 {
+  PtrBits flags = GetFlags();
flags doesn't seem to be used

nit: IMHO it'd be nice to have a #define for
~PtrBits(0)) >> GENERIC_ELEMENT_CONTENT_ID_BITS_OFFSET

+  EventListenerManagerMapEntry(const void *aKey)
+    : mKey(aKey), mListenerManager(nsnull)
don't need to init mListenerManager since it's a nsCOMPtr

with that r=sicking

Comment 7

[Johnny Stenback (:jst)]

Attachment: Fix issues found by sicking

PRWord is not guaranteed to be the same size as a pointer either, so I decided
to stick with unsigned long (as other similar code does) and added an assertion
that sizeof(PtrBits) == sizeof(void *).

Comment 8

[Johnny Stenback (:jst)]

Comment on attachment 90796 [details] [diff] [review]
Fix issues found by sicking

Markign sicking's r=, and rpotts says sr=rpotts.

Comment 9

[Johnny Stenback (:jst)]

Attachment: Merged to the tip.

Comment 10

[Johnny Stenback (:jst)]

Fix checked in.