410223 - Avoid extra allocations in WrappedNative2WrapperMap

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

See bug 399587, comment 6 where jst suggests that we store Links in the entries themselves as opposed to pointers to Links that are allocated elsewhere.

Comment 1

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix

This should do it.

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 295464 [details] [diff] [review]
Fix

>@@ -662,12 +662,25 @@ WrappedNative2WrapperMap::ClearLink(JSDH
> {
>     Entry* e = static_cast<Entry*>(entry);
>     e->key = nsnull;
>-    if(e->value)
>-    {
>-        PR_REMOVE_LINK(e->value);
>-        delete e->value;
>-        e->value = nsnull;
>-    }
>+    PR_REMOVE_LINK(&e->value);
>+    memset(e, 0, sizeof(*e));

Could use a free pattern instead of 0, and only if DEBUG -- the caller in jsdhash/pldhash is going to free the double hashtable to the malloc heap shortly.

>@@ -687,6 +700,7 @@ WrappedNative2WrapperMap::WrappedNative2
>     {
>         sOps = *JS_DHashGetStubOps();
>         sOps.clearEntry = WrappedNative2WrapperMap::ClearLink;
>+        sOps.moveEntry = WrappedNative2WrapperMap::CopyLink;
>     }

I whined to mrbkap in person about static init for sOps avoiding this code footprint micro-hit altogether; the counter-argument is that the ops struct layout might change, but that's pretty unlikely and adding more dependencies on it raises the tax threat, reducing the odds even more over time ;-).

Righteous static_cast goodness.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Updated to comments

I couldn't use a free pattern because we test that memory for null later (since entries can be reused). This switches over to static initialization. I'll check it in post-haste.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 298810 [details] [diff] [review]
Updated to comments

Oops, thought this was blocking. This patch should help reduce malloc noise and may help fix bug 410291.

Comment 5

[Mike Schroepfer]

Comment on attachment 298810 [details] [diff] [review]
Updated to comments

a+ for sure - get it in!

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

I had to back this out because it caused mochitests to crash.

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

The crashing was, in turn, caused by memory corruption because of the moveEntry hook I added. Here's a potential fix, but I don't really like the const_cast (I don't see a way around it, though, am I missing anything?).

diff --git a/js/src/xpconnect/src/xpcmaps.cpp b/js/src/xpconnect/src/xpcmaps.cpp
--- a/js/src/xpconnect/src/xpcmaps.cpp
+++ b/js/src/xpconnect/src/xpcmaps.cpp
@@ -682,15 +682,15 @@ WrappedNative2WrapperMap::CopyLink(JSDHa
                                    const JSDHashEntryHdr* from,
                                    JSDHashEntryHdr* to)
 {
-    const Entry* oldEntry = static_cast<const Entry*>(from);
+    Entry* oldEntry = const_cast<Entry*>(static_cast<const Entry*>(from));
     Entry* newEntry = static_cast<Entry*>(to);
 
     newEntry->key = oldEntry->key;
     newEntry->value = oldEntry->value;
 
     // Now update the list.
-    newEntry->value.next->prev = &newEntry->value;
-    newEntry->value.prev->next = &newEntry->value;
+    PR_INSERT_LINK(&newEntry->value, &oldEntry->value);
+    PR_REMOVE_LINK(&oldEntry->value);
 }
 
 // static

brendan: what do you think?

Comment 9

[Brendan Eich [:brendan]]

Over-used const, sorry -- file a bug?

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Now with less crashing

Here's the patch that I'll check in the next time I have time.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

I filed bug 416200 on making |from| not const in moveEntry.

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 301972 [details] [diff] [review]
Now with less crashing

>+WrappedNative2WrapperMap::MoveLink(JSDHashTable* table,
>+                                   const JSDHashEntryHdr* from,
>+                                   JSDHashEntryHdr* to)
>+{
>+    Entry* oldEntry = const_cast<Entry*>(static_cast<const Entry*>(from));

FIXME comment?

>+    Entry* newEntry = static_cast<Entry*>(to);
>+
>+    newEntry->key = oldEntry->key;
>+    newEntry->value = oldEntry->value;
>+
>+    // Now update the list.
>+    PR_INSERT_LINK(&newEntry->value, &oldEntry->value);
>+    PR_REMOVE_LINK(&oldEntry->value);

Comment detail needed? This is confusing looking, but it's correct.

/be

Comment 13

[Johnny Stenback (:jst)]

Comment on attachment 301972 [details] [diff] [review]
Now with less crashing

Blake, that only fixes part of the problem, there's a similar problem in WrappedNative2WrapperMap::Add() as well. With both fixed, I can run through mochitest w/o crashes. I'll attach a followup diff, and there's a way to fix CopyLink() w/o violating the constness of the old entry, bogus or not.

Comment 14

[Johnny Stenback (:jst)]

Attachment: Additional change needed...

Blake, this applies on top of your latest fix and fixes the problem in Add() and doesn't violate the const argument. Violating the const argument does let you write code that makes more sense, so I don't really care either way there... And this also changes the XOW code to use a better named reserved slot constant in a couple of places.

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 301972 [details] [diff] [review]
Now with less crashing

Um, shouldn't have r-'ed this patch, as it's still a good change, just missed one spot.

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 302004 [details] [diff] [review]
Additional change needed...

>diff --git a/js/src/xpconnect/src/XPCCrossOriginWrapper.cpp b/js/src/xpconnect>-  return JS_SetReservedSlot(cx, xow, XPCWrapper::sNumSlots,
>+  return JS_SetReservedSlot(cx, xow, XPC_XOW_ScopeSlot,
>                             PRIVATE_TO_JSVAL(newScope)) &&

I missed the declaration of XPC_XOW_ScopeSlot in this patch, but I believe you.

>diff --git a/js/src/xpconnect/src/xpcmaps.cpp b/js/src/xpconnect/src/xpcmaps.cpp
>     // Now update the list.
>-    PR_INSERT_LINK(&newEntry->value, &oldEntry->value);
>-    PR_REMOVE_LINK(&oldEntry->value);
>+    if (PR_CLIST_IS_EMPTY(&oldEntry->value)) {
>+        PR_INIT_CLIST(&newEntry->value);

Need to initialize newEntry->value.obj here.

Also, being in xpcmaps.cpp, need to cuddle the |if| and ( and { gets its own line.

>@@ -728,7 +732,12 @@ WrappedNative2WrapperMap::Add(WrappedNative2WrapperMap* head,
>+    if (!l->next)
>+        PR_INIT_CLIST(l);

Cuddle if and (? Also, comment here that this only happens when |this == head|.

I wasn't hitting this case in the mochitests, but it's definitely needed.

r+sr=me with those things fixed.

Comment 17

[Johnny Stenback (:jst)]

Attachment: Patch that was checked in.

Comment 18

[Johnny Stenback (:jst)]

Fix checked in.