Bug 18826 – Make JavaScript heap per-thread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Alexey Proskuryakov]

Patch forthcoming.

[Alexey Proskuryakov]

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.

[Geoffrey Garen]

     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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

+    // 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?

[Geoffrey Garen]

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

[Geoffrey Garen]

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

[Geoffrey Garen]

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

[Geoffrey Garen]

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?

[Geoffrey Garen]

-    class SavedProperty : Noncopyable {

Music to my eyes.

[Geoffrey Garen]

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.

[Geoffrey Garen]

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

[Geoffrey Garen]

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. 

[Alexey Proskuryakov]

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.