Closed Bug 563345 Opened 10 years ago Closed 10 years ago

using js::HashMap for JSRuntime::threads

Categories

(Core :: JavaScript Engine, enhancement)

Other Branch
enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: igor, Assigned: igor)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 1 obsolete file)

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.
Attached patch v1 (obsolete) — Splinter Review
Attachment #443104 - Flags: review?(lw)
Attachment #443104 - Flags: review?(lw) → review+
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.
Attached patch v2Splinter Review
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.
Attachment #443104 - Attachment is obsolete: true
Attachment #444048 - Flags: review?(lw)
Comment on attachment 444048 [details] [diff] [review]
v2

Looks good.
Attachment #444048 - Flags: review?(lw) → review+
http://hg.mozilla.org/tracemonkey/rev/60c4693a80ef
Whiteboard: fixed-in-tracemonkey
Robert Sayre - this has been backed out. What was the reason for that?
Whiteboard: fixed-in-tracemonkey
Repeating the question since I forgot to CC Rob:
 
> this has been backed out. What was the reason for that?
(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.
here is XP Tp4 private bytes increasing by 100% or so:

http://bit.ly/9zdHmS
(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.
(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.
I will try to reland this than.
http://hg.mozilla.org/tracemonkey/rev/70c78d2f7efd
Whiteboard: fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/70c78d2f7efd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.