195350 - (attrs) Tracker for attributes rework

Status: RESOLVED FIXED

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

Description

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

This bug will work as a tracker for my attributes rework. I'll do this in the
following stages (approximatly)

Stage 1: New class that holds an array of attribute name+value. Use this array for
         nsXMLElement
Stage 2: Write new implementation of .attributes and attribute-nodes
Stage 3: Use new classes for nsXULElement
Stage 4: Use new classes for html-elements
Stage 5: extend attribute-array class to also hold pointers to child-nodes

Comment 1

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

I'll start working on this next week. The plan has changed a bit though to allow
element work like bug 198533 to happen sooner. So something like this is the plan:

1.
Create a class that holds children and attributes, this is jsts design (or
rather, as i recall his design):

Children are stored as a single pointer pointing to the childs nsIContent.

Attributes are stored as two pointers. The first pointer is to the name and will
either point to an nsIAtom or an nsINodeInfo, similarly to how html-elements
store names now. The second pointer is a void* pointing to the attributes value.

Make nsGenericElement contain this class and let it treat the void* pointers as
pointers to a string (what kind of string needs to be ironed out, but i tend to
lean towards nsCheapStringBufferUtils).
nsGenericHTMLElement will override attributehandling functions and treat the
value as a nsHTMLValue*.

Something needs to be done wrt mapped attributes, the classlist and the id-atom
for htmlelements. I'll probably put them in some sorts of slots struct in this
first step. (which is where they are now too)

2.
Clean up the nsHTMLValue class and 
  a) make it only 4 bytes long. This way we can put the nsHTMLValue itself in
     the array itself rather then putting a pointer in there.
  b) make it able to store atoms so that xul can store its atom-values in there.

3.
Make XUL use this attributes+children class. One big problem is attributes in
prototypes. See http://bugzilla.mozilla.org/show_bug.cgi?id=198533#c2

This would also include making xul lazy-create its attribute-nodes the same way
that html and xml does. It should be able to use the same classes for the
attribute-nodes as xml and html uses.

4.
Write new implementation of .attributes and attribute-nodes. Might reduce
priority of this.

5.
Revamp nsHTMLValue, need to check if all of these are possible:
* Get rid of enumerated values in favour for atoms.
* Get rid of different integer types (including pixel) and different
  stringtypes.
* Get rid of the nulltype and the emptytype.


Step 3 and 5 is the hardest. 3 is really bug 198533 so it should be done there.

Comment 2

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

Oh, i should say that step 1 and 2 are the ones i'm planning on doing right
away, can't really give any promices about 3-5 (hoping to get help with at least 3)

Comment 3

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

Attachment: Stage 1, v1

This does most of stage one. Unfortunatly not the trickiest part which is
converting html to store its attributes in the new class. I'll leave that for a
later patch.

There are a few problems with this patch. First off it needs testing on gcc
since i suspect that we're breaking aliasing rules and need to add some
flag-magic to guard against that.

Second I'm not really happy with the way I get attributes from the array. See
the comment at the top of nsAttrAndChildArray::SetAttr. I would like to use
some union magic here, but I just can't figure out how to do that. Suggestions
very welcome.

Third this patch is potentially slowing us down right now. The problem is that
(the "main" version of) nsGenericElement::SetAttr takes a nodeinfo. However
since we with this patch most of the time won't store nodeinfos as names we
will end up creating and destroying nsINodeInfo objects a lot. I'll fix that in
the next patch by making SetAttr just take a localname+namespace+prefix instead
(and let SetAttr grab the nodeinfo if needed). However i'd rather do this in a
separate patch to keep them both more readable.

Comment 4

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

Comment on attachment 137090 [details] [diff] [review]
Stage 1, v1

requsting reviews.

Peterv: if you want to look at this that'd be great. I didn't know how your
availability was.

Alex: It'd be great if you could look at the SVG changes. Shouldn't be any
functional changes, but just in case...

Comment 5

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

Comment on attachment 137090 [details] [diff] [review]
Stage 1, v1

There are two minor problems in this patch that i've fixed locally.

First off there's an error in nsSVGAttributes::SetAttr. I forgot to put back
some stuff that I had #if 0'ed out.

Second there was a copy-n-paste error in the licenceplate of the new files.

Consider both problems fixed. I won't attach a new patch since i expect there
will be other comments.

Comment 6

[Alex Fritze]

The SVG changes look good (apart from the #ifdef 0 stuff, of course) and should
map to the new SVG branch code pretty easily. Thanks for the nsVoidArray ->
nsCOMArray cleanup in nsSVGElement.
At some stage we're going to get rid of nsSVGAttributes altogether and use
nsGenericElement's mAttrsAndChildren, right? I'm happy to do some work on that
if you want. Should be straightforward by extending nsAttrValue::Type to
nsSVGAttributes.

Comment 7

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

Attachment: Fix SetAttr signature

This patch solves the third issue in comment 3 by chaning the signature of
SetAttr to never take a nodeinfo. This should be applied on top of a tree with
the previous patch but i'm attaching it as a separate patch to ease review

Comment 8

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

Attachment: Fix SetAttr signature -w

same as attachment 137529 [details] [diff] [review] but with -w. Should be easier to review in a few
spots

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 137090 [details] [diff] [review]
Stage 1, v1

- In content/base/src/nsAttrAndChildArray.cpp:

+nsAttrValue::nsAttrValue(const nsAString& aValue)
+    : mBits(0)
+{
+  SetTo(aValue);
+}
+
+nsAttrValue::nsAttrValue(nsHTMLValue* aValue)
+    : mBits(0)
+{
...

I think I'd rather see this code in its own file, i.e. nsAttrValue.{h,cpp}
rather than dumping it in with the array code.

- In nsAttrValue::SetTo(const nsAString& aValue):

+  Reset();
+  PRUnichar* str = nsnull;
+  if (!aValue.IsEmpty()) {
+    nsCheapStringBufferUtils::CopyToBuffer(str, aValue);
+  }
+  mBits = NS_REINTERPRET_CAST(PtrBits, str) | eString;

Ideally this would re-alloc in stead of potentially freeing and mallocing
again...

>+nsAttrAndChildArray::nsAttrAndChildArray()
>+{
>+  mImpl = nsnull;
>+}

How about using a member initializer instead of an explicit null assignment
here...

- In nsAttrAndChildArray::GetSafeChildAt():

+  return aPos < ChildCount() ? ChildAt(aPos) : nsnull;

This would be easier to work with when debugging if it was an if-statement in
stead...

- In nsAttrAndChildArray::InsertChildAt():

+  PRUint32 offset = AttrSlotCount() * 2;

Please put all these '2's in a #define, or a static const so that we don't need
to go through the code and look for all the '2's if this ever changes. And I
see the above code in lots of place, maybe an inline AttrSlotsSize() could hide
the above?

+    memmove(pos + 1, pos, (childCount - aPos) * sizeof(nsIContent*));

I'm assuming that the common case in this code will be to insert at the end of
the array, so the above call (and most of the other calls) to memmove() is a
noop in that case. Would it be worth checking for that and not calling
memmove() in that case, or is memmove() nicely inlined so it doesn't matter?

- In nsAttrAndChildArray::SetAttr():

+  // XXX this is dangerous, we're not really dereferencing mImpl here, but it
looks like it
+  // And it could be null
+  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);

Um, what? How is that not dereferencing mImpl here? Same thing in the
nsINodeInfo case.

- In nsAttrAndChildArray:

+nsAttrAndChildArray::InitFrom(const nsAttrAndChildArray& aOther, PRBool
aWillCopyChildren)

How about aDoCopyChildren or aShouldCopyChildren in stead?

- In nsAttrAndChildArray.h (nsAttrName):

+  PRBool Equals(nsIAtom* aLocalName, PRInt32 aNamespaceID) const
+  {
+    return aNamespaceID == kNameSpaceID_None ?
+	      NS_REINTERPRET_CAST(PtrBits, aLocalName) == mBits :
+	      !IsAtom() && NodeInfo()->Equals(aLocalName, aNamespaceID);
+  }

Ok, that really needs to be an if-statement to be even a little bit readable.

+  void AddRef()
+  {
+    // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first
+    // interface we can safly assume that it's first in the vtable
+    nsISupports* name = NS_REINTERPRET_CAST(nsISupports *,
+      mBits & ~NS_ATTRNAME_NODEINFO_BIT);
+
+    // XXX Alias
+    NS_ADDREF(name);
+  }
+
+  void Release()
+  {
+    // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first
+    // interface we can safly assume that it's first in the vtable
+    nsISupports* name = NS_REINTERPRET_CAST(nsISupports *,
+      mBits & ~NS_ATTRNAME_NODEINFO_BIT);
+
+    NS_RELEASE(name);
+  }

This is kinda confusing, generally AddRef() and Release() addref or release the
class on which they're implemented, not just something internal to that class.
How about AddRefInternalName() and ReleaseInternalName() in stead?

- In nsAttrAndChildArray:

+  // XXX need better name
+  nsresult InitFrom(const nsAttrAndChildArray& aOther, PRBool
aWillCopyChildren);

How about SetTo()?

+  struct Impl {
+    PRUint32 mAttrAndChildCount;
+    PRUint32 mBufferSize;
+    void* mBuffer[1];
+  };
+
+  Impl* mImpl;

This should eventually be done so that we have a small internal buffer, ala
nsAutoVoidArray, etc, but we'll worry about that later.

If you want to you could run some statistics and check how common elements with
no attributes and no children are. IIRC, they're *really* uncommon, so it might
be worth making mImpl a direct member in stead of mallocing it all the time.
But I don't really care for now...

- In nsDOMAttributeMap::GetNamedItemNS():

+  PRUint32 i, count = mContent->GetChildCount();
+  for (i = 0; i < count; ++i) {
...
+    mContent->GetAttrNameAt(i, &attrNS, getter_AddRefs(nameAtom),
+			     getter_AddRefs(prefix));

Um, don't you mean GetAttrCount() there before the loop? :-)

- In nsDocumentFragment::DropChildReferences():

+  PRUint32 count = mAttrsAndChildren.ChildCount();
+  while (count < 0) {

Um, count > 0, no? :-)

- In content/base/src/nsGenericElement.h:

+  /**
+   * Internal hook for converting a attribute name-string to an atomized name

"...converting a_n_ attribute..."

That's all for now, got through up to nsGenericHTMLElement.cpp

Comment 10

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

> +  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);
> Um, what? How is that not dereferencing mImpl here? Same thing in the
> nsINodeInfo case.

since mBuffer is an 'inlined' array the pointer to it, i.e. the value of
mBuffer, isn't stored in struct. Only the values of the actual elements are in
the struct. So mImpl->mBuffer is the same as

&(mImpl->mBuffer[0])

or

mImpl + offsetof(Impl, mBuffer[0]).

So there is no need to dereference mImpl.

I definitly agree it's not pretty, in fact it's the part i'm most unhappy about
with this code. Suggestions how to fix this is welcome but i'd rather not add a
'useless' |if|. We could do some sort of macro like:

#define ATTRS(_impl) \
    NS_REINTERPRET_CAST(InternalAttr*, _impl->mBuffer)

and then do

  PRUint32 i, slotCount = AttrSlotCount();
  for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {
    if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
      ATTRS(mImpl)[i].mValue.SetTo(aValue);

that gets around the problem entierly since we're not doing anything without
knowing that mImpl is set.

Comment 11

[Christopher Aillon (sabbatical, not receiving bugmail)]

(from update of attachment 137090 [details] [diff] [review])
>+ * The Original Code is TransforMiiX XSLT processor.

Please remember to fix this (in several places).  You said you did, but I still
don't see it fixed in your second patch.

>+ *
>+ * The Initial Developer of the Original Code is
>+ * IBM Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2002

2003 (or 4)?

>+ * IBM Corporation. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   IBM Corporation

You may wish to add yourself here :-)

>+void
>+nsAttrValue::Reset()
>+{
>+  void* ptr = NS_REINTERPRET_CAST(void*, mBits & NS_ATTRVALUE_VALUE_MASK);

Why do you need the intermediate cast to void*?  Can't you just do |PtrBits =
mBits & NS_ATTRVALUE_VALUE_MASK;| here and then reinterpret cast that later on?
 It might even be nice to replace GetType() with something like since you always
care about the type when accessing the ptr value, and vice versa.

  inline void GetPtrValueAndType(PtrBits& ptr, Type& type)
  {
    ptr = mBits & NS_ATTRVALUE_VALUE_MASK;
    type = NS_STATIC_CAST(Type, mBits & NS_ATTRVALUE_TYPE_MASK)
  }

...

>+void
>+nsAttrValue::SetTo(const nsAString& aValue)
>+{
>+  Reset();
>+  PRUnichar* str = nsnull;
>+  if (!aValue.IsEmpty()) {
>+    nsCheapStringBufferUtils::CopyToBuffer(str, aValue);
>+  }
>+  mBits = NS_REINTERPRET_CAST(PtrBits, str) | eString;

Yuck.  How about a SetPtrValueAndType() method complementing the other one I
suggested to make this code (and the similar code in a few other places) clearer
and cleaner?

...

>+
>+nsAttrAndChildArray::nsAttrAndChildArray()
>+{
>+  mImpl = nsnull;

This is c++.  We use initialization lists :-)

>+}
>+
>+nsAttrAndChildArray::~nsAttrAndChildArray()
>+{
>+  if (!mImpl) {
>+    return;
>+  }
>+
>+  Clear();
>+
>+  if (mImpl) {
>+    PR_Free(mImpl);
>+  }

No need for the if statement since you already know that we have an mImpl. 
(early return).  Also note that if we did not know that, there is a PR_FREEIF
macro :-)

...

>+void
>+nsAttrAndChildArray::RemoveChildAt(PRUint32 aPos)
>+{
>+  NS_ASSERTION(aPos < ChildCount(), "out-of-bounds");
>+
>+  PRUint32 childCount = ChildCount();
>+  void** pos = mImpl->mBuffer + AttrSlotCount() * 2 + aPos;

All this AttrSlotCount * 2 stuff probably should get turned into an inline
method or a macro or something....

>+  nsIContent* child = NS_STATIC_CAST(nsIContent*, *pos);
>+  NS_RELEASE(child);
>+  memmove(pos, pos + 1, (childCount - aPos - 1) * sizeof(nsIContent*));
>+  SetChildCount(childCount - 1);
>+}
>+

...

>+PRInt32
>+nsAttrAndChildArray::IndexOfChild(nsIContent* aPossibleChild) const
>+{
>+  if (!mImpl) {
>+    return -1;
>+  }
>+  void** children = mImpl->mBuffer + AttrSlotCount() * 2;
>+  PRUint32 i, count = ChildCount();
>+  for (i = 0; i < count; ++i) {
>+    if (children[i] == aPossibleChild) {
>+      return (PRInt32)i;

NS_STATIC_CAST(PRInt32, i)

>+    }
>+  }
>+
>+  return -1;
>+}
>+
>+PRUint32
>+nsAttrAndChildArray::AttrCount() const
>+{
>+  if (!mImpl) {
>+    return 0;
>+  }
>+
>+  PRUint32 count = AttrSlotCount();
>+  while (count > 0 && !mImpl->mBuffer[(count - 1) * 2]) {
>+    --count;
>+  }

Empty for-loops are sexy :-)

...

>+nsresult
>+nsAttrAndChildArray::SetAttr(nsIAtom* aLocalName, nsHTMLValue* aValue)
>+{
>+  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);
>+  PRUint32 i, slotCount = AttrSlotCount();
>+  for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {
>+    if (attrs[i].mName.Equals(aLocalName)) {
>+      attrs[i].mValue.SetTo(aValue);
>+
>+      return NS_OK;
>+    }
>+  }
>+
>+  NS_ENSURE_TRUE(slotCount < ATTRCHILD_ARRAY_MAX_ATTR_COUNT,
>+                 NS_ERROR_FAILURE);
>+
>+  if (i == slotCount && !AddAttrSlot()) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }
>+
>+  attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer); //
mImpl->mBuffer could have changed

Nit: boost the comment up above the code so that its easier to find on smaller
windows.

>+
>+  new (&attrs[i].mName) nsAttrName(aLocalName);
>+  new (&attrs[i].mValue) nsAttrValue(aValue);
>+
>+  return NS_OK;
>+}
>+
>+nsresult
>+nsAttrAndChildArray::SetAttr(nsINodeInfo* aName, const nsAString& aValue)
>+{
...
>+  attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer); //
mImpl->mBuffer could have changed

Again, move the comment above the code.

>+
>+  new (&attrs[i].mName) nsAttrName(aName);
>+  new (&attrs[i].mValue) nsAttrValue(aValue);
>+
>+  return NS_OK;
>+}
>+
>+

...

>+const nsAttrName*
>+nsAttrAndChildArray::GetExistingAttrNameFromQName(const nsAString& aName) const
>+{
>+  NS_ConvertUTF16toUTF8 name(aName);
>+  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);
>+
>+  PRUint32 i, slotCount = AttrSlotCount();
>+  for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {
>+    if (attrs[i].mName.IsAtom()) {
>+      if (attrs[i].mName.Atom()->EqualsUTF8(name)) {
>+        return &attrs[i].mName;
>+      }
>+    }
>+    else {
>+      if (attrs[i].mName.NodeInfo()->QualifiedNameEquals(name)) {
>+        return &attrs[i].mName;
>+      }
>+    }


  if (attrs[i].mName.IsAtom() &&
      attrs[i].mName.Atom()->EqualsUTF8(name) ||
      attrs[i].mName.NodeInfo()->QualifiedNameEquals(name)) {
    return &attrs[i].mName;
  }


Actually, it would be nice if nsAttrName provided the comparision functionality
itself so the caller didn't even have to bother with this stuff.



>+PRInt32
>+nsAttrAndChildArray::IndexOfAttr(nsIAtom* aLocalName, PRInt32 aNamespaceID) const
>+{
>+  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);
>+  PRUint32 i, slotCount = AttrSlotCount();
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    // This should be the common case so lets make an optimized loop


Shouldn't the optimizer be able to do that in this case, since it knows that the
namespace won't change in between loop iterations?  I think you should be able
to write one for loop with the namespace check contained within and still be
able to generate the right code.


>+    for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {
>+      if (attrs[i].mName.Equals(aLocalName)) {
>+        return (PRInt32)i;

NS_STATIC_CAST(PRInt32, i)

>+      }
>+    }
>+  }
>+  else {
>+    for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {
>+      if (attrs[i].mName.Equals(aLocalName, aNamespaceID)) {
>+        return (PRInt32)i;

NS_STATIC_CAST(PRInt32, i)

>+      }
>+    }
>+  }
>+
>+  return -1;
>+}
>+
>+nsresult
>+nsAttrAndChildArray::InitFrom(const nsAttrAndChildArray& aOther, PRBool
aWillCopyChildren)
>+{
>+  Clear();
>+
>+  PRUint32 otherAttrCount = aOther.AttrCount();
>+  PRUint32 size = aWillCopyChildren ? otherAttrCount * 2 + aOther.ChildCount() :
>+                                      otherAttrCount * 2;
>+
>+  if (!size) {
>+    // Other array doesn't contain attributes (or children that we're
interested in)
>+    return NS_OK;
>+  }
>+
>+  PRUint32 allocSize = (size + NS_IMPL_EXTRA_SIZE) * sizeof(void*);
>+  
>+  Impl* newImpl = NS_STATIC_CAST(Impl*,
>+      mImpl ? PR_Realloc(mImpl, allocSize) : PR_Malloc(allocSize));
>+  NS_ENSURE_TRUE(newImpl, NS_ERROR_OUT_OF_MEMORY);
>+
>+  mImpl = newImpl;
>+  mImpl->mBufferSize = size;
>+  SetAttrSlotAndChildCount(0, 0);
>+
>+  InternalAttr* attrs = NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer);
>+  InternalAttr* otherAttrs = NS_REINTERPRET_CAST(InternalAttr*,
aOther.mImpl->mBuffer);
>+  PRUint32 i;

You can safely declare this within the for loops initialization statement

>+  for (i = 0; i < otherAttrCount; ++i) {
>+    new (&attrs[i]) InternalAttr(otherAttrs[i]);
>+  }
>+  
>+  return NS_OK;
>+}
>+

>Index: content/base/src/nsAttrAndChildArray.h

>+class nsAttrName
>+{

...

>+  // Faster comparison in the case we know the namespace is null
>+  PRBool Equals(nsIAtom* aAtom) const
>+  {
>+    return aAtom == Atom();
>+  }
>+
>+  PRBool Equals(nsIAtom* aLocalName, PRInt32 aNamespaceID) const
>+  {
>+    return aNamespaceID == kNameSpaceID_None ?
>+             NS_REINTERPRET_CAST(PtrBits, aLocalName) == mBits :

this probably should call Equals(nsIAtom*)

>+             !IsAtom() && NodeInfo()->Equals(aLocalName, aNamespaceID);
>+  }

...

>+  void AddRef()
>+  {
>+    // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first

"inherit", "its"

>+    // interface we can safly assume that it's first in the vtable

"safely", and just a note so you don't change this one too, but on this line
"it's" is in fact correct.

>+    nsISupports* name = NS_REINTERPRET_CAST(nsISupports *,
>+      mBits & ~NS_ATTRNAME_NODEINFO_BIT);
>+
>+    // XXX Alias
>+    NS_ADDREF(name);
>+  }
>+
>+  void Release()
>+  {
>+    // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first
>+    // interface we can safly assume that it's first in the vtable

Same comment nits as above.


>+#define NS_ATTRVALUE_TYPE_MASK (PtrBits(3))
>+#define NS_ATTRVALUE_VALUE_MASK (~NS_ATTRVALUE_TYPE_MASK)
>+#define NS_ATTRVALUE_TYPE_STRING 0
>+#define NS_ATTRVALUE_TYPE_HTMLVALUE 1

Hmm, if you're going to use bitmasks, then 0 really should mean "uninitialized"
and its a bad value IMO for a string.  Plus, using decimal constants for
defining bitfields kind of sucks ;-)

#define NS_ATTRVALUE_TYPE_STRING 0x01
#define NS_ATTRVALUE_TYPE_HTMLVALUE 0x10
#define NS_ATTRVALUE_TYPE_MASK (NS_ATTRVALUE_TYPE_STRING |
NS_ATTRVALUE_TYPE_HTMLVALUE)
#define NS_ATTRVALUE_VALUE_MASK (~NS_ATTRVALUE_TYPE_MASK)

That looks much nicer :-)

>+
>+
>+class nsAttrValue {
>+public:
>+  nsAttrValue(const nsAString& aValue);
>+  nsAttrValue(nsHTMLValue* aValue);
>+  ~nsAttrValue();
>+
>+  void Reset();
>+  void SetTo(const nsAString& aValue);
>+  void SetTo(nsHTMLValue* aValue);
>+
>+  void ToString(nsAString& aResult) const;
>+
>+  enum Type {
>+    eString = 0,
>+    eHTMLValue = 1  // This should eventually die

This looks like an XXX comment to me.  But I'm not so sure about it.  You'd run
into similar problems when using a union I imagine since you would still need to
know the type when performing some operations on the pointer value.

>+  };


>+class nsAttrAndChildArray
>+{

>+  // XXX need better name
>+  nsresult InitFrom(const nsAttrAndChildArray& aOther, PRBool aWillCopyChildren);

I'd suggest just making this a copy constructor, since that is pretty much what
it is.  The only thing you'd have to worry about is an out parameter for the
nsresult but since you really only care about one potential failure, I'd make it
just have a |PRBool ok| as an outparameter.

>+
>+  Impl* mImpl;

Hmm.  Do you really think its worth it for mImpl to be an Impl* rather than just
an Impl?  Won't most things have attributes and/or children and thus need an
Impl?  If that's the case (I suspect it is, especially because we make it even
harder for people to have childless nodes because of bug 26179) then you'd save
4 bytes per object plus some alloc fu and some more code size wins since you
wouldn't need to do null checks if you changed it to a class member rather than
a pointer.


>Index: content/base/src/nsDOMAttributeMap.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsDOMAttributeMap.cpp,v
>retrieving revision 1.40
>diff -u -6 -p -r1.40 nsDOMAttributeMap.cpp
>--- content/base/src/nsDOMAttributeMap.cpp	15 Nov 2003 21:29:07 -0000	1.40
>+++ content/base/src/nsDOMAttributeMap.cpp	9 Dec 2003 03:38:08 -0000
>@@ -274,51 +274,53 @@ nsDOMAttributeMap::GetNamedItemNS(const 
>                                   const nsAString& aLocalName,
>                                   nsIDOMNode** aReturn)
> {
>   NS_ENSURE_ARG_POINTER(aReturn);
>   *aReturn = nsnull;
> 
>-  nsresult rv = NS_OK;
>-  if (mContent) {
>-    nsCOMPtr<nsIAtom> nameAtom = do_GetAtom(aLocalName);
>-    PRInt32 nameSpaceID = kNameSpaceID_None;
>-    nsCOMPtr<nsIAtom> prefix;
>-
>-    nsINodeInfo *contentNi = mContent->GetNodeInfo();
>-    NS_ENSURE_TRUE(contentNi, NS_ERROR_FAILURE);
>-
>-    if (aNamespaceURI.Length()) {
>-      nsContentUtils::GetNSManagerWeakRef()->GetNameSpaceID(aNamespaceURI,
>-                                                            &nameSpaceID);
>+  if (!mContent) {
>+    return NS_OK;
>+  }
> 
>-      if (nameSpaceID == kNameSpaceID_Unknown)
>-        return NS_OK;
>-    }
>+  NS_ConvertUTF16toUTF8 utf8Name(aLocalName);
>+  PRInt32 nameSpaceID = kNameSpaceID_None;
> 
>-    nsresult attrResult;
>-    nsAutoString value;
>+  if (!aNamespaceURI.IsEmpty()) {
>+    nsContentUtils::GetNSManagerWeakRef()->GetNameSpaceID(aNamespaceURI,
>+                                                          &nameSpaceID);
> 
>-    attrResult = mContent->GetAttr(nameSpaceID, nameAtom,
>-                                   getter_AddRefs(prefix), value);
>+    if (nameSpaceID == kNameSpaceID_Unknown)
>+      return NS_OK;

Nit: add braces. :-)


>Index: content/base/src/nsDocumentFragment.cpp
> NS_IMETHODIMP
> nsDocumentFragment::DropChildReferences()
> {
>-  PRUint32 count = GetChildCount();
>-
>-  for (PRUint32 index = 0; index < count; ++index) {
>-    nsIContent* kid = NS_STATIC_CAST(nsIContent*, mChildren.ElementAt(index));
>-    NS_RELEASE(kid);
>+  PRUint32 count = mAttrsAndChildren.ChildCount();
>+  while (count < 0) {

Dude, check your compiler warnings.  I'm sure gcc flags this as a check that is
always false. (unsigned < 0)


>Index: content/base/src/nsGenericElement.cpp
>@@ -3605,215 +3533,156 @@ nsGenericContainerElement::SetAttr(nsINo
> }
> 
> nsresult
> nsGenericContainerElement::GetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>                                    nsAString& aResult) const
> {
...

>-  if (rv == NS_CONTENT_ATTR_NOT_THERE) {
>-    // In other cases we already set the out param.
>+  const nsAttrValue* val = mAttrsAndChildren.GetAttr(aName, aNameSpaceID);
>+  if (!val) {
>     // Since we are returning a success code we'd better do

What success code?  :-)

>     // something about the out parameters (someone may have
>     // given us a non-empty string).
>     aResult.Truncate();
>+    
>+    return NS_CONTENT_ATTR_NOT_THERE;
>   }


> #ifdef DEBUG
> void
> nsGenericContainerElement::ListAttributes(FILE* out) const
> {
>-  PRUint32 index, count = GetAttrCount();
>+  PRUint32 index, count = mAttrsAndChildren.AttrCount();
> 
>   for (index = 0; index < count; index++) {
>-    const nsGenericAttribute* attr = (const
nsGenericAttribute*)mAttributes->ElementAt(index);
>+    const nsAttrName* name = mAttrsAndChildren.GetSafeAttrNameAt(index);
>+
>     nsAutoString buffer;
> 
>     // name
>-    nsAutoString name;
>-    attr->mNodeInfo->GetQualifiedName(name);
>-    buffer.Append(name);
>+    if (name->IsAtom()) {
>+      name->Atom()->ToString(buffer);
>+    }
>+    else {
>+      name->NodeInfo()->GetQualifiedName(buffer);
>+    }


It sucks that callers need to do this now.  Please add a QualifiedName() method
to nsAttrName which provides this functionality.


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

>+  /**
>+   * Internal hook for converting a attribute name-string to an atomized name
>+   */
>+  virtual const nsAttrName* InternalGetExistingAttrNameFromQName(const
nsAString& aStr) = 0;

Whew!  What a name!  Can we do something with it, to shorten it? 
QNameToAttrName perhaps?  (Note that I think the Internal prefix thing is kind
of silly since a name doesn't prevent external users from calling it -- if it
needs to be internal there are ways to make it so)




>Index: content/html/style/src/nsHTMLAttributes.h

>     void Addref()
>     {
>         // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first
>         // interface we can safly assume that it's first in the vtable
>         nsISupports* name = NS_REINTERPRET_CAST(nsISupports *,
>-            mBits & ~NS_ATTRNAME_NODEINFO_BIT);
>+            mBits & ~NS_HTMLATTRNAME_NODEINFO_BIT);
> 
>         NS_IF_ADDREF(name);
>     }
>     
>     void Release()
>     {
>         // Since both nsINodeInfo and nsIAtom inherits nsISupports as it's first
>         // interface we can safly assume that it's first in the vtable

Aha!  Here are the real culprits!  Please also fix these comments as outlined in
my earlier nit.


>Index: content/svg/content/src/nsSVGAttributes.cpp


> NS_IMETHODIMP
> nsSVGAttribute::SetPrefix(const nsAString& aPrefix)
> {
>   // XXX: Validate the prefix string!
>+
>+  if (mName.IsAtom()) {

Please just comment that we are throwing here because the namespace is null. 
(it's probably not obvious to the casual reader).  Maybe similar comments in the
methods above, though that is somewhat more obvious.

>+    return NS_ERROR_DOM_NAMESPACE_ERR;
>+  }
>   
>   nsCOMPtr<nsINodeInfo> newNodeInfo;
>   nsCOMPtr<nsIAtom> prefix;
>   
>   if (!aPrefix.IsEmpty()) {
>     prefix = do_GetAtom(aPrefix);
>     NS_ENSURE_TRUE(prefix, NS_ERROR_OUT_OF_MEMORY);
>   }
>   
>-  nsresult rv = mNodeInfo->PrefixChanged(prefix,
>-                                         getter_AddRefs(newNodeInfo));
>+  nsresult rv = mName.NodeInfo()->PrefixChanged(prefix,
>+                                                getter_AddRefs(newNodeInfo));
>   NS_ENSURE_SUCCESS(rv, rv);
>   
>-  mNodeInfo = newNodeInfo;
>+  mName.SetTo(newNodeInfo);
>   
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsSVGAttribute::GetLocalName(nsAString& aLocalName)
> {
>-  mNodeInfo->GetLocalName(aLocalName);
>+  mName.LocalName()->ToString(aLocalName);
>   return NS_OK;
> }
> 
> NS_IMETHODIMP
> nsSVGAttribute::InsertBefore(nsIDOMNode* aNewChild, nsIDOMNode* aRefChild,
nsIDOMNode** aReturn)
> {
>@@ -332,13 +350,18 @@ nsSVGAttribute::GetValue(nsAString& aVal
> }
> 
> NS_IMETHODIMP
> nsSVGAttribute::SetValue(const nsAString& aValue)
> {
>   if (mOwner) {
>-    return mOwner->SetAttr(mNodeInfo, aValue, PR_TRUE);
>+    if (mName.IsAtom()) {
>+      return mOwner->SetAttr(kNameSpaceID_None, mName.Atom(), aValue, PR_TRUE);
>+    }
>+    else {
>+      return mOwner->SetAttr(mName.NodeInfo(), aValue, PR_TRUE);
>+    }
>   }
> 
>   return GetValue()->SetValueString(aValue);
> }
> 
> NS_IMETHODIMP
>@@ -390,13 +413,18 @@ nsSVGAttribute::DidModifySVGObservable (
> //----------------------------------------------------------------------
> // Implementation functions
> 
> void
> nsSVGAttribute::GetQualifiedName(nsAString& aQualifiedName)const
> {
>-  mNodeInfo->GetQualifiedName(aQualifiedName);
>+  if (mName.IsAtom()) {
>+    mName.Atom()->ToString(aQualifiedName);
>+  }
>+  else {
>+    mName.NodeInfo()->GetQualifiedName(aQualifiedName);
>+  }

Again, this really begs for a QualifiedName() method on nsAttrName



> NS_IMETHODIMP
>+nsSVGAttributes::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
>+                         const nsAString& aValue,
>+                         PRBool aNotify)
>+{
>+#if 0
>+  NS_ENSURE_TRUE(mContent, NS_ERROR_FAILED);
>+  
>+  nsCOMPtr<nsINodeInfo> ni;
>+  nsresult rv = mContent->GetNodeInfo()->NodeInfoManager()->GetNodeInfo(
>+      aName, nsnull, aNameSpaceID, getter_AddRefs(ni));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  return SetAttr(ni, aValue, aNotify);
>+#endif
>+  return NS_OK;
>+}
>+
>+NS_IMETHODIMP
> nsSVGAttributes::SetAttr(nsINodeInfo* aNodeInfo,
>                          const nsAString& aValue,
>                          PRBool aNotify)
> {
>   NS_ENSURE_ARG_POINTER(aNodeInfo);
>   nsAutoString oldValue;
>@@ -591,15 +632,17 @@ nsSVGAttributes::SetAttr(nsINodeInfo* aN
>   PRInt32 index;
>   PRInt32 count;
> 
>   count = Count();
>   for (index = 0; index < count; index++) {
>     attr = ElementAt(index);
>-    if (attr->GetNodeInfo() == aNodeInfo) {
>+    if (attr->Name()->Equals(aNodeInfo->NameAtom(),
>+                             aNodeInfo->NamespaceID())) {
>       attr->GetValue()->GetValueString(oldValue);
>-      if (oldValue.Equals(aValue)) {
>+      if (oldValue.Equals(aValue) &&
>+          attr->Name()->GetPrefix() == aNodeInfo->GetPrefixAtom()) {
>         // Do nothing if the value is not changing
>         return NS_OK;
>       }
>       break;
>     }
>   }
>@@ -693,15 +736,13 @@ nsSVGAttributes::UnsetAttr(PRInt32 aName
>   PRInt32 count = Count();
>   PRInt32 index;
>   PRBool  found = PR_FALSE;
>   nsSVGAttribute* attr = nsnull;
>   for (index = 0; index < count; index++) {
>     attr = ElementAt(index);
>-    if ((aNameSpaceID == kNameSpaceID_Unknown ||
>-         attr->GetNodeInfo()->NamespaceEquals(aNameSpaceID)) &&
>-        attr->GetNodeInfo()->Equals(aName) &&
>+    if (attr->Name()->Equals(aName, aNameSpaceID) &&
>         !attr->IsRequired() &&
>         !attr->IsFixed()) {
>       found = PR_TRUE;
>       break;
>     }
>   }
>@@ -761,33 +802,35 @@ nsSVGAttributes::HasAttr(PRInt32 aNameSp
> {
>   // XXX - this should be hashed, or something
>   PRInt32 count = Count();
>   PRInt32 index;
>   for (index = 0; index < count; index++) {
>     nsSVGAttribute *attr = ElementAt(index);
>-    if ((aNameSpaceID == kNameSpaceID_Unknown ||
>-         attr->GetNodeInfo()->NamespaceEquals(aNameSpaceID)) &&
>-        (attr->GetNodeInfo()->Equals(aName))) {
>+    if (attr->Name()->Equals(aName, aNameSpaceID)) {
>       return PR_TRUE;
>     }
>   }
>   return PR_FALSE;
> }
> 
>-NS_IMETHODIMP_(already_AddRefed<nsINodeInfo>)
>+const nsAttrName*
> nsSVGAttributes::GetExistingAttrNameFromQName(const nsAString& aStr)
> {
>   PRInt32 indx, count = Count();
>   NS_ConvertUCS2toUTF8 utf8String(aStr);
>   for (indx = 0; indx < count; indx++) {
>-    nsSVGAttribute* attr = ElementAt(indx);
>-    nsINodeInfo* ni = attr->GetNodeInfo();
>-    if (ni->QualifiedNameEquals(utf8String)) {
>-      NS_ADDREF(ni);
>-
>-      return ni;
>+    const nsAttrName* name = ElementAt(indx)->Name();
>+    if (name->IsAtom()) {
>+      if (name->Atom()->EqualsUTF8(utf8String)) {
>+        return name;
>+      }
>+    }
>+    else {
>+      if (name->NodeInfo()->QualifiedNameEquals(utf8String)) {
>+        return name;
>+      }
>     }

  if (name->isAtom() && name->Atom()->EqualsUTF8(utf8String) ||
      name->NodeInfo()->QualifiedNameEquals(utf8String)) {
    return name;
  }
  

Actually, a method returning a bool on nsAttrName would again be nice to do
this... The caller shouldn't need to really care about how nsAttrName stores
things internally for certain conditions.  It should just ask it something and
then nsAttrName can deal with where to get the data from to do the things it
needs to do...


The rest looks cool.  Merry Christmas!  :-)

Comment 12

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

> You may wish to add yourself here :-)

Unfortunatly IBM doesn't like that.

> >+  void* ptr = NS_REINTERPRET_CAST(void*, mBits & NS_ATTRVALUE_VALUE_MASK);
> Why do you need the intermediate cast to void*?  Can't you just do |PtrBits =
> mBits & NS_ATTRVALUE_VALUE_MASK;|

I prefer to keep the number of reinterpret casts down to a minimum. They are a
very evil and risky cast so i'd rather do it just once and then do static casts
later on. Note that the casts will compile away.

> It might even be nice to replace GetType() with something like since you
> always care about the type when accessing the ptr value, and vice versa.

Actually that would result in more code at the call-site since i would have to
declare the variables separatly from getting their values, so i'd rather stick
with the current. I'll look into adding something like |void* GetValueAsPtr()|
though.

> >+  mBits = NS_REINTERPRET_CAST(PtrBits, str) | eString;
> Yuck.  How about a SetPtrValueAndType() method

Hmm.. personally i don't think this is very yucky, but I guess i can add that.

> Empty for-loops are sexy :-)

Ugh, no, that would make that code even harder to read.

>  if (attrs[i].mName.IsAtom() &&
>      attrs[i].mName.Atom()->EqualsUTF8(name) ||
>      attrs[i].mName.NodeInfo()->QualifiedNameEquals(name)) {
>    return &attrs[i].mName;
>  }
>Actually, it would be nice if nsAttrName provided the comparision functionality
>itself so the caller didn't even have to bother with this stuff.

That code won't work. The |attrs[i].mName.NodeInfo()->QualifiedNameEquals(name)|
test will be run if mName is an atom and the EqualsUTF8 test fails, which is
unneccesary work and will crash. I'll add a QualifiedNameEquals method to
nsAttrName though.

> Shouldn't the optimizer be able to do that in this case, since it knows that
> the namespace won't change in between loop iterations?

I'm very sceptical that all optimizers would be able to break that test out of
the loop. I'd like to do some serious testing before i relied on that, which of
course you're welcome to do ;-)

> >+  PRUint32 i;
> You can safely declare this within the for loops initialization statement

The XP guidelines says not to, even if the variables are not used after the loop
since someone might add code later that does.


>+#define NS_ATTRVALUE_TYPE_MASK (PtrBits(3))
>+#define NS_ATTRVALUE_VALUE_MASK (~NS_ATTRVALUE_TYPE_MASK)
>+#define NS_ATTRVALUE_TYPE_STRING 0
>+#define NS_ATTRVALUE_TYPE_HTMLVALUE 1
> Hmm, if you're going to use bitmasks, then 0 really should mean
> "uninitialized" and its a bad value IMO for a string.

Why waste an entire type for 'unitialized' which is a state that an nsAttrValue
should never be in? In the future we will use all four types to mean different
value-types. I'll change to use hex though. The last two #defines should be
removed since they're not used anywhere.

> This looks like an XXX comment to me.  But I'm not so sure about it.  You'd
> run into similar problems when using a union I imagine since you would still
> need to know the type when performing some operations on the pointer value.

I'm not sure what you're talking about here? What 'similar problem'? Note that
the existance of the html-type isn't a hack or anything. It's exists because I
don't want to kill htmlvalues in this patch since that is a huge amount of work.
See comment 2 step 5.

> I'd suggest just making this a copy constructor, since that is pretty much
> what it is.
Can't do that since my code isn't controlling when the nsAttrAndChildArray is
created. It would be nice to change the element-cloning code in the future so
that we can use the copy-consturctor, but that's a separate bug.


> >+  Impl* mImpl;
> Hmm.  Do you really think its worth it for mImpl to be an Impl* rather than
> just an Impl?
Investigating this and possibly (probably) changing this is a separate patch.
Increasing the size of elements requires some measurements and analysis before
we can do. This way we know we're better the what's on the trunk now.

> >+  if (!val) {
> >     // Since we are returning a success code we'd better do
> What success code?  :-)

Err.. NS_CONTENT_ATTR_NOT_THERE

> Whew!  What a name!  Can we do something with it, to shorten it?
> QNameToAttrName perhaps?
Feel free to raise this in a separate bug :). But you need the 'Get' in the
beginning since it can return null, and the 'Existing' to show that it only
returns names of existing attributes. 

> Aha!  Here are the real culprits!  Please also fix these comments as outlined
> in my earlier nit.

I'm removing this class in the next patch, so let's hold off with this.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Er... even if you list IBM as the original author and contributor there is still
a problem with listing yourself as a contributor?

Comment 14

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

Yup, i didn't bother asking why, it probably wouldn't have made sense to me anyway.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 137530 [details] [diff] [review]
Fix SetAttr signature  -w

+  nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
+		    const nsAString& aValue, PRBool aNotify)
+  {
+    return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify);
+  }

Seems like most (all?) these methods are identical now, wanna make it an inline
(virtual?) in nsIContent to avoid duplicating this all over the place?

- In nsXULElement.cpp:

-    return SetAttr(ni, aValue, PR_TRUE);
+    // XXX do we need a nodeinfo version on xul?
+    return SetAttr(ni->NamespaceID(), ni->NameAtom(), ni->GetPrefixAtom(),
+		    aValue, PR_TRUE);

Why would we need a nodeinfo version for XUL? Do we need that comment, or can
we settle this before checking in?

sr=jst

Comment 16

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

> +  nsresult SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
> +		    const nsAString& aValue, PRBool aNotify)
> +  {
> +    return SetAttr(aNameSpaceID, aName, nsnull, aValue, aNotify);
> +  }
> 
> Seems like most (all?) these methods are identical now, wanna make it an inline
> (virtual?) in nsIContent to avoid duplicating this all over the place?

Err... it's implemented everywhere to get around compileerrors due to one
SetAttr hiding the other as we discussed on irc a while back. All
implementations should be inlined already and virtual seems like unneccesary
overhead.

Comment 17

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

jst: are you done reviewing attachment 137090 [details] [diff] [review]? Your last line in comment 9 seems
to indicate "no"?

Comment 18

[Johnny Stenback (:jst)]

Nope, not yet. Though I should get through it tomorrow.

Comment 19

[Johnny Stenback (:jst)]

Comment on attachment 137090 [details] [diff] [review]
Stage 1, v1

- In nsGenericHTMLElement.cpp:

+
+  return nsnull;
+}
\ No newline at end of file

Add a newline.

 NS_IMETHODIMP
 nsSVGAttribute::GetPrefix(nsAString& aPrefix)
 {
-  mNodeInfo->GetPrefix(aPrefix);
+  if (mName.IsAtom()) {
+    SetDOMStringToNull(aPrefix);
+  }
+  else {
+    mName.NodeInfo()->GetPrefix(aPrefix);
+  }
+
   return NS_OK;
 }
...
 void
 nsSVGAttribute::GetQualifiedName(nsAString& aQualifiedName)const
 {
-  mNodeInfo->GetQualifiedName(aQualifiedName);
+  if (mName.IsAtom()) {
+    mName.Atom()->ToString(aQualifiedName);
+  }
+  else {
+    mName.NodeInfo()->GetQualifiedName(aQualifiedName);
+  }
 }

Seems like it'd be worth adding GetPrefix() and GetQualifiedName() on
nsAttrName, just as inlines to make code easier to read...

With that, and with what others commented on, sr=jst

Comment 20

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

Attachment: Stage 1, v2

This fixes all reviewcomments from jst and caillon other then the ones i've
commented on.

I decided to kill InitFrom entierly because when i used the same code on
html-elements (in a different tree i'm working on) things broke horribly. The
reason is that some elemenets assume that the clone-code will call SetAttr for
all attributes on the new element. This is of course a waste of time and we
should fix that by going through all implementations of nsIDOMNode::Clone and
make sure that they properly set all members. While we're at it we should look
into possibly using a copy-ctor rather then the current CopyInnerTo codepath.
The implementation of ::Clone for all the elements could probably be done using
a macro (for example the already existing NS_FORWARD_NSIDOMNODE_NO_CLONENODE
could be modified). All this should happen in an entierly different bug of
course.

Comment 21

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

Comment on attachment 138493 [details] [diff] [review]
Stage 1, v2

Rerequesting reviews since the changes were fairly big

Comment 22

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

Attachment: Stage 1, v2.1

This one includes the new nsAttrName.h and nsAttrValue.(cpp|h). Otherwise it
should be the exact same as attachment 138493 [details] [diff] [review]

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 138969 [details] [diff] [review]
Stage 1, v2.1

sr=jst

Comment 24

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

Regarding the second comment in comment 15:

The reason i put that comment in there is that XUL still stores attribute-names
as nodeinfos. So in the few cases where we are calling SetAttr and already have
a nsINodeInfo available we might take a small perf-hit from the death of the
nodeinfo-version of SetAttr (since we inside SetAttr will have to find the same
nodeinfo again).

However I'm pretty confident that killing SetAttr(nsINodeInfo...) is still a
gain all in all so I think we should go ahead and land as is. Don't really care
about if we leave the comment or not. In the end XUL should store
attribute-names using nsAttrNames and then the problem goes away entierly.

Comment 25

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 138969 [details] [diff] [review]
Stage 1, v2.1

>Index: base/src/nsAttrAndChildArray.cpp

>+ * Portions created by the Initial Developer are Copyright (C) 2003

2004?  Same for other new files.


>+nsresult
>+nsAttrAndChildArray::InsertChildAt(nsIContent* aChild, PRUint32 aPos)
>+{
...
>+  // First try to fit new child in existing buffer
>+  if (mImpl && offset + childCount < mImpl->mBufferSize) {
...
>+  }
>+
>+  // Try to fit new child in existing buffer by compressing attrslots

The above two comments make sense individually, but clash when read as a whole
in the flow of this method.  Perhaps you could reword the second one to
something like "The new child didn't fit in our buffer, so let's compress
attrslots and try again" or similar.


>+PRInt32
>+nsAttrAndChildArray::IndexOfChild(nsIContent* aPossibleChild) const
>+{
>+  if (!mImpl) {
>+    return -1;
>+  }
>+  void** children = mImpl->mBuffer + AttrSlotsSize();
>+  PRUint32 i, count = ChildCount();
>+  for (i = 0; i < count; ++i) {
>+    if (children[i] == aPossibleChild) {
>+      return (PRInt32)i;


NS_STATIC_CAST


>Index: base/src/nsGenericElement.cpp

> nsresult
> nsGenericContainerElement::UnsetAttr(PRInt32 aNameSpaceID,
>                                      nsIAtom* aName, PRBool aNotify)
> {
...

>+  if (HasMutationListeners(this, NS_EVENT_BITS_MUTATION_ATTRMODIFIED)) {
>+    nsCOMPtr<nsIDOMEventTarget> node(do_QueryInterface(NS_STATIC_CAST(nsIContent *, this)));
>+    nsMutationEvent mutation;
>+    mutation.eventStructType = NS_MUTATION_EVENT;
>+    mutation.message = NS_MUTATION_ATTRMODIFIED;
>+    mutation.mTarget = node;

Be sure and pick up the changes that bryner made to initializing events.


>Index: html/content/src/nsGenericHTMLElement.cpp

>@@ -3666,17 +3624,16 @@ nsGenericHTMLContainerElement::CopyInner
>   if (NS_FAILED(rv)) {
>     return rv;
>   }
> 
>   if (aDeep) {
>     PRInt32 indx;
>-    PRInt32 count = mChildren.Count();
>+    PRInt32 count = mAttrsAndChildren.ChildCount();
>     for (indx = 0; indx < count; indx++) {
>-      nsIContent* child = (nsIContent*)mChildren.ElementAt(indx);
>-
>-      nsCOMPtr<nsIDOMNode> node(do_QueryInterface(child));
>+      nsCOMPtr<nsIDOMNode> node(do_QueryInterface(
>+          mAttrsAndChildren.ChildAt(indx)));

Could you change this instead to:

       nsCOMPtr<nsIDOMNode> node =
	 do_QueryInterface(mAttrsAndChildren.ChildAt(indx));


>Index: shared/public/nsHTMLValue.h

>+  static void CopyToExistingBuffer(PRUnichar*& aBuf, PRUnichar* aOldBuf,
>+                                   const nsAString& aStr) {
>+    NS_ASSERTION(aOldBuf, "Cannot work on null buffer!");
>+    PRUint32 len = aStr.Length();
>+    aBuf = (PRUnichar*)nsMemory::Realloc(aOldBuf, sizeof(PRUint32) +
>+                                                  len * sizeof(PRUnichar));
>+    *((PRUint32*)aBuf) = len;

Could you change to c++ style casts (for both PRUnichar* and PRUint32*) here?

r=caillon

Comment 26

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 137530 [details] [diff] [review]
Fix SetAttr signature  -w

>diff -wu6r second/mozilla/content/svg/content/src/nsSVGAttributes.cpp trunk/mozilla/content/svg/content/src/nsSVGAttributes.cpp
>--- second/mozilla/content/svg/content/src/nsSVGAttributes.cpp	2003-12-09 14:49:12.000000000 -0600
>+++ trunk/mozilla/content/svg/content/src/nsSVGAttributes.cpp	2003-12-11 10:33:54.000000000 -0600

>-nsSVGAttribute::Create(nsINodeInfo* aNodeInfo,
>+nsSVGAttribute::Create(const nsAttrName& aName,
...
>-  *aResult = (nsSVGAttribute*) new nsSVGAttribute(aNodeInfo, value, flags);
>+  *aResult = (nsSVGAttribute*) new nsSVGAttribute(aName, value, flags);

I think the cast can safely go. 

r=caillon.

Comment 27

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

Attachment: Merged Stage1 and SetAttr-Fix to tip

This is what i'll check in tomorrow morning

Comment 28

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

Attachment: Patch that was checked in

More tip-merging and removed the xul-comments on request by jst

Comment 29

[Philip K. Warren]

The checkin for this bug has caused regression Bug 231104 on AIX.

Comment 30

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

Attachment: html-attributes, v1

This moves nsGenericHTMLElement to using the new array. The patch isn't as big
as it first looks, around half of it is removed files (nsHTMLAttributes.h|cpp)
and a s/nsHTMLI?Attributes/nsMappedAttributes/ change.

Comment 31

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

Attachment: trunkpatch to fix getAttribute for html-elements

Comment 32

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 139232 [details] [diff] [review]
trunkpatch to fix getAttribute for html-elements

It might be nice if we had a way to do the case conversion at the same time as
we do the UTF16 to UTF8 conversion, but we don't.  Oh well.  r=caillon.

Comment 33

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

Comment on attachment 139232 [details] [diff] [review]
trunkpatch to fix getAttribute for html-elements

checked in, thanks for reviews

Comment 34

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

I just found a problem with the code. AttrAt doesn't take mapped attributes into
account. I'll fix that in the next patch (i'm fairly confident i'll get
reviewcomments ;-) )

Comment 35

[Johnny Stenback (:jst)]

Comment on attachment 139227 [details] [diff] [review]
html-attributes, v1

- In nsAttrAndChildArray::AttrCount():

+  PRInt32 count = NonMappedAttrCount();
+  if (mImpl && mImpl->mMappedAttrs) {
+    count += mImpl->mMappedAttrs->Count();
   }

   return count;

Shouldn't all that be just:

+  return NonMappedAttrCount() + MappedAttrCount();

If that's not fast enough, then inline MappedAttrCount() (which is tiny).

- In nsAttrAndChildArray::GetAttr():

+  if (aNamespaceID == kNameSpaceID_None) {
[...]
+  }
+
+  if (mImpl && mImpl->mMappedAttrs && aNamespaceID == kNameSpaceID_None) {
+    return mImpl->mMappedAttrs->GetAttr(aLocalName);
   }

This checks aNamespaceID == kNameSpaceID_None twice, move the latter code
inside the first if.

- In nsAttrName.h:

+  PRUint32 HashValue() const
+  {
+    return mBits - 0;
+  }

This could use a comment.

- In nsAttrValue::GetStringValue():

+  static const PRUnichar blankStr[] = { '\0' };
+  void* ptr = NS_REINTERPRET_CAST(void*, mBits & NS_ATTRVALUE_VALUE_MASK);
+  return ptr
+	  ?
nsCheapStringBufferUtils::GetDependentString(NS_STATIC_CAST(PRUnichar*, ptr))
+	  : Substring(blankStr, blankStr);

You could use EmptyString() here now that it exists :-)

That's all for now. nsGenericHTMLElement.cpp is next.

Comment 36

[Johnny Stenback (:jst)]

Comment on attachment 139227 [details] [diff] [review]
html-attributes, v1

- In nsGenericHTMLElement.cpp:

- In ParseClassAttribute():

+  if (aStr.IsEmpty()) {
+    aValue.Reset();
+    
+    return NS_OK;
   }

+  nsAString::const_iterator iter, end;
+  aStr.BeginReading(iter);
+  aStr.EndReading(end);
+
+  // skip initial whitespace
+  while (nsCRT::IsAsciiSpace(*iter)) {
+    ++iter;
+    if (iter == end) {
+      aValue.Reset();
+
+      return NS_OK;
+    }
+  }


You could reduce the ammount of code above if you don't think the
aStr.IsEmpty() check at the beginning is essential for performance here. Maybe
something like this:

+  nsAString::const_iterator iter, end;
+  aStr.BeginReading(iter);
+  aStr.EndReading(end);
+
+  // skip initial whitespace
+  while (iter != end && nsCRT::IsAsciiSpace(*iter)) {
+    ++iter;
+  }

+  if (iter == end) {
+    aValue.Reset();
+
+    return NS_OK;
+  }

I'm not sure it's worth it tho, your call.

+  // parse the rest of the classnames
+  do {
+    start = iter;
+
+    do {
+      ++iter;
+    } while (iter != end && !nsCRT::IsAsciiSpace(*iter));

Wouldn't it be cool if the above little loop would live in an inline helper in
nsReadableUtils.h or somesuch place? Interested? :-) I'm sure there's a bunch
of places where we do exactly that.

- In nsGenericHTMLElement::SetAttrAndNotify():

+      rv = mAttrsAndChildren.SetAndTakeMappedAttr(aAttribute, aParsedValue,
+						       this, sheet);

Fix indentation.

- In nsGenericHTMLElement::GetID():

+    if (attrVal->GetType() == nsAttrValue::eAtom) {
+      NS_ADDREF(*aResult = attrVal->GetAtomValue());
+    }
+    else {
+      // XXX this shouldn't happen, but just in case

If it does, wanna assert?

+	 *aResult = NS_NewAtom(str);

And do some error checking on this call?

- In nsGenericHTMLElement::HasClass():

+      const char *class1, *class2;
+      aClass->GetUTF8String(&class1);
+      val->GetAtomValue()->GetUTF8String(&class2);
+
+      return nsCRT::strcasecmp(class1, class2) == 0;

This won't do a proper case insensitive compare, convert to UTF16 and do a
proper caseinsensitive string compare.

[...]
+	 if (nsCRT::strcasecmp(class1, class2) == 0) {
+	   return PR_TRUE;

Same here.

+      nsCOMPtr<nsISupports> supports =
+	 attrVal->GetHTMLValue()->GetISupportsValue();

I smell the need for deCOMtamination here, but nsHTMLValue is dieing, so I
guess we don't care.

Next up, nsHTMLAnchorElement.cpp

Comment 37

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

> - In nsGenericHTMLElement::HasClass():
> 
> +      const char *class1, *class2;
> +      aClass->GetUTF8String(&class1);
> +      val->GetAtomValue()->GetUTF8String(&class2);
> +
> +      return nsCRT::strcasecmp(class1, class2) == 0;
> 
> This won't do a proper case insensitive compare, convert to UTF16 and do a
> proper caseinsensitive string compare.

Hmm.. i think i'd like to make that change in a separate patch since this could
be quite a performance hit, this codepath is called *a lot*. The old code did
exactly what the code above does.

Ideally i would like to be able to get some sort of dependant string from an
nsIAtom so that we can call proper stringfunctions.

Comment 38

[Boris Zbarsky [:bzbarsky]]

David, Jungshik, see comment 37 and the relevant part of comment 36.

Comment 39

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

found another bug: nsGenericHTMLElement::HasClass doesn't honor |aCaseSensitive|
when the attr-value is an atomarray. Fixed locally.

Comment 40

[Johnny Stenback (:jst)]

Ok, I'm fine with fixing the case-insensitive UTF8 string compare issue in a
separate bug. Wanna file one?

Comment 41

[Johnny Stenback (:jst)]

Comment on attachment 139227 [details] [diff] [review]
html-attributes, v1

sr=jst with the above comments addressed.

Comment 42

[Christopher Aillon (sabbatical, not receiving bugmail)]

Comment on attachment 139227 [details] [diff] [review]
html-attributes, v1

> const nsAttrValue*
> nsAttrAndChildArray::GetAttr(nsIAtom* aLocalName, PRInt32 aNamespaceID) const
> {
>-  PRInt32 i = IndexOfAttr(aLocalName, aNamespaceID);
>-  if (i < 0) {
>-    return nsnull;
>+  PRUint32 i, slotCount = AttrSlotCount();
>+  if (aNamespaceID == kNameSpaceID_None) {
>+    // This should be the common case so lets make an optimized loop
>+    for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {

Shouldn't this use mBuffer[i * ATTRSIZE]?

>+      if (ATTRS(mImpl)[i].mName.Equals(aLocalName)) {
>+        return &ATTRS(mImpl)[i].mValue;
>+      }
>+    }
>+  }
>+  else {
>+    for (i = 0; i < slotCount && mImpl->mBuffer[i * 2]; ++i) {

And here too?  In fact, since we do this in so many places it might be a nice
idea to use something like:

#define LOOP_ATTR_SLOTS_BUFFER_LOOP_START(_iter, _count) \
  for (_iter = 0, _iter < _count && mImpl->mBuffer[_iter * ATTRSIZE]; ++_iter)
{
#define LOOP_ATTR_SLOTS_BUFFER_LOOP_END \
  }


>+      if (ATTRS(mImpl)[i].mName.Equals(aLocalName, aNamespaceID)) {
>+        return &ATTRS(mImpl)[i].mValue;
>+      }
>+    }
>+  }
>+
>+  if (mImpl && mImpl->mMappedAttrs && aNamespaceID == kNameSpaceID_None) {

Didn't you already check for kNameSpaceID_None? Seems this code needs to be
moved into one of the above branches.



>+nsresult
> nsAttrAndChildArray::RemoveAttrAt(PRUint32 aPos)
> {
>   NS_ASSERTION(aPos < AttrCount(), "out-of-bounds");
> 
>+  nsresult rv;

You're only using this in once place below.  Please just move it to where you
use it.


>+
>+  PRUint32 mapped = MappedAttrCount();
>+  if (aPos < mapped) {
>+    if (mapped == 1) {
>+      // We're removing the last mapped attribute.
>+      mImpl->mMappedAttrs->ReleaseUse();
>+      NS_RELEASE(mImpl->mMappedAttrs);
>+
>+      return NS_OK;
>+    }
>+
>+    nsRefPtr<nsMappedAttributes> mapped;
>+    rv = GetModifiableMapped(nsnull, nsnull, getter_AddRefs(mapped));
>+    NS_ENSURE_SUCCESS(rv, rv);

...

> const nsAttrName*
> nsAttrAndChildArray::GetSafeAttrNameAt(PRUint32 aPos) const
> {
>+  PRUint32 mapped = MappedAttrCount();
>+  if (aPos < mapped) {
>+    return (const nsAttrName*)mImpl->mMappedAttrs->NameAt(aPos);

NameAt() returns a |const nsAttrName*|, no need to cast.


>Index: content/base/src/nsAttrAndChildArray.h

>@@ -92,27 +96,40 @@ public:
>   const nsAttrValue* AttrAt(PRUint32 aPos) const
>   {
>     NS_ASSERTION(aPos < AttrCount(), "out-of-bounds access in nsAttrAndChildArray");
>     return &NS_REINTERPRET_CAST(InternalAttr*, mImpl->mBuffer)[aPos].mValue;

This needs to be updated, no?  Oh it would be nice if you moved out your macro
so it could be used here too (though maybe renamed).


>Index: content/base/src/nsAttrName.h
>@@ -163,12 +168,22 @@ public:
>     if (IsAtom()) {
>       SetDOMStringToNull(aStr);
>     }
>     else {
>       NodeInfo()->GetPrefix(aStr);
>     }
>+  }
>+
>+  PRUint32 HashValue() const
>+  {
>+    return mBits - 0;
>+  }

Um, this won't work on 64bit platforms?  Try NS_PTR_TO_INT32


>Index: content/base/src/nsAttrValue.cpp
>+nsDependentSingleFragmentSubstring
>+nsAttrValue::GetStringValue() const
>+{
>+  NS_ASSERTION(GetType() == eString,
>+               "Some dork called GetStringValue() on a non-string!");

Heh.  Shouldn't that be swedish dork?  :)

NS_PRECONDITION since its a precondition to calling the method.  Semantics
rule.  Also, make sure its documented in the header.

>+
>+  static const PRUnichar blankStr[] = { '\0' };
>+  void* ptr = NS_REINTERPRET_CAST(void*, mBits & NS_ATTRVALUE_VALUE_MASK);
>+  return ptr
>+         ? nsCheapStringBufferUtils::GetDependentString(NS_STATIC_CAST(PRUnichar*, ptr))
>+         : Substring(blankStr, blankStr);

I think someone added an EmptyString() accessor you could maybe use instead of
rolling this your self.


>+}
>+
>+const nsHTMLValue*
>+nsAttrValue::GetHTMLValue() const
>+{
>+  NS_ASSERTION(GetType() == eHTMLValue,
>+               "Some dork called GetHTMLValue() on a non-htmlvalue!");

Same as above.

>+  return NS_REINTERPRET_CAST(nsHTMLValue*, mBits & NS_ATTRVALUE_VALUE_MASK);
>+}
>+
>+nsIAtom*
>+nsAttrValue::GetAtomValue() const
>+{
>+  NS_ASSERTION(GetType() == eAtom,
>+               "Some dork called GetAtomValue() on a non-atomvalue!");

Ditto :)

>+  return NS_REINTERPRET_CAST(nsIAtom*, mBits & NS_ATTRVALUE_VALUE_MASK);
>+}
>+
>+PRUint32
>+nsAttrValue::HashValue() const
>+{
>+  void* ptr = NS_REINTERPRET_CAST(void*, mBits & NS_ATTRVALUE_VALUE_MASK);
>+  switch(GetType()) {
>+    case eString:
>+    {
>+      PRUnichar* str = NS_STATIC_CAST(PRUnichar*, ptr);
>+      return str ? nsCheapStringBufferUtils::HashCode(str) : 0;
>+    }
>+    case eHTMLValue:
>+    {
>+      return NS_STATIC_CAST(nsHTMLValue*, ptr)->HashValue();
>+    }
>+    case eAtom:
>+    {
>+      return NS_PTR_TO_INT32(ptr);
>+    }
>+  }
>+
>+  NS_ERROR("shouldn't get here");

NS_NOTREACHED("unknown attrvalue type");

>+  return 0;
>+}
>+
>+PRBool
>+nsAttrValue::EqualsIgnoreCase(const nsAttrValue& aOther) const
>+{
>+  if (GetType() != aOther.GetType()) {
>+    return PR_FALSE;
>+  }
>+
>+  switch(GetType()) {
>+    case eString:
>+    {
>+      return GetStringValue().Equals(aOther.GetStringValue(),
>+                                     nsCaseInsensitiveStringComparator());
>+    }
>+    case eHTMLValue:
>+    {
>+      return *GetHTMLValue() == *aOther.GetHTMLValue();
>+    }
>+    case eAtom:
>+    {
>+      const char *class1, *class2;
>+      GetAtomValue()->GetUTF8String(&class1);
>+      aOther.GetAtomValue()->GetUTF8String(&class2);
>+
>+      return nsCRT::strcasecmp(class1, class2) == 0;
>+    }
>   }
>+
>+  NS_ERROR("shouldn't get here");

Again, NS_NOTREACHED as per above.




>Index: content/base/src/nsMappedAttributes.cpp
>+
>+nsresult
>+nsMappedAttributes::GetAttribute(nsIAtom* aAttrName,
>+                                 nsHTMLValue& aValue) const
>+{
>+  NS_ASSERTION(aAttrName, "null-name");

PRECONDITION?  And space, not a dash between the words.


>+const nsAttrValue*
>+nsMappedAttributes::GetAttr(nsIAtom* aAttrName) const
>+{
>+  NS_ASSERTION(aAttrName, "null-name");

Same.



>Index: content/base/src/nsMappedAttributes.h

>+  virtual ~nsMappedAttributes();

Does this need to be virtual?




>Index: content/html/content/src/nsGenericHTMLElement.cpp
> nsresult
> nsGenericHTMLElement::GetID(nsIAtom** aResult) const
> {
>-  if (mAttributes) {
>-    return mAttributes->GetID(aResult);
>-  }
>-
>   *aResult = nsnull;
> 
>+  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(nsHTMLAtoms::id);
>+  if (attrVal) {
>+    if (attrVal->GetType() == nsAttrValue::eAtom) {
>+      NS_ADDREF(*aResult = attrVal->GetAtomValue());
>+    }
>+    else {
>+      // XXX this shouldn't happen, but just in case

Should we just remove this code then, and place an assertion here instead?

>+      nsAutoString str;
>+      attrVal->ToString(str);
>+      if (!str.IsEmpty()) {
>+        *aResult = NS_NewAtom(str);
>+      }
>+    }
>+  }
>+
>   return NS_OK;
> }


>+nsGenericHTMLElement::ReparseStyleAttribute()
> {
...
>+      // Don't bother going through SetHTMLAttribute, we don't want to fire off
>+      // mutation-events or document-notifiactions anyway

check your spelling dude.  What's with the dashes btw?	"mutation events or
document notifications".


> void
>+nsGenericHTMLElement::MapBackgroundAttributesInto(const nsMappedAttributes* aAttributes,
>                                                   nsRuleData* aData)
> {
...
>           // Resolve url to an absolute url
>           // XXX this breaks if the HTML element has an xml:base
>           // attribute (the xml:base will not be taken into account)
>+          // as well as elements with _baseHref set. We need to be able
>+          // to get to the element somehow, or store the baseuri in the

base URI


>Index: content/html/content/src/nsGenericHTMLElement.h
>+  /**
>+   * Turn an attribute value into string based on the type of attribute
>+   * (does not need to do standard types such as string, integer, pixel,
>+   * color ...).  Called by GetAttr().
>+   *
>+   * @param aAttribute the attribute to convert
>+   * @param aValue the value to convert
>+   * @param aResult the string [OUT]
>+   * @throws NS_CONTENT_ATTR_HAS_VALUE if the value was successfully converted
>+   * @throws NS_CONTENT_ATTR_NOT_THERE if the value could not be converted

NS_CONTENT_ATTR_HAS_VALUE and NS_CONTENT_ATTR_NOT_THERE are success codes, not
error codes.

So this method doesn't throw.  Use @return instead (or is it @returns?).


>+   * @see nsGenericHTMLElement::GetAttr
>+   */
>   NS_IMETHOD AttributeToString(nsIAtom* aAttribute,
>                                const nsHTMLValue& aValue,
>                                nsAString& aResult) const;
>+
>+  /**
>+   * Convert an attribute string value to attribute type based on the type of
>+   * attribute.  Called by SetAttr().
>+   *
>+   * @param aAttribute to attribute to convert
>+   * @param aValue the string value to convert
>+   * @param aResult the HTMLValue [OUT]
>+   * @throws NS_CONTENT_ATTR_HAS_VALUE if the string was successfully converted
>+   * @throws NS_CONTENT_ATTR_NOT_THERE if the string could not be converted

Same as above.

>+   * @see nsGenericHTMLElement::SetAttr
>+   */
>+  NS_IMETHOD StringToAttribute(nsIAtom* aAttribute,
>+                               const nsAString& aValue,
>+                               nsHTMLValue& aResult);


>Index: content/shared/public/nsHTMLValue.h

>@@ -360,12 +364,14 @@ protected:
>     /** String.  First 4 bytes are the length, non-null-terminated. */
>     PRUnichar*    mString;
>     /** ISupports.  Strong reference.  */
>     nsISupports*  mISupports;
>     /** Color. */
>     nscolor       mColor;
>+    /** Arrat if atoms */

Uh, what?  :)

>+    nsCOMArray<nsIAtom>* mAtomArray;
>   } mValue;



r=caillon with stuff addressed.

Comment 43

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

> And here too?  In fact, since we do this in so many places it might be a nice
> idea to use something like:
> 
> #define LOOP_ATTR_SLOTS_BUFFER_LOOP_START(_iter, _count) \
>   for (_iter = 0, _iter < _count && mImpl->mBuffer[_iter * ATTRSIZE]; ++_iter)
> {
> #define LOOP_ATTR_SLOTS_BUFFER_LOOP_END \
>   }

I think that'd reduce readability of the code so i'd rather stick to what i have.

> Um, this won't work on 64bit platforms?  Try NS_PTR_TO_INT32

Actually, the above is what NS_PTR_TO_INT32 does, i've added comment explaining.

> I think someone added an EmptyString() accessor you could maybe use instead of
> rolling this your self.

Can't do that, the function is returning an nsDependentSingleFragmentSubstring
so that won't compile. And I have to return a real object rather then a
reference since what i'd reference would go out of scope as the function exits.

Comment 44

[Christopher Aillon (sabbatical, not receiving bugmail)]

> Actually, the above is what NS_PTR_TO_INT32 does, i've added comment explaining.
Yeah, I realized that after the fact.  Not sure what I was thinking.

Comment 45

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

Attachment: Patch that was checked in

Comment 46

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

This seems to have caused a regression in Tp. The only thing i can think of
causing this is that we might be doing more heapallocations now since we're
using more nsHTMLValue-pointers rather then using inline nsHTMLValues.

There shouldn't be a whole lot more allocations though since we used to allocate
HTMLAttribute structs instead which should cause almost as many allocations. The
only case where i can find that we are actually doing more allocations is for
the first (possibly first two) attributes in nsMappedAttributes. It used to be
that the first attribute name+value was inlined, but now they all go in a
separate buffer.

I'm working on a patch that will inline a small buffer in nsMappedAttributes,
but it'll take a couple of days to finish? Hope it's ok that we're living with
the perfregression until then (seems to be in the order of .5 - 1%).

Comment 47

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

Oh, i should say that the real fix is to kill nsHTMLValue, but that will take
far more work, but it's something i'm planning on doing. That way we can
heap-allocate a lot fewer objects.

I need to instrument how much different value-types are used. We might need to
make nsAttrValue be 8 bytes big rather then 4 bytes and have separate type and
value members, which is what nsHTMLValue does. I had hoped to be able to squeeze
down that size, but it might not be possible.

Comment 48

[Sam Emrick]

I'd suggest perhaps... a look into who does the most dynamic 
nsHTMLValue manipulations, such as highest frequency usages of:
 
nsAttrValue::nsAttrValue
nsAttrValue::~nsAttrValue
nsAttrValue::SetTo
etc, in;  

Likely, most frequent callers of this can be made smarter. Also, not 
looked most resent code, but in lxr there is inane code to clear data 
just before manipulation or free of same. Some sort of ifdef debug 
would reduce memory churn. Consider also cases where code loops through 
constructor/destructor pairs with normal case is nothing more happening 
between.

(Overall, tres grande bon so far, but it sure is an onion, isn't it.)

Comment 49

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

Attachment: Patch fix perfregression

This patch does two things:

To fix the perf-regression i've tried to inline the attribute buffer of
nsMappedAttributes into the class rather then to allocate it separatly. This is
done by creating a custom |operator new| that allocates enough space to fit the
needed number of attributes.
We won't have to worry about running out of bufferspace since we are allocating
a new nsMappedAttributes object every time an attribute is set, so in the cases
where we're adding a new attribute we just need to make sure we can fit one
more then what's in the current map. (Changing that is covered by bug 166450). 



I also realized that the entire AddUse/ReleaseUse code was unneccesary. The
only purpose it has ever served is to remove the the map from the document-hash
when there is no more elements using the map. Removing it from the map has no
sideeffects what so ever. However we might as well do that when the map is
deleted instead.
Other then simplifying the code this is actually a theoretical performance
increase. In some cases when mucking with attributes we might be able to grab a
map that is already in the tree, rather then to have to insert a new one.
There is no risk of (additional) leaks, keeping the map in the hash won't
change its lifetime since the hash holds weak references.

Comment 50

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

In previous comment, change "document-hash" to "stylesheet-hash", i.e. the hash
in the stylesheet that keeps track of all nsMappedAttributes objects for the
elements in a document

Comment 51

[Johnny Stenback (:jst)]

Comment on attachment 140040 [details] [diff] [review]
Patch fix perfregression

sr=jst

Comment 52

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

Attachment: Stage 3, v1

This patch implements stage 3 from comment 1. I.e. makes XUL use the new
nsAttrAndChildArray class for storing attributes and children.

This includes a lot of cleanup and getting rid of a lot of codeduplication. For
example gems like:
* Only parse special attributes such as 'class' and 'style' in 2 locations (one

  for prototypes and one for normal attributes) rather then 4.
* Don't copy classlist and inline style just because an attribute is set
  locally. The new code uses the classlist and inline style from the prototype
  as long as the prototype is available.
* Use a function to search prototype attributes.
* Make the id-attribute always be stored as an atom in prototypes too. The old
  code wouldn't when the id was bigger then 12 characters.
* Remove the nsXULAttributes.cpp|h and nsXULAttributeValue.cpp|h files.
* Only make an element heavyweight when an attribute is removed. Accessing or
  modifying .style only sets the style attribute.

Comment 53

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

Attachment: Stage 3, v1 -w

Same as previous with -w

Comment 54

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

Oh, btw, this'll be the last patches i use this bug for, i promice. Well, modulo
reviews of course. I'll file separate bugs for stage 4 and 5.

(Stage 2 was more or less done in the first patch)

Comment 55

[Boris Zbarsky [:bzbarsky]]

> * Don't copy classlist and inline style just because an attribute is set
>  locally.

Unless it's the style or class attribute, right?

Comment 56

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

> > * Don't copy classlist and inline style just because an attribute is set
> >  locally.
> 
> Unless it's the style or class attribute, right?

Well, then it's just set, not copied :)

Comment 57

[Boris Zbarsky [:bzbarsky]]

Ah, OK.  Excellent.

Comment 58

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

Comment on attachment 140040 [details] [diff] [review]
Patch fix perfregression

got r=caillon over irc with two changes.

Comment 59

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

Attachment: Patch that was checked in

Includes the two fixes requested by caillon.

Comment 60

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 140259 [details] [diff] [review]
Patch that was checked in

>Index: content/base/src/nsMappedAttributes.cpp
>+void* nsMappedAttributes::operator new(size_t aSize, PRUint32 aAttrCount)
>+{

>Index: content/base/src/nsMappedAttributes.h
>+  void* operator new(size_t size, PRUint32 aAttrCount = 1) CPP_THROW_NEW;

This inconsistency broke the HP build.	The definition is missing
CPP_THROW_NEW.

Comment 61

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

Comment on attachment 140259 [details] [diff] [review]
Patch that was checked in

Doesn't seem like this improved performance as much as I had hoped. I've filed
bug 232706 on the other thing that is probably causing performance problems
with this new code. I should be able to come up with a partial fix for that one
next week, I hope that's good enough cause I really would like to avoid backing
anything out. Especially considering the amout of codecleanups that this new
code allows.

Comment 62

[Boris Zbarsky [:bzbarsky]]

Frankly, I would say we should go on with the changes here (HTMLValue removal,
etc), then just profile various attribute use cases once we're done to look for
things to improve.  I was going to rerun some profiles of attribute setting
anyway once this work was done....

Comment 63

[Johnny Stenback (:jst)]

Comment on attachment 140228 [details] [diff] [review]
Stage 3, v1 -w

- In nsAttrName.h:

+  void SetTo(nsIAtom* aAtom)
+  {
+    NS_ASSERTION(aAtom, "null nodeinfo-name in nsAttrName");

You mean atom-name here, right? :-)

- In nsXULElement.cpp:

+static const PRUint32 kMaxAtomValueLength = 12;

I don't see this being used anywhere, and this doesn't really belong here any
more either.

- In nsXULElement::~nsXULElement()

-    // Force child's parent to be null. This ensures that we don't
-    // have dangling pointers if a child gets leaked.
-    for (PRInt32 i = mChildren.Count() - 1; i >= 0; --i) {
-	 nsIContent* child = NS_STATIC_CAST(nsIContent*, mChildren[i]);
-	 child->SetParent(nsnull);
-	 NS_RELEASE(child);
-    }

Um, why remove this code? Seems like we still need this...

- In nsXULElement::GetAttributeNode():

     if (node) {
-	 return CallQueryInterface(node, aReturn);
+      rv = CallQueryInterface(node, aReturn);
+      NS_ENSURE_SUCCESS(rv, rv);

This is a diff -w, but this still looks like bad indentation. Use 4 spaces in
this file.

- In nsXULElement::SetAttributeNode():

+    if (returnNode) {
+      rv = CallQueryInterface(returnNode, aReturn);
+      NS_ENSURE_SUCCESS(rv, rv);

Ditto.

+    }

-    NS_ADDREF(aNewAttr);
-    *aReturn = aNewAttr;
     return NS_OK;

Loose the NS_ENSURE_SUCCESS(rv, rv) and just return rv. Why waste cycles on
checking a value if you can just return it? Same in GetAttributeNode(),
RemoveAttributeNode(), GetAttributeNodeNS(), and SetAttributeNodeNS(). There
might be more, please make sure you go through and eliminate all of these.

- In nsXULElement::GetAttributeNS():

-    GetAttr(nsid, name, aReturn);
-
-    return NS_OK;
+    return GetAttr(nsid, name, aReturn);

Hmm, this will expose the NS_CONTENT_ATTR_NOT_THERE success code in places
where it IMO doesn't belong. I'd leave this as is, or if you really think this
is a good idea, change all places (i.e. nsGenericElement n' friends too).


+nsresult
+nsXULElement::MaybeAddPopupListener(nsIAtom* aLocalName)

Why nsresult? This just returns NS_OK. A void function would be less code...

- In nsXULElement::IndexOf():

     nsresult rv;
     if (NS_FAILED(rv = EnsureContentsGenerated())) {
	 return -1;
     }

Maybe loose rv there?

- In nsXULElement::GetExistingAttrNameFromQName():

-    NS_ConvertUCS2toUTF8 utf8String(aStr);
+  const nsAttrName* name = InternalGetExistingAttrNameFromQName(aStr);
+  if (!name) {
+    return nsnull;
+  }

4-space indentation, same in ParseClassAttribute().

Oh, and isn't ParseClassAttribute() identical to the one in
nsGenericHTMLElement.cpp? If so, just make that one non-static and call it from
here (and comment on the fact that the one in nsGHE is used by others too).

- nsXULElement::ParseStyleRule() looks an aweful lot like
nsGenericHTMLElement::ParseStyleAttribute(), care to share the code?

- In nsXULElement::SetAttr():

...
+	 // here is an optimization, the check for haveListeners is a
correctness

+	 // issue.

Extra newline in comment?

+	     if (modification && aValue.Equals(oldValue)) {
+	       return NS_OK;
+	     }

4-space indentation.

+	 const nsHTMLValue* htmlVal;
+	 if (val->GetType() == nsAttrValue::eAtom) {
+	     // NOTE atom is not addrefed
+	     aArray.AppendElement(val->GetAtomValue());
+	 }

Yay, nsHTMLValue in nsXULElement! nsHTMLValue soo needs to die! :-)

- In nsXULElement.h:

     nsXULPrototypeAttribute()
-	 : mEventHandler(nsnull)
+	 : mEventHandler(nsnull),
+	   mName(nsXULAtoms::id)   // XXX this is a hack, but names have to
have a value

It'd be kinda cool to not need this. One way around it would be to change how
you construct these guys. You could simply new a chunk of memory, and then loop
through and placement new these guys over the memory you just allocated where
you construct these in chunks. Not sure which one is less uggly...

- In nsXULPrototypeDocument.cpp, GetNodeInfos():

+    if (aArray.IndexOf(aPrototype->mNodeInfo) < 0) {

This doesn't scale very well. But does it matter? We end up doing a lot of
linear searches here. Would a hash be better suited here?

sr=jst

Comment 64

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

> - In nsXULElement::~nsXULElement()
> 
> -    // Force child's parent to be null. This ensures that we don't
> -    // have dangling pointers if a child gets leaked.
> -    for (PRInt32 i = mChildren.Count() - 1; i >= 0; --i) {
> -	 nsIContent* child = NS_STATIC_CAST(nsIContent*, mChildren[i]);
> -	 child->SetParent(nsnull);
> -	 NS_RELEASE(child);
> -    }
> 
> Um, why remove this code? Seems like we still need this...

The nsAttrAndChildArray class does exactly that in its dtor.

> - In nsXULElement::SetAttr():
> 
> ...
> +	 // here is an optimization, the check for haveListeners is a
> correctness
> 
> +	 // issue.
> 
> Extra newline in comment?

Not sure what you mean here. I've reformatted the comment though so no lines are
longer then 80 characters.

> +	 const nsHTMLValue* htmlVal;
> +	 if (val->GetType() == nsAttrValue::eAtom) {
> +	     // NOTE atom is not addrefed
> +	     aArray.AppendElement(val->GetAtomValue());
> +	 }
> 
> Yay, nsHTMLValue in nsXULElement! nsHTMLValue soo needs to die! :-)

Yup, see bug 232706. I've got some great ideas for how to do this. Soon to be
implemented in a patch near you :)

> - In nsXULElement.h:
> 
>      nsXULPrototypeAttribute()
> -	 : mEventHandler(nsnull)
> +	 : mEventHandler(nsnull),
> +	   mName(nsXULAtoms::id)   // XXX this is a hack, but names have to
> have a value
> 
> It'd be kinda cool to not need this. One way around it would be to change how
> you construct these guys. You could simply new a chunk of memory, and then loop
> through and placement new these guys over the memory you just allocated where
> you construct these in chunks. Not sure which one is less uggly...

I think i'd like to stick with the current hack. I seriously doubt that there's
a performance problem here since it'll just result in an extra addref/relese
(which probably will happen on a permanent atom even). It would be a lot more
work to malloc and explicitly call the ctor/dtor. Especially since we allocate
the attributes in 3 different places, (xul contentsink, xbl contentsink and
fastload deserializer).

> - In nsXULPrototypeDocument.cpp, GetNodeInfos():
> 
> +    if (aArray.IndexOf(aPrototype->mNodeInfo) < 0) {
> 
> This doesn't scale very well. But does it matter? We end up doing a lot of
> linear searches here. Would a hash be better suited here?

First of all I don't think we really care. Serializing will only happen the
first time ever a piece of chrome is loaded. All subsequent loads will be
fastloaded. Even after restarts.
Second the scaling problem isn't that big since it scales with number of
different nodenames in a document, not with the size of the document. So only if
we add a lot new element-types/attributes to the xul spec this will become an issue.

Comment 65

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Jonas and I already discussed that on IRC but I want to write it again here:
the loss of attributes' order will be considered as a regression by Composer
users. I understand perfectly the reason why this is done and the impact on the
browser. But Mozilla is not just a browser. We also discussed a possible solution
to the problem for the editor that would enable us to have a colorized source
view quite easily I must say... I am buying that! We need to find a way to
preserve again attributes order in Composer.

Comment 66

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

Attachment: Stage 3, v2

Addresses jsts comments

Comment 67

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

Attachment: Stage 3, v2 -w

Same as previous with -w

Comment 68

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

Comment on attachment 141014 [details] [diff] [review]
Stage 3, v2 -w

forwarding sr from jst

Comment 69

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 141014 [details] [diff] [review]
Stage 3, v2 -w

>Index: content/base/src/nsAttrAndChildArray.h
>+  const nsAttrName* GetExistingAttrNameFromQName(const nsACString& aName) 
const;

Comment that this is UTF-8.


>Index: content/base/src/nsAttrValue.cpp
>+nsAttrValue::SetToStringOrAtom(const nsAString& aValue)
>+{
>+  if (len && len <= NS_ATTRVALUE_MAX_STRINGLENGTH_ATOM) {

Add a comment as to why "" is not atomized here?

>Index: content/base/src/nsGenericElement.cpp
>+  // Hold a strong reference here so that the atom or nodeinfo doesn't go
>+  // away during UnsetAttr.
>+  nsAttrName tmp(*name);

This should explain _why_ it's bad if they do....

>Index: content/html/content/src/nsGenericHTMLElement.h
>+   * Note: this function is used by other classes then nsGenericHTMLElement

"by classes other than" (not "then")

>Index: content/xbl/src/nsXBLContentSink.cpp

Remove the no-longer-used mCSSParser from nsXBLContentSink.h, eh?

>Index: content/xul/content/src/nsXULElement.cpp
> nsXULElement::GetAttributes(nsIDOMNamedNodeMap** aAttributes)
>+    NS_ENSURE_ARG_POINTER(aAttributes);

Just assert; don't check.

> nsXULElement::CloneNode(PRBool aDeep, nsIDOMNode** aReturn)

>             result->SetDocument(mDocument, PR_TRUE, PR_TRUE);

Could we have a nice XXX comment before that line?  That's setting mDocument on
nodes not in the tree.... Needs to be done because XBL is all broken like that
and some chrome expects XBL to bind to nodes not in a tree that are clones.

> nsXULElement::GetAttributeNode(const nsAString& aName,

>+    NS_ENSURE_ARG_POINTER(aReturn);

Again, just assert.

> nsXULElement::SetAttributeNode(nsIDOMAttr* aNewAttr, nsIDOMAttr** aReturn)

>+    NS_ENSURE_ARG_POINTER(aReturn);

And here.

> nsXULElement::RemoveAttributeNode(nsIDOMAttr* aOldAttr, nsIDOMAttr** aReturn)

>+    NS_ENSURE_ARG_POINTER(aReturn);

And here.

> nsXULElement::HasAttribute(const nsAString& aName, PRBool* aReturn)

>   NS_ENSURE_ARG_POINTER(aReturn);

Just assert.

>+nsXULElement::AddListenerFor(const nsAttrName& aName,
>+    if (aName.IsAtom()) {

Comment that these are always in the null namespace?

> nsXULElement::InsertChildAt(nsIContent* aKid, PRUint32 aIndex, PRBool aNotify,
>+    if (aIndex > mAttrsAndChildren.ChildCount()) {
>+      // This *does* happen, probably a bug in the overlay code.
>         return NS_ERROR_FAILURE;

Could we NS_ERROR there?  Maybe someone will fix the bug?  ;)

> nsXULElement::SetAttr(PRInt32 aNamespaceID, nsIAtom* aName, nsIAtom* aPrefix,
>+        // when we have no listeners and don't plan to notify.  The check for
>+        // aNotify here is an optimization, the check for haveListeners is a
>+        // correctness issue.
>+        if (hasListeners || aNotify || isAccessKey) {

And the check for isAccessKey?

>+            // It would be nice to not have to call GetAttr here but to rather
>+            // just grab the nsAttrValue from mAttrsAndChildren and compare that

Yeah, and we can do that now!  At least here; no enums in XUL.

>+    // WARNING!!
>+    // This code is largly duplicated in nsXULPrototypeElement::SetAttrAt.

"largely"

>+nsXULElement::SetAttrAndNotify(PRInt32 aNamespaceID,
>+            // It sucks that we have to call GetAttr just to get the
>+            // stringvalue here. Ideally we would be able to convert all
>+            // nsAttrValues to strings.

For XUL you can, no?  No enums!  ;)

> nsXULElement::UnsetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, PRBool aNotify)

>         // otherwords, we'll become ``heavyweight''.

"other words"

>+    // It sucks that we have to call GetAttr just to get the
>+    // stringvalue here. Ideally we would be able to convert all
>+    // nsAttrValues to strings.
>+    GetAttr(aNameSpaceID, aName, oldValue);

No enums!  So we can.

>     // If the accesskey attribute is removed, unregister it here
>     // Also see nsAreaFrame, nsBoxFrame and nsTextBoxFrame's AttributeChanged
>-    if (aNameSpaceID == kNameSpaceID_None &&
>-        (aName == nsXULAtoms::accesskey || aName == nsXULAtoms::control))
>+        if (aName == nsXULAtoms::accesskey || aName == nsXULAtoms::control) {

SetAttr didn't handle the control attr... should it have?

>     // Check to see if the OBSERVES attribute is being unset.  If so, we
>     // need to remove our broadcaster goop completely.
>-    if (mDocument && (aNameSpaceID == kNameSpaceID_None) &&
>-        (aName == nsXULAtoms::observes || aName == nsXULAtoms::command)) {
>+        if (mDocument && (aName == nsXULAtoms::observes ||
>+                          aName == nsXULAtoms::command)) {

Again, should SetAttr handle this?  File followup bugs as needed.

> nsXULElement::GetAttrNameAt(PRUint32 aIndex, PRInt32* aNameSpaceID,
> #ifdef DEBUG_ATTRIBUTE_STATS
>-    int local = Attributes() ? Attributes()->Count() : 0;
>     int proto = mPrototype ? mPrototype->mNumAttributes : 0;
>     fprintf(stderr, "GANA: %p[%d] of %d/%d:", (void *)this, aIndex, local, proto);
> #endif

Please build with DEBUG_ATTRIBUTE_STATS before checking in -- that should catch
errors like that....

> nsXULElement::GetClasses(nsVoidArray& aArray) const

File a bug to make this api suck less (be deCOMified and return some sort of
sane value that's just an internal pointer to what we're holding; perhaps
nsAttrValue*?).

> nsXULElement::GetInlineStyleRule(nsICSSStyleRule** aStyleRule)

Per our conversation, file a bug to have MakeHeavyweight clone the rule and
all.


> nsresult nsXULElement::MakeHeavyweight()
>           // XXXshaver Snapshot the local attrs, so we don't search the ones we
>           // XXXshaver just appended from the prototype!

File a bug on this?

>+        if (protoattr->mName.IsAtom()) {
>+            rv = mAttrsAndChildren.SetAndTakeAttr(protoattr->mName.Atom(), attrValue);
>       }
>+        else {
>+            rv = mAttrsAndChildren.SetAndTakeAttr(protoattr->mName.NodeInfo(),
>+                                                  attrValue);
>+        }

That cries out for a SetAndTakeAttr() that takes an nsAttrName.

r=bzbarsky with that

Comment 70

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

> > nsXULElement::InsertChildAt(nsIContent* aKid, PRUint32 aIndex, PRBool aNotify,
> >+    if (aIndex > mAttrsAndChildren.ChildCount()) {
> >+      // This *does* happen, probably a bug in the overlay code.
> >         return NS_ERROR_FAILURE;
> 
> Could we NS_ERROR there?  Maybe someone will fix the bug?  ;)

We'd get a ton of assertions on startup so i'd prefer not to.

> > nsresult nsXULElement::MakeHeavyweight()
> >           // XXXshaver Snapshot the local attrs, so we don't search the ones
> >           // XXXshaver just appended from the prototype!
> 
> File a bug on this?

I'm not sure what shaver had in mind. We seldom have enough attributes that it'd
matter so i don't think this is worthy of a bug.

Comment 71

[Boris Zbarsky [:bzbarsky]]

> I'm not sure what shaver had in mind.

Make a local copy of the attr array and use that for the indexof calls.

If we don't think we'll need that, just remove the comment.

Comment 72

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

Attachment: Patch that was checked in

checked in

Comment 73

[Boris Zbarsky [:bzbarsky]]

That slowed down Ts and Txul a little bit...

Comment 74

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

The only thing that i can think of that slowed down is GetAttrNameAt and
GetAttrCount.
Before we used to much more often just make the element heavyweight which ment
that GetAttrNameAt and GetAttrCount didn't have to bother with finding
attributes existing both in the prototype and locally.
However with the new code we rarly make an element heavyweight which means that
those operations will be slow.

I'll look into if I can speed them up.

Other theories welcome of course :)

Comment 75

[Boris Zbarsky [:bzbarsky]]

Perhaps it's because we now set attrs one at a time instead of preallocating the
array?  What's the allocation behavior of nsAttrAndChildArray?  How many
allocations will it do for 5 attrs, eg?  Is it possible to tell it to
preallocate space for all the attrs from the content sink's "I know how many
attrs there are" loop?  If so, may be worth doing.

Comment 76

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

Ah, that's a good suggestion. In fact, we now end up with a lot of
childpointer-shuffeling since the child-pointers are located after the
attributes in the array so we have to move all the children for each attribute
added. This usually isn't a big deal since children are added after attributes
during normal parsing. However here it could defenetly be a hit.

Comment 77

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

Marking this closed. I filed bug 235511 on the regression in Txul and bug 235512
on stage 4 in comment 1.

Thanks to everyone that helped comming up with the design of the new code and
that helped with reviewing patches!