148368 - convert mapped attribute table to pldhash

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

I was looking at a profile for the big callers of nsHashtable::Get, and the
mapped attribute table was #3 in the profile (where #1 and #2 were the
already-changed-to-pldhash RuleHash (bug 112318) and the binding manager's
binding table).

So, patch to convert the mapped attribute table to pldhash (which should be a
bit of memory savings, too) coming up...

Comment 1

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

Attachment: patch

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 85759 [details] [diff] [review]
patch

sr=bzbarsky

Comment 3

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

Comment on attachment 85759 [details] [diff] [review]
patch

+  memset(entry, 0, sizeof(MappedAttrTableEntry));

are you allowed to clear out the entire entry including the PLDHashEntryHdr
part? Wouldn't entry->mAttributes = nsnull do?

in HTMLStyleSheetImpl::Reset
+  PL_DHashTableFinish(&mMappedAttrTable);
+  mMappedAttrTable.ops = nsnull;
do you need an if (mMappedAttrTable.ops) around that?

in HTMLStyleSheetImpl::UniqueMappedAttributes
+  if (!mMappedAttrTable.ops) {
+    PL_DHashTableInit(&mMappedAttrTable, &MappedAttrTable_Ops, nsnull,
+		       sizeof(MappedAttrTableEntry), 16);
   }
you should check if this succeeds


 HTMLStyleSheetImpl::DropMappedAttributes(nsIHTMLMappedAttributes* aMapped)
 {
+  NS_ENSURE_TRUE(aMapped, NS_OK);

if it's valid to pass null as aMapped then you should probably use 
if (!aMapped)
  return NS_OK;
instead since NS_ENSURE warns.


r=sicking

Comment 4

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

The |memset| is what the default ClearEntry op does, and is needed to maintain
the invariant that all table entries are zero-initialized.

I had that null-check in Reset in my tree already -- I probably put it in just
after writing the patch because it crashed, and then forgot to reattach the patch.

I added a success-check to the PL_DHashTableInit call and return
NS_ERROR_OUT_OF_MEMORY with a null out parameter.

It's not (IMO) valid to pass null, but the old code allowed it, so I changed it
to an assertion with NS_OK return.

Comment 5

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

Fix checked in, 2002-08-17 05:17 PDT.