73645 - JS engine should use JSDHashTable for rt->gcRootsHash

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Brendan Eich [:brendan]]

per the comments in http://lxr.mozilla.org/mozilla/source/js/src/jsdhash.h#94. 
At most three words per entry are needed: keyHash, key (the root pointer), and
name (which may be null).

Same goes for rt->gcLocksHash, except key is a JSGCThing *, and there's a lock
count instead of a name member.

/be

Comment 1

[Mike Shaver (:shaver emeritus)]

Mozilla 0.9: The Milestone of Double Hashing!

Comment 2

[Mike Shaver (:shaver emeritus)]

Attachment: Use JSDHashTable for gcRootsHash and gcLocksHash

Comment 3

[Mike Shaver (:shaver emeritus)]

I haven't done extensive testing, and I'll have to write a test for the
JS_LockGCThing stuff, but it seems to work for me.

Comment 4

[Brendan Eich [:brendan]]

Every embedding I know of uses roots -- the engine uses them internally too, for
objects.  Object locks, OTOH, are not commonly used.  So why not make

+    JSDHashTable        *gcRootsHash;

be an inline (struct, not struct pointer) member, and use JS_DHashTableInit and
JS_DHashTableFinish?

Why are JSGCRootHashEntry and JSGCLockHashEntry declared/defined in jsgc.h and
jsprvtd.h?  They seem to me to be entirely jsgc.c's business.

root->root looks funny, why not name the JSGCRootHashEntry *rhe or some such, so
you have rhe->root?  Ditto for JSGCLockHashEntry *lhe instead of entry?

+                entry = (JSGCLockHashEntry *)
+                    JS_DHashTableOperate(rt->gcLocksHash, thing,
+                                         JS_DHASH_LOOKUP);
+                JS_ASSERT(entry);
+                if (entry) {
+                    JS_ASSERT(gc_lock_get_count(entry) >= 1);
+                    gc_lock_increment(entry);
                 }

There is no need to assert and null-check: JS_DHASH_LOOKUP never returns null,
and if it did, a crash on null deref is better than an assertion.

Perhaps JS_DHashTableOperate's JS_DHASH_LOOKUP switch case should test
JS_DHASH_ENTRY_IS_BUSY and return null if false.  I didn't put that test in the
jsdhash.c code, thinking it better to let callers do it, in case they could make
use of a FREE entry pointer.

But I may have been stupid: what use is a free pointer?  The caller can't claim
it, in an attempt to add an entry, because the table may become overloaded
thereby (JS_DHASH_ADD is the way to go).  Making everyone who calls Operate with
JS_DHASH_LOOKUP test JS_DHASH_ENTRY_IS_BUSY after the lookup returns, rather
than letting callers make a faster and more modular null test, now seems better
to me.  Argh.  Time for another minor jihad?  Better now than later, if this is
the thing to do.

Anyway, this isn't relevant to your patch, because the code above knows (due to
GCF_LOCK being set) that an entry exists.

-#define gc_lock_get_count(he)   ((jsrefcount)(he)->value)
-#define gc_lock_set_count(he,n) ((jsrefcount)((he)->value = (void *)(n)))
+#define gc_lock_get_count(he)   ((jsrefcount)(he)->count)
+#define gc_lock_set_count(he,n) ((jsrefcount)((he)->count = (jsrefcount)(n)))
 #define gc_lock_increment(he)   gc_lock_set_count(he, gc_lock_get_count(he)+1)
 #define gc_lock_dec...

Why not macro-expand the gc_lock_.* macros, now that you have a strong type with
a count member?  And get rid of the left-over he name.

/be

Comment 5

[Mike Shaver (:shaver emeritus)]

I'll make gcRootsHash inline.

jsapi.c needs JSGCRootHashEntry for the root dumping and mapping, and I exposed
JSGCLockHashEntry for symmetry.

I think returning NULL from lookup is a fine thing.  I'll switch my assert to
check JS_DHASH_ENTRY_IS_BUSY in the meantime.  (The old code ``knew'' that the
entry had to be present as well, but it also asserted it, and I think the
assertion is better in case the hash table and lock flags get out of synch. 
Belt and suspenders!)

How about root->address?  I think the name is part of the root
(``AddNamedRoot'', not ``AddRootAndName''), so I think the |root| variable
naming is good.  In fact, I went through the code and changed it from the
previous name of ``entry'' about half-way through my patch.  As the person who
riddled the code with (const char *)he->value, can you really complain about the
awkwardness of root->root? =)

(Question: why is the void * arg to the mapping and dumping callbacks not const?)

Comment 6

[Brendan Eich [:brendan]]

My follies are no excuse for further name hiccups ((const char *)he->value
doesn't hiccup, but I don't defend it).

If you think the *entry* pointer should be named root, why not name the
rt->gcLocksHash entry pointer "lock"?  Doesn't work, does it?  The root is the
pointer over in user-land, e.g., mScriptObject. The root hash table entry is,
well, an entry.  By capitalized-and-significant letters in the type name, its
canonical variable name should be rhe.  See lots of similar code in the JS
engine.  The function is AddNamedRoot, but that has nothing to do with the hash
table entry type name or its canonical variable nick.

(I still fail to constipate where I should.  OTOH, const void *arg would require
callbacks that want to cast to a struct pointer and update a member cast away
const.  I think I did the right thing here.  Can you say more about why you
think void *arg should be const for these callback passthru args?).

/be

Comment 7

[Mike Shaver (:shaver emeritus)]

Attachment: inline gcRootsHash, rhe/lhe naming, expanded gc_lock_*

Comment 8

[Mike Shaver (:shaver emeritus)]

Attachment: oops, botched an rhe/lhe name

Comment 9

[Brendan Eich [:brendan]]

+    JS_DHashTableInit(&rt->gcRootsHash, JS_DHashGetStubOps(), NULL,
+                      sizeof(JSGCRootHashEntry), GC_ROOTS_SIZE);
     rt->gcLocksHash = NULL;     /* create lazily */

Check for false return from Init (it allocates the initial entryStore).

+                lhe = 
+                    (JSGCLockHashEntry *)JS_DHashTableOperate(rt->gcLocksHash,
+                                                              thing,
+                                                              JS_DHASH_ADD);

Eww, why not put the cast on the same line as the = and not scrunch up the
call expression so much?  Like you do with LOOKUP below?

+                lhe = (JSGCLockHashEntry *)
+                    JS_DHashTableOperate(rt->gcLocksHash, thing,
+                                         JS_DHASH_LOOKUP);
+                JS_ASSERT(lhe);
+                if (lhe) {
+                    JS_ASSERT(lhe->count >= 1);
+                    lhe->count++;
                 }

Shaver and I chinwagged more about changing LOOKUP to return null for not found,
and it became clear that if we do that, we preclude a JS_DHashTableRawAdd or
similar function that wants LOOKUP to return a free entry to signify not found.
So let's not worry about changing LOOKUP, which means the above JS_ASSERT, to
match the old code, should assert JS_DHASH_ENTRY_IS_BUSY(lhe).

Hey, you do almost that in js_UnlockGCThing, but the if doesn't test the
complement of what's asserted:

+                lhe = (JSGCLockHashEntry *)
+                    JS_DHashTableOperate(rt->gcLocksHash, thing,
+                                         JS_DHASH_LOOKUP);
+                JS_ASSERT(JS_DHASH_ENTRY_IS_BUSY((JSDHashEntryHdr *)lhe));
+                if (lhe && --lhe->count == 0) {
+                    (void) JS_DHashTableOperate(rt->gcLocksHash, thing,
+                                                JS_DHASH_REMOVE);
                     *flagp = (uint8)(flags & ~GCF_LOCKMASK);
                 }

Nit: in js_AddRootRT you don't align the hanging-indented args with the first
arg (off by one, or else you're treating & as not part of the actual arg expr!).
Maybe add an extra space on the first line, after the cast?

+    rhe = (JSGCRootHashEntry *)JS_DHashTableOperate(&rt->gcRootsHash, rp,
+                                                     JS_DHASH_ADD);

Ultra-special js/src-style-fascism nit: make the member declarators (yes, that
includes the *'s) line up on a 1 mod 4 column (colnums start from 1).

+/* These are compatible with JSDHashEntryStub. */
+struct JSGCRootHashEntry {
+    JSDHashEntryHdr hdr;
+    const void *root;
+    const char *name;
+};
+
+struct JSGCLockHashEntry {
+    JSDHashEntryHdr hdr;
+    const JSGCThing *thing;
+    uint32 count;
+};
+

Non-nit -- why const void *root?  All that does, it seems to me, is require
ugly casts to (void*), diminishing readability; and (minor) it makes work for
you when you have to jihad thru and allow roots to be re-set by the engine,
for a copying GC(!).

/be

Comment 10

[Mike Shaver (:shaver emeritus)]

Attachment: return values checked, LOOKUP-return checks fixed, indentation repaired and JSGCRootHashEntry.root deconsted

Comment 11

[Mike Shaver (:shaver emeritus)]

Attachment: diff -wu for E-Z-Review

Comment 12

[Brendan Eich [:brendan]]

Shaver fixed two more nits (upcast when &lhe->hdr, overindentation of member
declarators in JSGC*HashEntry structs), and I sr=[e-mail]'d him via
IRC.  So who will r=?  jband?

/be

Comment 13

[Chris Waterson]

shaver and I talked about this on IRC. r=waterson

Comment 14

[Mike Shaver (:shaver emeritus)]

In the tree.

Comment 15

[Phil Schwartau]

Marking Verified -