Patch forthcoming.
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.
Do you think you can land the "Collector" => "Heap" renaming first, separately? That should make things easier if we ever have to return to this patch and understand the behavior change.
+ // 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?
-void Collector::markMainThreadOnlyObjects() -{ -#if USE(MULTIPLE_THREADS) - ASSERT(!onMainThread()); -#endif - - // Optimization for clients that never register "main thread only" objects. - if (!mainThreadOnlyObjectCount) - return; - - // FIXME: We can optimize this marking algorithm by keeping an exact set of - // "main thread only" objects when the "main thread only" object count is - // small. We don't want to keep an exact set all the time, because WebCore - // tends to create lots of "main thread only" objects, and all that set - // thrashing can be expensive. Man, you're really hurting my job security here.... ;)
- 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?
- class SavedProperty : Noncopyable { Music to my eyes.
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.