563345 - using js::HashMap for JSRuntime::threads

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Igor Bukanov]

The attached patch replaces JSDHashTable with js::HasmMap for JSRuntime::threads. The patch also adds a special iterator to loop over all JSThreadData instances that also works in the !JS_THREADSAFE case to have less ifdef #JS_THREADSAFE in the code. Currently there is only one such case, but for the bug 516832 I will one or two extra threaddata iterations.

The patch assumes that it is safe to use the range iterator on a hashmap where the init method has failed. If this is accidental and not by design, then I will update the patch to add HashMap::isInitialized method.

Comment 1

[Igor Bukanov]

Attachment: v1

Comment 2

[Luke Wagner [:luke]]

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

Mmmm, so much simpler!  Those _destroyer/_purger/_marker callbacks have been annoying me for a while; down with inversion of control!

>+        unsigned generation = rt->threads.generation();
>         JS_UNLOCK_GC(rt);
>         thread = NewThread(id);
>         if (!thread)
>             return NULL;
>         JS_LOCK_GC(rt);
>         js_WaitForGC(rt);
>-        entry = (JSThreadsHashEntry *)
>-                JS_DHashTableOperate(&rt->threads, (const void *) id,
>-                                     JS_DHASH_ADD);
>-        if (!entry) {
>+        if (generation != rt->threads.generation()) {
>+            p = rt->threads.lookupForAdd(id);
>+
>+            /* Another thread cannot add an entry for id. */
>+            JS_ASSERT(!p);
>+        }
>+        if (!rt->threads.add(p, id, thread)) {

This review assumes a satisfactory resolution of bug 563326.

>+    for (JSThread::Map::Range i = rt->threads.all(); !i.empty(); i.popFront()) {
>+        JSThread *thread = i.front().value;
>+        JS_ASSERT(JS_CLIST_IS_EMPTY(&thread->contextList));
>+        DestroyThread(thread);
>+    }

Perhaps (here and in the other loops) 'r' (or 'e' for enums) instead of 'i'; 'i' makes me think 'iterator' and 'iterator.front' is confusing.
 
>+    for (JSThread::Map::Enum i(cx->runtime->threads);
>+         !i.empty();
>+         i.popFront()) {

This looks good, but I just noticed that Enum's constructor was not made 'explicit' in bug 515812.  Could you add that in this patch?

>+/* Default hashing policies. */

Perhaps a newline between this and the next?

>+template <class Key, size_t shift = tl::FloorLog2<sizeof(void *)>::result>

Nice

>+    static uint32 hash(const Lookup &l) {
>+        /* Hash if can implicitly cast to hash number type. */
>+        return uint32((size_t) l >> shift);
>+    }

Here you are casting l to size_t, even if l is not implicitly convertible.  For the comment to be correct, you need something more like:

    static uint32 hash(const Lookup &l) {
        /* Hash if can implicitly cast to hash number type. */
        size_t tmp = l;
        return uint32(tmp >> shift);
    }

Alternatively, several times I've wanted to add the following to jstl.h, but have been too lazy:

template <class T, class U>
static inline T
implicit_cast(const U &u)
{
  T t = u;
  return t;
}

which would allow:

       return uint32(implicit_cast<size_t>(l) >> shift);

Either way.

Comment 3

[Igor Bukanov]

Attachment: v2

The new patch updates for the changes in the proposed patch from the bug 563326. I replaced the thread id from jsword to void *. This avoids the need to have DefaultShiftHasher since the pointer specialization of DEfaultHasher would do the job of shifting out zero bits just as well. I also added initialized() method to check is the table was successfully initialized. This way js_FinishThreads does not need to rely on range enumeration working for the not initialized map.

Comment 4

[Luke Wagner [:luke]]

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

Looks good.

Comment 5

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/60c4693a80ef

Comment 6

[Igor Bukanov]

Robert Sayre - this has been backed out. What was the reason for that?

Comment 7

[Igor Bukanov]

Repeating the question since I forgot to CC Rob:
 
> this has been backed out. What was the reason for that?

Comment 8

[Robert Sayre]

(In reply to comment #7)
> Repeating the question since I forgot to CC Rob:
> 
> > this has been backed out. What was the reason for that?

Rats, I forgot to write this note.

Something regressed our tp4 nubmers, and I'm trying to find it.

Comment 9

[Robert Sayre]

here is XP Tp4 private bytes increasing by 100% or so:

http://bit.ly/9zdHmS

Comment 10

[Igor Bukanov]

(In reply to comment #9)
> here is XP Tp4 private bytes increasing by 100% or so:
> 
> http://bit.ly/9zdHmS

As far as I can see from the numbers the increase happens before the bug 563326. Yet that bug is still in the tree.

Comment 11

[Robert Sayre]

(In reply to comment #10)
> (In reply to comment #9)
> > here is XP Tp4 private bytes increasing by 100% or so:
> > 
> > http://bit.ly/9zdHmS
> 
> As far as I can see from the numbers the increase happens before the bug
> 563326. Yet that bug is still in the tree.

The numbers seem to have gone back down, so I think either backing out this bug, or bug 559408 did the trick. It's hard to tell, because the tree was burning on all debug builds when you checked this in.

Comment 12

[Igor Bukanov]

I will try to reland this than.

Comment 13

[Igor Bukanov]

http://hg.mozilla.org/tracemonkey/rev/70c78d2f7efd

Comment 14

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/70c78d2f7efd