convert mapped attribute table to pldhash

RESOLVED FIXED in mozilla1.2alpha

Status

()

Core
CSS Parsing and Computation
P2
normal
RESOLVED FIXED
16 years ago
15 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.2alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch][whitebox])

Attachments

(1 attachment)

(Assignee)

Description

16 years ago
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...
(Assignee)

Comment 1

16 years ago
Created attachment 85759 [details] [diff] [review]
patch
(Assignee)

Updated

15 years ago
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [patch]
Target Milestone: --- → mozilla1.2alpha
Comment on attachment 85759 [details] [diff] [review]
patch

sr=bzbarsky
Attachment #85759 - Flags: superreview+
Attachment #85759 - Flags: review+
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
(Assignee)

Comment 4

15 years ago
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.
(Assignee)

Comment 5

15 years ago
Fix checked in, 2002-08-17 05:17 PDT.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED

Updated

15 years ago
Whiteboard: [patch] → [patch][whitebox]
You need to log in before you can comment on or make changes to this bug.