Created attachment 20911[details]
proposed patch
A couple of known loose ends:
- Collector renaming is not finished, will rename the file and remaining structures in a follow-up patch.
- When a thread dies, uncollected objects will leak. Still need to think about how to fix this right.

JSLock lock;
- if (!Collector::isBusy())
- Collector::collect();
+
+ ExecState* exec = toJS(ctx);
+ if (!exec->heap()->isBusy())
+ exec->heap()->collect();
We need to support a NULL ctx passed to JSGarbageCollect, for the case when you want to garbage collect after releasing the last reference to a JSGlobalContextRef. So, I think JSGarbageCollect should check for a NULL ctx, and do the slower thing. You should probably update the header documentation to that effect, too.
Still reading the rest of the patch.

+ // FIXME: should assert that the result equals threadHeap(), but currently, this fails as database code uses gcUnprotect from a different thread.
+ // That's a race condition and should be fixed.
Yikes! Double yikes!
I'd really like to have a working version of this ASSERT. Is it possible to land a fix for the database bug before landing this patch?

- ProtectCountSet& protectedValues = KJS::protectedValues();
- ProtectCountSet::iterator end = protectedValues.end();
- for (ProtectCountSet::iterator it = protectedValues.begin(); it != end; ++it) {
+ HashCountedSet<JSCell*>::iterator end = protectedValues.end();
+ for (HashCountedSet<JSCell*>::iterator it = protectedValues.begin(); it != end; ++it) {
Is it really a win to remove the use of the ProtectCountSet typedef?

- ASSERT(currentThreadIsMainThread || !curBlock->collectOnMainThreadOnly.get(i));
Would it be worthwhile to ASSERT inside Heap::allocate and Heap::collect that the heap doing the work was the current thread's heap?

Suppose I make object o1 on thread 1, and object o2 on thread 2, and then I set o1 to be a property of o2. When thread 2 collects, it will mark o1 but, because o1 is not in thread 2's heap, thread 2 will not unmark o1 (or its references) at the end of GC.
Is that OK? I believe it's not, since the next collect on thread 1 will fail to trace o1's references, since o1 will seem already marked.
So, I believe it is officially an error to set properties across threads. Would it be worthwhile to add thread ASSERTS to JSObject::get and JSObject::put?

Another thing that would help future readers of this patch: You could land the change to pass ExecState* in more places first, without using the ExecState for anything. You could even land operator new(ExecState*), and just have it ignore its ExecState* at first.

+ Removed GCController::garbageCollectOnAlternateThreadForDebugging(), because heaps are now
+ per-thread, and only the owner thread can access a heap.
I don't agree with this change. garbageCollectOnAlternateThreadForDebugging is a mechanism we use to *verify* that heaps are (successfully) per-thread, and that GC only touches per-thread data. I think we want garbageCollectOnAlternateThreadForDebugging to stay in, and to collect by calling Heap::threadHeap()->collect(). It's a lot less interesting than it used to be, but it's still interesting.

So, in summary:
Refactorings that would be very helpful to land before the behavior change: (1) Rename "Collector" to "Heap"; (2) Pass ExecState* everyhwere; (3) Fix the Database gcProtect bug. I'll leave these up to your judgement.
Things you must address before landing: (1) Handle NULL ctx passed to JSGarbageCollect; (2) Please leave in garbageCollectOnAlternateThreadForDebugging.

Committed revision 32807.
(In reply to comment #12)
> Refactorings that would be very helpful to land before the behavior change: (1)
> Rename "Collector" to "Heap"; (2) Pass ExecState* everyhwere;
I'm not sure that (1) adds a lot of noise - even if the renaming were done first, all calls to static Collector methods would still need to be changed to Heap::threadHeap() etc.
I agree that factoring out (2) would be useful, since it's a dangerous change (changing "new" to "new (exec)" for a non-JS class is easy to overlook, but very dangerous). But it's also hard to decouple from other changes, and error-prone, so I didn't do that.
> (3) Fix the Database gcProtect bug.
This one might involve quite a bit of Database refactoring.
> Things you must address before landing: (1) Handle NULL ctx passed to
> JSGarbageCollect; (2) Please leave in
> garbageCollectOnAlternateThreadForDebugging.
Done. Also, added assertions suggested in comment 7 and comment 8.
> Is it really a win to remove the use of the ProtectCountSet typedef?
That's debatable, but to me, it's easier to read code when the actual type is known - especially since in this case, it's not all that much longer.