389832 - ActionMonkey: Remove GCF_LOCK flag, Eager init gcLocksHash

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

This is another small chunk of GC-related work that can be done before making the leap to MMgc allocation.

Remove the GCF_LOCK flag from MMgc entirely.  "Locked" objects are already stored in a hash table; simply modify jsgc.cpp to make sure all those objects are marked from js_TraceRuntime.  Mark the empty string and special jsdoubles, too.

Comment 1

[Jason Orendorff [:jorendorff]]

Sorry, I meant "Remove the GCF_LOCK flag from *SpiderMonkey* entirely" of course, not MMgc.

Ed, can you take a look at this?  You will also have to modify js_LockGCThingRT(), where it says:

>    /*
>     * Avoid adding a rt->gcLocksHash entry for shallow things until someone
>     * nests a lock -- then start such an entry with a count of 2, not 1.
>     */
>    if (lock || deep) {

This can no longer be avoided; the body of the if statement must happen unconditionally (bug 389420 comment 22).

Don't waste a lot of time on it if it's not the right thing to do.  It just occurred to me this is possible.

Comment 2

[Jason Orendorff [:jorendorff]]

(In reply to comment #0)
> [...]  "Locked" objects are already
> stored in a hash table; simply modify jsgc.cpp to make sure all those objects
> are marked from js_TraceRuntime. [...]

Oops.  That's a no-op -- js_TraceRuntime already does this.

>    if (rt->gcLocksHash)
>        JS_DHashTableEnumerate(rt->gcLocksHash, gc_lock_traversal, trc);

Comment 3

[Ed Lee :Mardak]

Attachment: v1

- Removed GCF_LOCK from jsgc, jsnum, jsstr.
- Simplified js_(Lock|Unlock)GCThingRT to always add to the hash table (don't check if it's shallow)
-- this also means various sanity checks for singly-locked shallow items aren't needed
- Manually mark the constants from js_TraceRuntime for now, this will be removed when switching to MMgc (check null because they can disappear before the last GC)

Comment 4

[Igor Bukanov]

Comment on attachment 274212 [details] [diff] [review]
v1

> void
> js_TraceRuntime(JSTracer *trc, JSBool allAtoms)
> {
>     JSRuntime *rt = trc->context->runtime;
>     JSContext *iter, *acx;
> 
>+    /*
>+     * Temporary hack to manually mark constants that are pointed by the
>+     * JSRuntime. Eventually when switching to MMgc, the JSRuntime will
>+     * automatically be scanned and mark these constants.
>+     */
>+    if (rt->jsNaN)
>+        *js_GetGCThingFlags(rt->jsNaN) |= GCF_MARK;
>+    if (rt->jsNegativeInfinity)
>+        *js_GetGCThingFlags(rt->jsNegativeInfinity) |= GCF_MARK;
>+    if (rt->jsPositiveInfinity)
>+        *js_GetGCThingFlags(rt->jsPositiveInfinity) |= GCF_MARK;
>+    if (rt->emptyString)
>+        *js_GetGCThingFlags(rt->emptyString) |= GCF_MARK;
>+

This is wrong. Use JS_TRACE_DOUBLE/JS_TRACE_STRING here as tracing can happen without any GC activity.

>diff -r f3f3535e5b4e js/src/jsnum.cpp
>--- a/js/src/jsstr.cpp	Thu Jul 26 17:24:46 2007 -0400
>+++ b/js/src/jsstr.cpp	Fri Jul 27 14:30:52 2007 -0700
>@@ -2349,17 +2349,17 @@ js_InitRuntimeStringState(JSContext *cx)
>     JS_ASSERT(!rt->deflatedStringCacheLock);
>     rt->deflatedStringCacheLock = JS_NEW_LOCK();
>     if (!rt->deflatedStringCacheLock)
>         return JS_FALSE;
> #endif
> 
>     /* Make a permanently locked empty string. */
>     JS_ASSERT(!rt->emptyString);
>-    empty = js_NewStringCopyN(cx, js_empty_ucstr, 0, GCF_LOCK);
>+    empty = js_NewStringCopyN(cx, js_empty_ucstr, 0, 0);
>     if (!empty)
>         goto bad;

This collides with my patch for bug 389880. Hopefully hg will do the right thing here.

Comment 5

[Igor Bukanov]

Comment on attachment 274212 [details] [diff] [review]
v1

> void
> js_TraceRuntime(JSTracer *trc, JSBool allAtoms)
> {
...
>+    if (rt->emptyString)
>+        *js_GetGCThingFlags(rt->emptyString) |= GCF_MARK;
>+

This is not necessary. rt->emptyString is marked when pinned atoms are marked in jsatom.c. Thus only the numbers should be traced.

Comment 6

[Ed Lee :Mardak]

Attachment: v2

(In reply to comment #4)
> >+    if (rt->jsNaN)
> >+        *js_GetGCThingFlags(rt->jsNaN) |= GCF_MARK;
> This is wrong. Use JS_TRACE_DOUBLE/JS_TRACE_STRING here as tracing can happen
> without any GC activity.
Fixed by using JS_CALL_DOUBLE_TRACER and JS_CALL_STRING_TRACER.

(In reply to comment #5)
> >+    if (rt->emptyString)
> >+        *js_GetGCThingFlags(rt->emptyString) |= GCF_MARK;
> This is not necessary. rt->emptyString is marked when pinned atoms are marked
> in jsatom.c. Thus only the numbers should be traced.
Removed.

Comment 7

[Jason Orendorff [:jorendorff]]

> JSBool
> js_UnlockGCThingRT(JSRuntime *rt, void *thing)

It looks like with v1, rt->gcPoke is set unconditionally in js_UnlockGCThingRT().  Before, the poke was skipped if the lock count was still >0 after decrementing it.

Comment 8

[Igor Bukanov]

Comment on attachment 274218 [details] [diff] [review]
v2

> JSBool
> js_UnlockGCThingRT(JSRuntime *rt, void *thing)
> {
>-    uint8 *flagp, flags;
>     JSGCLockHashEntry *lhe;
> 
>     if (!thing)
>         return JS_TRUE;
> 
>-    flagp = js_GetGCThingFlags(thing);
>     JS_LOCK_GC(rt);
>-    flags = *flagp;
>-
>-    if (flags & GCF_LOCK) {
>-        if (!rt->gcLocksHash ||
>-            (lhe = (JSGCLockHashEntry *)
>-                   JS_DHashTableOperate(rt->gcLocksHash, thing,
>-                                        JS_DHASH_LOOKUP),
>-             JS_DHASH_ENTRY_IS_FREE(&lhe->hdr))) {
>-            /* Shallow GC-thing with an implicit lock count of 1. */
>-            JS_ASSERT(!GC_THING_IS_DEEP(flags & GCF_TYPEMASK, thing));
>-        } else {
>-            /* Basis or nested unlock of a deep thing, or nested of shallow. */
>-            if (--lhe->count != 0)
>-                goto out;
>+
>+    if (rt->gcLocksHash) {
>+        lhe = (JSGCLockHashEntry *) JS_DHashTableOperate(rt->gcLocksHash, thing,
>+                                                         JS_DHASH_LOOKUP);
>+        if (--lhe->count == 0)

The current version of js_UnlockGCThingRT allows to pass unlocked thing and does nothing in this case. To preserve the behavior for API compatibility it is necessary to check for non-null lhe here. r+ with this addressed.

Comment 9

[Ed Lee :Mardak]

Comment on attachment 274218 [details] [diff] [review]
v2

(In reply to comment #7)
> It looks like with v1, rt->gcPoke is set unconditionally in
> js_UnlockGCThingRT().  Before, the poke was skipped if the lock count was still
> >0 after decrementing it.
Ah right. I moved it out earlier when I had it inside the first if..

Poke if..
1) LOCK is set but wasn't put in the table (1 lock)
2) in the table but now removed

I might as well just *not* lazily init gcLocksHash in the patch.

Comment 10

[Igor Bukanov]

(In reply to comment #8)
> The current version of js_UnlockGCThingRT allows to pass unlocked thing and
> does nothing in this case. To preserve the behavior for API compatibility it is
> necessary to check for non-null lhe here. r+ with this addressed. 

Of cause the check should not do lhe !+ null but rather those JS_DHASH_ENTRY_IS_FREE checks.

Comment 11

[Igor Bukanov]

(In reply to comment #9)
> I might as well just *not* lazily init gcLocksHash in the patch.

That is a nice idea.

Comment 12

[Ed Lee :Mardak]

Attachment: v3

(In reply to comment #7)
> Before, the poke was skipped if the lock count was still 
> >0 after decrementing it.
Poked only on removal.

(In reply to comment #11)
> > I might as well just *not* lazily init gcLocksHash in the patch.
> That is a nice idea.
Un-lazy-ified. Saves on some null checks for every lock/unlock. (And no more gotos!)

(In reply to comment #10)
> > The current version of js_UnlockGCThingRT allows to pass unlocked thing and
> > does nothing in this case.
> Of course the check should not do lhe != null but rather those
> JS_DHASH_ENTRY_IS_FREE checks.
Checked.

brendan: Now that gcLocksHash isn't lazily inited and will hold all locked items (minus constants), it'll grow in size faster than before.
1) JSRuntime has 2 of these DHashTable structs near the beginning (instead of 1 struct, 1 pointer)
2) we're reusing #define GC_ROOTS_SIZE   256 for initial capacity for locks

<ibukanov>	Then lets move both hashes towards the end of JSRuntime.
<ibukanov>	This is to avoid code bloat with 2 bytes offset for more frequently accessed fields.
<ibukanov>	But OK, I can file a separated bug about it.
<ibukanov>	As it would help CVS tip as well.

Both those issues aren't critical right now for the current stage, but something to note for later.

jorendorff: With gcLocksHash not lazily created, it'll be Alloced as part of GCRoot, so it'll save a step later on. We'll still need to modify the Ops to not use the StubOps and have them GC::Alloc instead.

r? again for making not lazy.

Comment 13

[Igor Bukanov]

I filed bug 389907 against SpiderMonkey to optimize JSRuntime layout.

Comment 14

[Jason Orendorff [:jorendorff]]

(In reply to comment #12)
> jorendorff: With gcLocksHash not lazily created, it'll be Alloced as part of
> GCRoot, so it'll save a step later on. We'll still need to modify the Ops to
> not use the StubOps and have them GC::Alloc instead.

I said this a while ago, but we don't *have* to change this.  It wouldn't save code, and it wouldn't run faster.

Comment 15

[Ed Lee :Mardak]

pushed to hg/actionmonkey
remote: added 1 changesets with 5 changes to 5 files
changeset 3736:  	1d81f62e7970