Objective-C API external object graphs don't handle generational collection properly
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Apr 2014 21:05:09 +0000 (21:05 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 15 Apr 2014 21:05:09 +0000 (21:05 +0000)
https://bugs.webkit.org/show_bug.cgi?id=131634

Reviewed by Geoffrey Garen.

If the set of Objective-C objects transitively reachable through an object changes, we
need to update the set of opaque roots accordingly. If we don't, the next EdenCollection
won't rescan the external object graph, which would lead us to consider a newly allocated
JSManagedValue to be dead.

* API/JSBase.cpp:
(JSSynchronousEdenCollectForDebugging):
* API/JSVirtualMachine.mm:
(-[JSVirtualMachine initWithContextGroupRef:]):
(-[JSVirtualMachine dealloc]):
(-[JSVirtualMachine isOldExternalObject:]):
(-[JSVirtualMachine addExternalRememberedObject:]):
(-[JSVirtualMachine addManagedReference:withOwner:]):
(-[JSVirtualMachine removeManagedReference:withOwner:]):
(-[JSVirtualMachine externalRememberedSet]):
(scanExternalObjectGraph):
(scanExternalRememberedSet):
* API/JSVirtualMachineInternal.h:
* API/tests/testapi.mm:
* heap/Heap.cpp:
(JSC::Heap::markRoots):
* heap/Heap.h:
(JSC::Heap::slotVisitor):
* heap/SlotVisitor.h:
* heap/SlotVisitorInlines.h:
(JSC::SlotVisitor::containsOpaqueRoot):
(JSC::SlotVisitor::containsOpaqueRootTriState):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@167326 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/API/JSBase.cpp
Source/JavaScriptCore/API/JSVirtualMachine.mm
Source/JavaScriptCore/API/JSVirtualMachineInternal.h
Source/JavaScriptCore/API/tests/testapi.mm
Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/heap/Heap.cpp
Source/JavaScriptCore/heap/Heap.h
Source/JavaScriptCore/heap/SlotVisitor.h
Source/JavaScriptCore/heap/SlotVisitorInlines.h

index ff293a7e459b1fde6221a04056be8c2d0b7ac32c..31bdf2b94d487a3d13a2ef3344f0f01447b25778 100644 (file)
@@ -142,6 +142,7 @@ void JSReportExtraMemoryCost(JSContextRef ctx, size_t size)
 }
 
 extern "C" JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+extern "C" JS_EXPORT void JSSynchronousEdenCollectForDebugging(JSContextRef);
 
 void JSSynchronousGarbageCollectForDebugging(JSContextRef ctx)
 {
@@ -153,6 +154,16 @@ void JSSynchronousGarbageCollectForDebugging(JSContextRef ctx)
     exec->vm().heap.collectAllGarbage();
 }
 
+void JSSynchronousEdenCollectForDebugging(JSContextRef ctx)
+{
+    if (!ctx)
+        return;
+
+    ExecState* exec = toJS(ctx);
+    JSLockHolder locker(exec);
+    exec->vm().heap.collect(EdenCollection);
+}
+
 void JSDisableGCTimer(void)
 {
     GCActivityCallback::s_shouldCreateGCTimer = false;
index 8853441335d1fe342771971bb073d5d82e03d065..26e709ae16e7b5fe84f39b31c441534a5d79003c 100644 (file)
@@ -87,6 +87,7 @@ static NSMapTable *wrapperCache()
     JSContextGroupRef m_group;
     NSMapTable *m_contextCache;
     NSMapTable *m_externalObjectGraph;
+    NSMapTable *m_externalRememberedSet;
 }
 
 - (instancetype)init
@@ -113,6 +114,9 @@ static NSMapTable *wrapperCache()
     NSPointerFunctionsOptions weakIDOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
     NSPointerFunctionsOptions strongIDOptions = NSPointerFunctionsStrongMemory | NSPointerFunctionsObjectPersonality;
     m_externalObjectGraph = [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:strongIDOptions capacity:0];
+
+    NSPointerFunctionsOptions integerOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsIntegerPersonality;
+    m_externalRememberedSet = [[NSMapTable alloc] initWithKeyOptions:weakIDOptions valueOptions:integerOptions capacity:0];
    
     [JSVMWrapperCache addWrapper:self forJSContextGroupRef:group];
  
@@ -124,6 +128,7 @@ static NSMapTable *wrapperCache()
     JSContextGroupRelease(m_group);
     [m_contextCache release];
     [m_externalObjectGraph release];
+    [m_externalRememberedSet release];
     [super dealloc];
 }
 
@@ -145,6 +150,18 @@ static id getInternalObjcObject(id object)
     return object;
 }
 
+- (bool)isOldExternalObject:(id)object
+{
+    JSC::VM* vm = toJS(m_group);
+    return vm->heap.slotVisitor().containsOpaqueRoot(object);
+}
+
+- (void)addExternalRememberedObject:(id)object
+{
+    ASSERT([self isOldExternalObject:object]);
+    [m_externalRememberedSet setObject:[NSNumber numberWithBool:true] forKey:object];
+}
+
 - (void)addManagedReference:(id)object withOwner:(id)owner
 {    
     if ([object isKindOfClass:[JSManagedValue class]])
@@ -157,7 +174,9 @@ static id getInternalObjcObject(id object)
         return;
     
     JSC::JSLockHolder locker(toJS(m_group));
-    
+    if ([self isOldExternalObject:owner] && ![self isOldExternalObject:object])
+        [self addExternalRememberedObject:owner];
     NSMapTable *ownedObjects = [m_externalObjectGraph objectForKey:owner];
     if (!ownedObjects) {
         NSPointerFunctionsOptions weakIDOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
@@ -198,8 +217,10 @@ static id getInternalObjcObject(id object)
     if (count == 1)
         NSMapRemove(ownedObjects, object);
 
-    if (![ownedObjects count])
+    if (![ownedObjects count]) {
         [m_externalObjectGraph removeObjectForKey:owner];
+        [m_externalRememberedSet removeObjectForKey:owner];
+    }
 }
 
 @end
@@ -234,6 +255,11 @@ JSContextGroupRef getGroupFromVirtualMachine(JSVirtualMachine *virtualMachine)
     return m_externalObjectGraph;
 }
 
+- (NSMapTable *)externalRememberedSet
+{
+    return m_externalRememberedSet;
+}
+
 @end
 
 void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root)
@@ -253,13 +279,27 @@ void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root)
             visitor.addOpaqueRoot(nextRoot);
             
             NSMapTable *ownedObjects = [externalObjectGraph objectForKey:static_cast<id>(nextRoot)];
-            id ownedObject;
-            NSEnumerator *enumerator = [ownedObjects keyEnumerator];
-            while ((ownedObject = [enumerator nextObject]))
+            for (id ownedObject in ownedObjects)
                 stack.append(static_cast<void*>(ownedObject));
         }
     }
 }
 
-#endif
+void scanExternalRememberedSet(JSC::VM& vm, JSC::SlotVisitor& visitor)
+{
+    @autoreleasepool {
+        JSVirtualMachine *virtualMachine = [JSVMWrapperCache wrapperForJSContextGroupRef:toRef(&vm)];
+        if (!virtualMachine)
+            return;
+        NSMapTable *externalObjectGraph = [virtualMachine externalObjectGraph];
+        NSMapTable *externalRememberedSet = [virtualMachine externalRememberedSet];
+        for (id key in externalRememberedSet) {
+            NSMapTable *ownedObjects = [externalObjectGraph objectForKey:key];
+            for (id ownedObject in ownedObjects)
+                scanExternalObjectGraph(vm, visitor, ownedObject);
+        }
+        [externalRememberedSet removeAllObjects];
+    }
+}
 
+#endif // JSC_OBJC_API_ENABLED
index 7292265664c35d590a8207d02e696b340f5b4f99..009d8e4e5d4b560866704569d67773584be68cbf 100644 (file)
 #ifndef JSVirtualMachineInternal_h
 #define JSVirtualMachineInternal_h
 
-#import <JavaScriptCore/JavaScriptCore.h>
-
 #if JSC_OBJC_API_ENABLED
 
+#import <JavaScriptCore/JavaScriptCore.h>
+
 namespace JSC {
 class VM;
 class SlotVisitor;
@@ -51,7 +51,8 @@ JSContextGroupRef getGroupFromVirtualMachine(JSVirtualMachine *);
 #endif // defined(__OBJC__)
 
 void scanExternalObjectGraph(JSC::VM&, JSC::SlotVisitor&, void* root);
+void scanExternalRememberedSet(JSC::VM&, JSC::SlotVisitor&);
 
-#endif
+#endif // JSC_OBJC_API_ENABLED
 
 #endif // JSVirtualMachineInternal_h
index bc2d099dd74fff0cb6beca5f3f2c5097c96b1eb0..c528e691ee326b8a269eec79d6905da49afd5122 100644 (file)
@@ -30,6 +30,7 @@
 #import "JSExportTests.h"
 
 extern "C" void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+extern "C" void JSSynchronousEdenCollectForDebugging(JSContextRef);
 
 extern "C" bool _Block_has_signature(id);
 extern "C" const char * _Block_signature(id);
@@ -1326,6 +1327,38 @@ void testObjectiveCAPI()
         [[JSValue valueWithInt32:42 inContext:context] toArray];
     }
 
+    @autoreleasepool {
+        JSContext *context = [[JSContext alloc] init];
+
+        // Create the root, make it reachable from JS, and force an EdenCollection
+        // so that we scan the external object graph.
+        TestObject *root = [TestObject testObject];
+        @autoreleasepool {
+            context[@"root"] = root;
+        }
+        JSSynchronousEdenCollectForDebugging([context JSGlobalContextRef]);
+
+        // Create a new Obj-C object only reachable via the external object graph
+        // through the object we already scanned during the EdenCollection.
+        TestObject *child = [TestObject testObject];
+        [context.virtualMachine addManagedReference:child withOwner:root];
+
+        // Create a new managed JSValue that will only be kept alive if we properly rescan
+        // the external object graph.
+        JSManagedValue *managedJSObject = nil;
+        @autoreleasepool {
+            JSValue *jsObject = [JSValue valueWithObject:@"hello" inContext:context];
+            managedJSObject = [JSManagedValue managedValueWithValue:jsObject];
+            [context.virtualMachine addManagedReference:managedJSObject withOwner:child];
+        }
+
+        // Force another EdenCollection. It should rescan the new part of the external object graph.
+        JSSynchronousEdenCollectForDebugging([context JSGlobalContextRef]);
+        
+        // Check that the managed JSValue is still alive.
+        checkResult(@"EdenCollection doesn't reclaim new managed values", [managedJSObject value] != nil);
+    }
+
     currentThisInsideBlockGetterTest();
     runDateTests();
     runJSExportTests();
index c4c0c039f3827e2760ed24b0b0cf56b3b4119bdd..8d1e334e5471d85e38986e09612a42f90817e652 100644 (file)
@@ -1,3 +1,38 @@
+2014-04-15  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        Objective-C API external object graphs don't handle generational collection properly
+        https://bugs.webkit.org/show_bug.cgi?id=131634
+
+        Reviewed by Geoffrey Garen.
+
+        If the set of Objective-C objects transitively reachable through an object changes, we 
+        need to update the set of opaque roots accordingly. If we don't, the next EdenCollection 
+        won't rescan the external object graph, which would lead us to consider a newly allocated 
+        JSManagedValue to be dead.
+
+        * API/JSBase.cpp:
+        (JSSynchronousEdenCollectForDebugging):
+        * API/JSVirtualMachine.mm:
+        (-[JSVirtualMachine initWithContextGroupRef:]):
+        (-[JSVirtualMachine dealloc]):
+        (-[JSVirtualMachine isOldExternalObject:]):
+        (-[JSVirtualMachine addExternalRememberedObject:]):
+        (-[JSVirtualMachine addManagedReference:withOwner:]):
+        (-[JSVirtualMachine removeManagedReference:withOwner:]):
+        (-[JSVirtualMachine externalRememberedSet]):
+        (scanExternalObjectGraph):
+        (scanExternalRememberedSet):
+        * API/JSVirtualMachineInternal.h:
+        * API/tests/testapi.mm:
+        * heap/Heap.cpp:
+        (JSC::Heap::markRoots):
+        * heap/Heap.h:
+        (JSC::Heap::slotVisitor):
+        * heap/SlotVisitor.h:
+        * heap/SlotVisitorInlines.h:
+        (JSC::SlotVisitor::containsOpaqueRoot):
+        (JSC::SlotVisitor::containsOpaqueRootTriState):
+
 2014-04-15  Filip Pizlo  <fpizlo@apple.com>
 
         DFG IR should keep the data flow of doubles and int52's separate from the data flow of JSValue's
index 5a18ffc03bda3ed4e7bda5a32327a2c9183c385d..d42e6781cbb49670f72a9696085a24b47c2445a4 100644 (file)
@@ -41,6 +41,7 @@
 #include "JSLock.h"
 #include "JSONObject.h"
 #include "JSCInlines.h"
+#include "JSVirtualMachineInternal.h"
 #include "RecursiveAllocationScope.h"
 #include "Tracing.h"
 #include "UnlinkedCodeBlock.h"
@@ -512,6 +513,7 @@ void Heap::markRoots(double gcStartTime)
     {
         ParallelModeEnabler enabler(m_slotVisitor);
 
+        visitExternalRememberedSet();
         visitSmallStrings();
         visitConservativeRoots(conservativeRoots);
         visitCompilerWorklists();
@@ -591,6 +593,13 @@ void Heap::clearLivenessData()
     m_objectSpace.clearMarks();
 }
 
+void Heap::visitExternalRememberedSet()
+{
+#if JSC_OBJC_API_ENABLED
+    scanExternalRememberedSet(*m_vm, m_slotVisitor);
+#endif
+}
+
 void Heap::visitSmallStrings()
 {
     GCPHASE(VisitSmallStrings);
index e588a9fa1c46b4b234522c7210dfa0a5ed667cc2..69c393f6ef1dd573925f0684f9082684e025fe0a 100644 (file)
@@ -115,6 +115,8 @@ public:
     MarkedSpace& objectSpace() { return m_objectSpace; }
     MachineThreads& machineThreads() { return m_machineThreads; }
 
+    const SlotVisitor& slotVisitor() const { return m_slotVisitor; }
+
     JS_EXPORT_PRIVATE GCActivityCallback* fullActivityCallback();
     JS_EXPORT_PRIVATE GCActivityCallback* edenActivityCallback();
     JS_EXPORT_PRIVATE void setFullActivityCallback(PassRefPtr<FullGCActivityCallback>);
@@ -267,6 +269,7 @@ private:
     void gatherJSStackRoots(ConservativeRoots&);
     void gatherScratchBufferRoots(ConservativeRoots&);
     void clearLivenessData();
+    void visitExternalRememberedSet();
     void visitSmallStrings();
     void visitConservativeRoots(ConservativeRoots&);
     void visitCompilerWorklists();
index 4612742677db94e8e2c534d7537523127eb52010..25e32ea2c8967c790822a948e58c0d9321b2e596 100644 (file)
@@ -78,8 +78,8 @@ public:
     void unconditionallyAppend(JSCell*);
     
     void addOpaqueRoot(void*);
-    bool containsOpaqueRoot(void*);
-    TriState containsOpaqueRootTriState(void*);
+    bool containsOpaqueRoot(void*) const;
+    TriState containsOpaqueRootTriState(void*) const;
     int opaqueRootCount();
 
     GCThreadSharedData& sharedData() const { return m_shared; }
index 2d5712890e454c70c392a1c71ea6c307718fe1d0..f3355a58f56146ff9fb6b52b38280044e4cc5c33 100644 (file)
@@ -173,7 +173,7 @@ inline void SlotVisitor::addOpaqueRoot(void* root)
 #endif
 }
 
-inline bool SlotVisitor::containsOpaqueRoot(void* root)
+inline bool SlotVisitor::containsOpaqueRoot(void* root) const
 {
     ASSERT(!m_isInParallelMode);
 #if ENABLE(PARALLEL_GC)
@@ -184,7 +184,7 @@ inline bool SlotVisitor::containsOpaqueRoot(void* root)
 #endif
 }
 
-inline TriState SlotVisitor::containsOpaqueRootTriState(void* root)
+inline TriState SlotVisitor::containsOpaqueRootTriState(void* root) const
 {
     if (m_opaqueRoots.contains(root))
         return TrueTriState;