Changeset 20004 in webkit for trunk/JavaScriptCore/kjs/collector.cpp

Timestamp:

Mar 6, 2007, 8:25:49 PM (18 years ago)

Author:

ggaren

Message:

JavaScriptCore:

Reviewed by Maciej Stachowiak.

Fixed all known crashers exposed by run-webkit-tests --threaded. This covers:

<rdar://problem/4565394> | ​http://bugs.webkit.org/show_bug.cgi?id=12585

PAC file: after closing a window that contains macworld.com, new window
crashes (KJS::PropertyMap::mark()) (12585)

<rdar://problem/4571215> | ​http://bugs.webkit.org/show_bug.cgi?id=9211

PAC file: Crash occurs when clicking on the navigation tabs at ​http://www.businessweek.com/ (9211)

<rdar://problem/4557926>

PAC file: Crash occurs when attempting to view image in slideshow mode
at ​http://d.smugmug.com/gallery/581716 ( KJS::IfNode::execute (KJS::
ExecState*) + 312) if you use a PAC file

(1) Added some missing JSLocks, along with related ASSERTs.

(2) Fully implemented support for objects that can only be garbage collected
on the main thread. So far, only WebCore uses this. We can add it to API
later if we learn that it's needed.

The implementation uses a "main thread only" flag inside each object. When
collecting on a secondary thread, the Collector does an extra pass through
the heap to mark all flagged objects before sweeping. This solution makes
the common case -- flag lots of objects, but never collect on a secondary
thread -- very fast, even though the uncommon case of garbage collecting
on a secondary thread isn't as fast as it could be. I left some notes
about how to speed it up, if we ever care.

For posterity, here are some things I learned about GC while investigating:

• Each collect must either mark or delete every heap object. "Zombie" objects, which are neither marked nor deleted, raise these issues:

• On the next pass, the conservative marking algorithm might mark a zombie, causing it to mark freed objects.

• The client might try to use a zombie, which would seem live because its finalizer had not yet run.

• A collect on the main thread is free to delete any object. Presumably, objects allocated on secondary threads have thread-safe finalizers.

• A collect on a secondary thread must not delete thread-unsafe objects.

• The mark function must be thread-safe.

Line by line comments:

• API/JSObjectRef.h: Added comment specifying that the finalize callback may run on any thread.

• JavaScriptCore.exp: Nothing to see here.

• bindings/npruntime.cpp: (_NPN_GetStringIdentifier): Added JSLock.

• bindings/objc/objc_instance.h:
• bindings/objc/objc_instance.mm: (ObjcInstance::~ObjcInstance): Use an autorelease pool. The other callers to CFRelease needed one, too, but they were dead code, so I removed them instead. (This fixes a leak seen while running run-webkit-tests --threaded, although I don't think it's specifically a threading issue.)

• kjs/collector.cpp: (KJS::Collector::collectOnMainThreadOnly): New function. Tells the collector to collect a value only if it's collecting on the main thread. (KJS::Collector::markMainThreadOnlyObjects): New function. Scans the heap for "main thread only" objects and marks them.

• kjs/date_object.cpp: (KJS::DateObjectImp::DateObjectImp): To make the new ASSERTs happy, allocate our globals on the heap, avoiding a seemingly unsafe destructor call at program exit time.
• kjs/function_object.cpp: (FunctionPrototype::FunctionPrototype): ditto

• kjs/interpreter.cpp: (KJS::Interpreter::mark): Removed boolean parameter, which was an incomplete and arguably hackish way to implement markMainThreadOnlyObjects() inside WebCore.
• kjs/interpreter.h:

• kjs/identifier.cpp: (KJS::identifierTable): Added some ASSERTs to check for thread safety problems.

• kjs/list.cpp: Added some ASSERTs to check for thread safety problems. (KJS::allocateListImp): (KJS::List::release): (KJS::List::append): (KJS::List::empty): Make the new ASSERTs happy.

• kjs/object.h: (KJS::JSObject::JSObject): "m_destructorIsThreadSafe" => "m_collectOnMainThreadOnly". I removed the constructor parameter because m_collectOnMainThreadOnly, like m_marked, is a Collector bit, so only the Collector should set or get it.

• kjs/object_object.cpp: (ObjectPrototype::ObjectPrototype): Make the ASSERTs happy.
• kjs/regexp_object.cpp: (RegExpPrototype::RegExpPrototype): ditto

• kjs/ustring.cpp: Added some ASSERTs to check for thread safety problems. (KJS::UCharReference::ref): (KJS::UString::Rep::createCopying): (KJS::UString::Rep::create): (KJS::UString::Rep::destroy): (KJS::UString::null): Make the new ASSERTs happy.
• kjs/ustring.h: (KJS::UString::Rep::ref): Added some ASSERTs to check for thread safety problems. (KJS::UString::Rep::deref):

• kjs/value.h: (KJS::JSCell::JSCell):

JavaScriptGlue:

Reviewed by Maciej Stachowiak.

Fixed all known crashers exposed by run-webkit-tests --threaded while using
a PAC file (for maximum carnage). See JavaScriptCore ChangeLog for
more details.

• JSBase.cpp: (JSBase::Release): Lock when deleting, because we may be deleting an object (like a JSRun) that holds thread-unsafe data.

• JSUtils.cpp: (CFStringToUString): Don't lock, because our caller locks. Also, locking inside a function that returns thread-unsafe data by copy will only mask threading problems.

• JavaScriptGlue.cpp: (JSRunEvaluate): Added missing JSLock. (JSRunCheckSyntax): Converted to JSLock.
• JavaScriptGlue.xcodeproj/project.pbxproj:

WebCore:

Reviewed by Maciej Stachowiak.

Fixed all known crashers exposed by run-webkit-tests --threaded [*]. See
JavaScriptCore ChangeLog for more details.

• bindings/js/kjs_binding.cpp: (KJS::domNodesPerDocument): Added thread safety ASSERT. (KJS::ScriptInterpreter::mark): Removed obsolete logic for marking unsafe objects when collecting on a secondary thread. The Collector takes care of this now.

• bindings/js/kjs_binding.h: (KJS::DOMObject::DOMObject): Used new API for specifying that WebCore objects should be garbage collected on the main thread only.

• bindings/js/kjs_window.cpp: (KJS::ScheduledAction::execute): Moved JSLock to cover implementedsCall() call, which, for some subclasses, ends up allocating garbage collected objects. (This fix was speculative. I didn't actually see a crash from this.) (KJS::Window::timerFired): Added JSLock around ScheduleAction destruction, since it destroys a KJS::List.

• bindings/objc/WebScriptObject.mm: (-[WebScriptObject setException:]): Added JSLock. (This fix was speculative. I didn't actually see a crash from this.)

• bridge/mac/WebCoreScriptDebugger.mm: (-[WebCoreScriptCallFrame evaluateWebScript:]): Added JSLock. (This fix was speculative. I didn't actually see a crash from this.)

• dom/Document.cpp: (WebCore::Document::~Document): Added JSLock around modification to domNodesPerDocument(), which can be accessed concurrently during garbage collection.
• dom/Node.cpp: (WebCore::Node::setDocument): ditto.

[*] fast/js/toString-stack-overflow.html is an exception. --threaded mode
crashes this test because it causes the garbage collector to run frequently,
and this test crashes if you happen to garbage collect while it's running.
This is a known issue with stack overflow during the mark phase. It's
not related to threading.

File:

• trunk/JavaScriptCore/kjs/collector.cpp (modified) (9 diffs)

trunk/JavaScriptCore/kjs/collector.cpp

@@ -109 +109 @@
 static CollectorHeap heap = {NULL, 0, 0, 0, NULL, 0, 0, 0, 0};
 
+size_t Collector::mainThreadOnlyObjectCount = 0;
 bool Collector::memoryFull = false;
 
@@ -474 +475 @@
 }
 
+void Collector::collectOnMainThreadOnly(JSValue* value)
+{
+    ASSERT(value);
+    ASSERT(JSLock::lockCount() > 0);
+
+    if (JSImmediate::isImmediate(value))
+      return;
+
+    JSCell* cell = value->downcast();
+    cell->m_collectOnMainThreadOnly = true;
+    ++mainThreadOnlyObjectCount;
+}
+
 void Collector::markProtectedObjects()
 {
@@ -485 +499 @@
 }
 
+void Collector::markMainThreadOnlyObjects()
+{
+    ASSERT(!pthread_main_np());
+
+    // 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.
+    
+    size_t count = 0;
+    
+    for (size_t block = 0; block < heap.usedBlocks; block++) {
+        ASSERT(count < mainThreadOnlyObjectCount);
+        
+        CollectorBlock* curBlock = heap.blocks[block];
+        size_t minimumCellsToProcess = curBlock->usedCells;
+        for (size_t i = 0; (i < minimumCellsToProcess) & (i < CELLS_PER_BLOCK); i++) {
+            CollectorCell* cell = curBlock->cells + i;
+            if (cell->u.freeCell.zeroIfFree == 0)
+                ++minimumCellsToProcess;
+            else {
+                JSCell* imp = reinterpret_cast<JSCell*>(cell);
+                if (imp->m_collectOnMainThreadOnly) {
+                    if(!imp->marked())
+                        imp->mark();
+                    if (++count == mainThreadOnlyObjectCount)
+                        return;
+                }
+            }
+        }
+    }
+
+    for (size_t cell = 0; cell < heap.usedOversizeCells; cell++) {
+        ASSERT(count < mainThreadOnlyObjectCount);
+
+        JSCell* imp = reinterpret_cast<JSCell*>(heap.oversizeCells[cell]);
+        if (imp->m_collectOnMainThreadOnly) {
+            if (!imp->marked())
+                imp->mark();
+            if (++count == mainThreadOnlyObjectCount)
+                return;
+        }
+    }
+}
+
 bool Collector::collect()
 {
@@ -502 +566 @@
     Interpreter* scr = Interpreter::s_hook;
     do {
-      scr->mark(currentThreadIsMainThread);
+      scr->mark();
       scr = scr->next;
     } while (scr != Interpreter::s_hook);
@@ -512 +576 @@
   markProtectedObjects();
   List::markProtectedLists();
+  if (!currentThreadIsMainThread)
+    markMainThreadOnlyObjects();
 
   // SWEEP: delete everything with a zero refcount (garbage) and unmark everything else
@@ -531 +597 @@
         if (imp->m_marked) {
           imp->m_marked = false;
-        } else if (currentThreadIsMainThread || imp->m_destructorIsThreadSafe) {
+        } else {
           // special case for allocated but uninitialized object
-          // (We don't need this check earlier because nothing prior this point assumes the object has a valid vptr.)
+          // (We don't need this check earlier because nothing prior this point 
+          // assumes the object has a valid vptr.)
           if (cell->u.freeCell.zeroIfFree == 0)
             continue;
 
+          ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+          if (imp->m_collectOnMainThreadOnly)
+            --mainThreadOnlyObjectCount;
           imp->~JSCell();
           --usedCells;
@@ -557 +627 @@
           if (imp->m_marked) {
             imp->m_marked = false;
-          } else if (currentThreadIsMainThread || imp->m_destructorIsThreadSafe) {
+          } else {
+            ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+            if (imp->m_collectOnMainThreadOnly)
+              --mainThreadOnlyObjectCount;
             imp->~JSCell();
             --usedCells;
@@ -600 +673 @@
     JSCell *imp = (JSCell *)heap.oversizeCells[cell];
     
-    if (!imp->m_marked && (currentThreadIsMainThread || imp->m_destructorIsThreadSafe)) {
+    if (imp->m_marked) {
+      imp->m_marked = false;
+      cell++;
+    } else {
+      ASSERT(currentThreadIsMainThread || !imp->m_collectOnMainThreadOnly);
+      if (imp->m_collectOnMainThreadOnly)
+        --mainThreadOnlyObjectCount;
       imp->~JSCell();
 #if DEBUG_COLLECTOR
@@ -618 +697 @@
         heap.oversizeCells = (CollectorCell **)fastRealloc(heap.oversizeCells, heap.numOversizeCells * sizeof(CollectorCell *));
       }
-    } else {
-      imp->m_marked = false;
-      cell++;
     }
   }