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 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.

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 on attachment 444048 [details] [diff] [review] v2 Looks good.

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

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.