- Revision
- 167326
- Author
- mhahnenb...@apple.com
- Date
- 2014-04-15 14:05:09 -0700 (Tue, 15 Apr 2014)
Log Message
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):
Modified Paths
Diff
Modified: trunk/Source/_javascript_Core/API/JSBase.cpp (167325 => 167326)
--- trunk/Source/_javascript_Core/API/JSBase.cpp 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/API/JSBase.cpp 2014-04-15 21:05:09 UTC (rev 167326)
@@ -142,6 +142,7 @@
}
extern "C" JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);
+extern "C" JS_EXPORT void JSSynchronousEdenCollectForDebugging(JSContextRef);
void JSSynchronousGarbageCollectForDebugging(JSContextRef ctx)
{
@@ -153,6 +154,16 @@
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;
Modified: trunk/Source/_javascript_Core/API/JSVirtualMachine.mm (167325 => 167326)
--- trunk/Source/_javascript_Core/API/JSVirtualMachine.mm 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/API/JSVirtualMachine.mm 2014-04-15 21:05:09 UTC (rev 167326)
@@ -87,6 +87,7 @@
JSContextGroupRef m_group;
NSMapTable *m_contextCache;
NSMapTable *m_externalObjectGraph;
+ NSMapTable *m_externalRememberedSet;
}
- (instancetype)init
@@ -113,6 +114,9 @@
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 @@
JSContextGroupRelease(m_group);
[m_contextCache release];
[m_externalObjectGraph release];
+ [m_externalRememberedSet release];
[super dealloc];
}
@@ -145,6 +150,18 @@
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 @@
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 @@
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 @@
return m_externalObjectGraph;
}
+- (NSMapTable *)externalRememberedSet
+{
+ return m_externalRememberedSet;
+}
+
@end
void scanExternalObjectGraph(JSC::VM& vm, JSC::SlotVisitor& visitor, void* root)
@@ -253,13 +279,27 @@
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
Modified: trunk/Source/_javascript_Core/API/JSVirtualMachineInternal.h (167325 => 167326)
--- trunk/Source/_javascript_Core/API/JSVirtualMachineInternal.h 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/API/JSVirtualMachineInternal.h 2014-04-15 21:05:09 UTC (rev 167326)
@@ -26,10 +26,10 @@
#ifndef JSVirtualMachineInternal_h
#define JSVirtualMachineInternal_h
+#if JSC_OBJC_API_ENABLED
+
#import <_javascript_Core/_javascript_Core.h>
-#if JSC_OBJC_API_ENABLED
-
namespace JSC {
class VM;
class SlotVisitor;
@@ -51,7 +51,8 @@
#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
Modified: trunk/Source/_javascript_Core/API/tests/testapi.mm (167325 => 167326)
--- trunk/Source/_javascript_Core/API/tests/testapi.mm 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/API/tests/testapi.mm 2014-04-15 21:05:09 UTC (rev 167326)
@@ -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 @@
[[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();
Modified: trunk/Source/_javascript_Core/ChangeLog (167325 => 167326)
--- trunk/Source/_javascript_Core/ChangeLog 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/ChangeLog 2014-04-15 21:05:09 UTC (rev 167326)
@@ -1,3 +1,38 @@
+2014-04-15 Mark Hahnenberg <mhahnenb...@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 <fpi...@apple.com>
DFG IR should keep the data flow of doubles and int52's separate from the data flow of JSValue's
Modified: trunk/Source/_javascript_Core/heap/Heap.cpp (167325 => 167326)
--- trunk/Source/_javascript_Core/heap/Heap.cpp 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/heap/Heap.cpp 2014-04-15 21:05:09 UTC (rev 167326)
@@ -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 @@
{
ParallelModeEnabler enabler(m_slotVisitor);
+ visitExternalRememberedSet();
visitSmallStrings();
visitConservativeRoots(conservativeRoots);
visitCompilerWorklists();
@@ -591,6 +593,13 @@
m_objectSpace.clearMarks();
}
+void Heap::visitExternalRememberedSet()
+{
+#if JSC_OBJC_API_ENABLED
+ scanExternalRememberedSet(*m_vm, m_slotVisitor);
+#endif
+}
+
void Heap::visitSmallStrings()
{
GCPHASE(VisitSmallStrings);
Modified: trunk/Source/_javascript_Core/heap/Heap.h (167325 => 167326)
--- trunk/Source/_javascript_Core/heap/Heap.h 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/heap/Heap.h 2014-04-15 21:05:09 UTC (rev 167326)
@@ -115,6 +115,8 @@
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 @@
void gatherJSStackRoots(ConservativeRoots&);
void gatherScratchBufferRoots(ConservativeRoots&);
void clearLivenessData();
+ void visitExternalRememberedSet();
void visitSmallStrings();
void visitConservativeRoots(ConservativeRoots&);
void visitCompilerWorklists();
Modified: trunk/Source/_javascript_Core/heap/SlotVisitor.h (167325 => 167326)
--- trunk/Source/_javascript_Core/heap/SlotVisitor.h 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/heap/SlotVisitor.h 2014-04-15 21:05:09 UTC (rev 167326)
@@ -78,8 +78,8 @@
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; }
Modified: trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h (167325 => 167326)
--- trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2014-04-15 20:26:16 UTC (rev 167325)
+++ trunk/Source/_javascript_Core/heap/SlotVisitorInlines.h 2014-04-15 21:05:09 UTC (rev 167326)
@@ -173,7 +173,7 @@
#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 @@
#endif
}
-inline TriState SlotVisitor::containsOpaqueRootTriState(void* root)
+inline TriState SlotVisitor::containsOpaqueRootTriState(void* root) const
{
if (m_opaqueRoots.contains(root))
return TrueTriState;
