Bug 18826 - Make JavaScript heap per-thread
: Make JavaScript heap per-thread
Status: RESOLVED FIXED
: WebKit
JavaScriptCore
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2008-05-01 02:08 PST by
Modified: 2008-05-02 03:19 PST (History)


Attachments
proposed patch (368.13 KB, patch)
2008-05-01 04:17 PST, Alexey Proskuryakov
ggaren: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2008-05-01 02:08:17 PST
Patch forthcoming.
------- Comment #1 From 2008-05-01 04:17:40 PST -------
Created an attachment (id=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.
------- Comment #2 From 2008-05-01 06:12:26 PST -------
     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.
------- Comment #3 From 2008-05-01 06:24:48 PST -------
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.
------- Comment #4 From 2008-05-01 06:30:53 PST -------
+    // 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?
------- Comment #5 From 2008-05-01 06:32:15 PST -------
-  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?
------- Comment #6 From 2008-05-01 06:32:53 PST -------
-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.... ;)
------- Comment #7 From 2008-05-01 06:34:53 PST -------
-                        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?
------- Comment #8 From 2008-05-01 06:42:04 PST -------
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?
------- Comment #9 From 2008-05-01 06:46:02 PST -------
-    class SavedProperty : Noncopyable {

Music to my eyes.
------- Comment #10 From 2008-05-01 06:50:04 PST -------
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.
------- Comment #11 From 2008-05-01 06:57:34 PST -------
+        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.
------- Comment #12 From 2008-05-01 07:05:36 PST -------
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. 
------- Comment #13 From 2008-05-02 03:19:53 PST -------
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.