[JSC] JSWrapperMap should not use Objective-C Weak map (NSMapTable with NSPointerFunc...
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Mar 2019 01:30:16 +0000 (01:30 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 30 Mar 2019 01:30:16 +0000 (01:30 +0000)
https://bugs.webkit.org/show_bug.cgi?id=196392

Reviewed by Saam Barati.

Weak representation in Objective-C is surprisingly costly in terms of memory. We can see that very easy program shows 10KB memory consumption due to
this weak wrapper map in JavaScriptCore.framework. But we do not need this weak map since Objective-C JSValue has a dealloc. We can unregister itself
from the map when it is deallocated without using Objective-C weak mechanism. And since Objective-C JSValue is tightly coupled to a specific JSContext,
and wrapper map is created per JSContext, JSValue wrapper and actual JavaScriptCore value is one-on-one, and [JSValue dealloc] knows which JSContext's
wrapper map holds itself.

1. We do not use Objective-C weak mechanism. We use WTF::HashSet instead. When JSValue is allocated, we register it to JSWrapperMap's HashSet. And unregister
   JSValue from this map when JSValue is deallocated.
2. We use HashSet<JSValue> (logically) instead of HashMap<JSValueRef, JSValue> to keep JSValueRef and JSValue relationship. We can achieve it because JSValue
   holds JSValueRef inside it.

* API/JSContext.mm:
(-[JSContext removeWrapper:]):
* API/JSContextInternal.h:
* API/JSValue.mm:
(-[JSValue dealloc]):
(-[JSValue initWithValue:inContext:]):
* API/JSWrapperMap.h:
* API/JSWrapperMap.mm:
(WrapperKey::hashTableDeletedValue):
(WrapperKey::WrapperKey):
(WrapperKey::isHashTableDeletedValue const):
(WrapperKey::Hash::hash):
(WrapperKey::Hash::equal):
(WrapperKey::Traits::isEmptyValue):
(WrapperKey::Translator::hash):
(WrapperKey::Translator::equal):
(WrapperKey::Translator::translate):
(-[JSWrapperMap initWithGlobalContextRef:]):
(-[JSWrapperMap dealloc]):
(-[JSWrapperMap objcWrapperForJSValueRef:inContext:]):
(-[JSWrapperMap removeWrapper:]):
* API/tests/testapi.mm:
(testObjectiveCAPIMain):

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

Source/JavaScriptCore/API/JSContext.mm
Source/JavaScriptCore/API/JSContextInternal.h
Source/JavaScriptCore/API/JSValue.mm
Source/JavaScriptCore/API/JSWrapperMap.h
Source/JavaScriptCore/API/JSWrapperMap.mm
Source/JavaScriptCore/API/tests/testapi.mm
Source/JavaScriptCore/ChangeLog

index 094c937..5a85f30 100644 (file)
     return [[self wrapperMap] objcWrapperForJSValueRef:value inContext:self];
 }
 
+- (void)removeWrapper:(JSValue *)value
+{
+    return [[self wrapperMap] removeWrapper:value];
+}
+
 + (JSContext *)contextWithJSGlobalContextRef:(JSGlobalContextRef)globalContext
 {
     JSContext *context = (__bridge JSContext *)toJSGlobalObject(globalContext)->apiWrapper();
index 958c479..6b1ac57 100644 (file)
@@ -54,6 +54,7 @@ struct CallbackData {
 - (JSWrapperMap *)wrapperMap;
 - (JSValue *)wrapperForObjCObject:(id)object;
 - (JSValue *)wrapperForJSObject:(JSValueRef)value;
+- (void)removeWrapper:(JSValue *)value;
 
 @end
 
index 86e10cf..32e587f 100644 (file)
@@ -71,6 +71,7 @@ NSString * const JSPropertyDescriptorSetKey = @"set";
 
 - (void)dealloc
 {
+    [_context removeWrapper:self];
     JSValueUnprotect([_context JSGlobalContextRef], m_value);
     [_context release];
     _context = nil;
@@ -1075,6 +1076,7 @@ JSValueRef valueInternalValue(JSValue * value)
     if (!self)
         return nil;
 
+    ASSERT(context);
     _context = [context retain];
     m_value = value;
     JSValueProtect([_context JSGlobalContextRef], m_value);
index 6c18c64..08cef54 100644 (file)
@@ -37,6 +37,8 @@
 
 - (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:(JSContext *)context;
 
+- (void)removeWrapper:(JSValue *)wrapper;
+
 @end
 
 id tryUnwrapObjcObject(JSGlobalContextRef, JSValueRef);
index 201cb1d..82e1d68 100644 (file)
@@ -581,10 +581,77 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
 
 @end
 
+struct WrapperKey {
+    static constexpr uintptr_t hashTableDeletedValue() { return 1; }
+
+    WrapperKey() = default;
+
+    explicit WrapperKey(WTF::HashTableDeletedValueType)
+        : m_wrapper(reinterpret_cast<JSValue *>(hashTableDeletedValue()))
+    {
+    }
+
+    explicit WrapperKey(JSValue *wrapper)
+        : m_wrapper(wrapper)
+    {
+    }
+
+    bool isHashTableDeletedValue() const
+    {
+        return reinterpret_cast<uintptr_t>(m_wrapper) == hashTableDeletedValue();
+    }
+
+    __unsafe_unretained JSValue *m_wrapper { nil };
+
+    struct Hash {
+        static unsigned hash(const WrapperKey& key)
+        {
+            return DefaultHash<JSValueRef>::Hash::hash([key.m_wrapper JSValueRef]);
+        }
+
+        static bool equal(const WrapperKey& lhs, const WrapperKey& rhs)
+        {
+            return lhs.m_wrapper == rhs.m_wrapper;
+        }
+
+        static const bool safeToCompareToEmptyOrDeleted = false;
+    };
+
+    struct Traits : public SimpleClassHashTraits<WrapperKey> {
+        static const bool hasIsEmptyValueFunction = true;
+        static bool isEmptyValue(const WrapperKey& key)
+        {
+            return key.m_wrapper == nullptr;
+        }
+    };
+
+    struct Translator {
+        struct ValueAndContext {
+            __unsafe_unretained JSContext *m_context;
+            JSValueRef m_value;
+        };
+
+        static unsigned hash(const ValueAndContext& value)
+        {
+            return DefaultHash<JSValueRef>::Hash::hash(value.m_value);
+        }
+
+        static bool equal(const WrapperKey& lhs, const ValueAndContext& value)
+        {
+            return [lhs.m_wrapper JSValueRef] == value.m_value;
+        }
+
+        static void translate(WrapperKey& result, const ValueAndContext& value, unsigned)
+        {
+            result = WrapperKey([[[JSValue alloc] initWithValue:value.m_value inContext:value.m_context] autorelease]);
+        }
+    };
+};
+
 @implementation JSWrapperMap {
     NSMutableDictionary *m_classMap;
     std::unique_ptr<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>> m_cachedJSWrappers;
-    NSMapTable *m_cachedObjCWrappers;
+    HashSet<WrapperKey, WrapperKey::Hash, WrapperKey::Traits> m_cachedObjCWrappers;
 }
 
 - (instancetype)initWithGlobalContextRef:(JSGlobalContextRef)context
@@ -593,10 +660,6 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
     if (!self)
         return nil;
 
-    NSPointerFunctionsOptions keyOptions = NSPointerFunctionsOpaqueMemory | NSPointerFunctionsOpaquePersonality;
-    NSPointerFunctionsOptions valueOptions = NSPointerFunctionsWeakMemory | NSPointerFunctionsObjectPersonality;
-    m_cachedObjCWrappers = [[NSMapTable alloc] initWithKeyOptions:keyOptions valueOptions:valueOptions capacity:0];
-
     m_cachedJSWrappers = std::make_unique<JSC::WeakGCMap<__unsafe_unretained id, JSC::JSObject>>(toJS(context)->vm());
 
     ASSERT(!toJSGlobalObject(context)->wrapperMap());
@@ -607,7 +670,6 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
 
 - (void)dealloc
 {
-    [m_cachedObjCWrappers release];
     [m_classMap release];
     [super dealloc];
 }
@@ -662,12 +724,14 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
 - (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:context
 {
     ASSERT(toJSGlobalObject([context JSGlobalContextRef])->wrapperMap() == self);
-    JSValue *wrapper = (__bridge JSValue *)NSMapGet(m_cachedObjCWrappers, value);
-    if (!wrapper) {
-        wrapper = [[[JSValue alloc] initWithValue:value inContext:context] autorelease];
-        NSMapInsert(m_cachedObjCWrappers, value, (__bridge void*)wrapper);
-    }
-    return wrapper;
+    WrapperKey::Translator::ValueAndContext valueAndContext { context, value };
+    auto addResult = m_cachedObjCWrappers.add<WrapperKey::Translator>(valueAndContext);
+    return addResult.iterator->m_wrapper;
+}
+
+- (void)removeWrapper:(JSValue *)wrapper
+{
+    m_cachedObjCWrappers.remove(WrapperKey(wrapper));
 }
 
 @end
index 5ad486f..ac6dc19 100644 (file)
@@ -565,12 +565,28 @@ static void runJITThreadLimitTests()
 static void testObjectiveCAPIMain()
 {
     @autoreleasepool {
-        JSVirtualMachinevm = [[JSVirtualMachine alloc] init];
-        JSContextcontext = [[JSContext alloc] initWithVirtualMachine:vm];
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
         [context evaluateScript:@"bad"];
     }
 
     @autoreleasepool {
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
+        JSValue *number1 = [context evaluateScript:@"42092389"];
+        JSValue *number2 = [context evaluateScript:@"42092389"];
+        checkResult(@"wrapper cache for numbers", number1 == number2 && number1.isNumber && [number1 toInt32] == 42092389);
+    }
+
+    @autoreleasepool {
+        JSVirtualMachine *vm = [[JSVirtualMachine alloc] init];
+        JSContext *context = [[JSContext alloc] initWithVirtualMachine:vm];
+        JSValue *object1 = [context evaluateScript:@"({})"];
+        JSValue *object2 = [context evaluateScript:@"({})"];
+        checkResult(@"wrapper cache for objects", object1 != object2);
+    }
+
+    @autoreleasepool {
         JSContext *context = [[JSContext alloc] init];
         JSValue *result = [context evaluateScript:@"2 + 2"];
         checkResult(@"2 + 2", result.isNumber && [result toInt32] == 4);
index 6633a76..e711256 100644 (file)
@@ -1,3 +1,45 @@
+2019-03-29  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        [JSC] JSWrapperMap should not use Objective-C Weak map (NSMapTable with NSPointerFunctionsWeakMemory) for m_cachedObjCWrappers
+        https://bugs.webkit.org/show_bug.cgi?id=196392
+
+        Reviewed by Saam Barati.
+
+        Weak representation in Objective-C is surprisingly costly in terms of memory. We can see that very easy program shows 10KB memory consumption due to
+        this weak wrapper map in JavaScriptCore.framework. But we do not need this weak map since Objective-C JSValue has a dealloc. We can unregister itself
+        from the map when it is deallocated without using Objective-C weak mechanism. And since Objective-C JSValue is tightly coupled to a specific JSContext,
+        and wrapper map is created per JSContext, JSValue wrapper and actual JavaScriptCore value is one-on-one, and [JSValue dealloc] knows which JSContext's
+        wrapper map holds itself.
+
+        1. We do not use Objective-C weak mechanism. We use WTF::HashSet instead. When JSValue is allocated, we register it to JSWrapperMap's HashSet. And unregister
+           JSValue from this map when JSValue is deallocated.
+        2. We use HashSet<JSValue> (logically) instead of HashMap<JSValueRef, JSValue> to keep JSValueRef and JSValue relationship. We can achieve it because JSValue
+           holds JSValueRef inside it.
+
+        * API/JSContext.mm:
+        (-[JSContext removeWrapper:]):
+        * API/JSContextInternal.h:
+        * API/JSValue.mm:
+        (-[JSValue dealloc]):
+        (-[JSValue initWithValue:inContext:]):
+        * API/JSWrapperMap.h:
+        * API/JSWrapperMap.mm:
+        (WrapperKey::hashTableDeletedValue):
+        (WrapperKey::WrapperKey):
+        (WrapperKey::isHashTableDeletedValue const):
+        (WrapperKey::Hash::hash):
+        (WrapperKey::Hash::equal):
+        (WrapperKey::Traits::isEmptyValue):
+        (WrapperKey::Translator::hash):
+        (WrapperKey::Translator::equal):
+        (WrapperKey::Translator::translate):
+        (-[JSWrapperMap initWithGlobalContextRef:]):
+        (-[JSWrapperMap dealloc]):
+        (-[JSWrapperMap objcWrapperForJSValueRef:inContext:]):
+        (-[JSWrapperMap removeWrapper:]):
+        * API/tests/testapi.mm:
+        (testObjectiveCAPIMain):
+
 2019-03-29  Robin Morisset  <rmorisset@apple.com>
 
         B3ReduceStrength should know that Mul distributes over Add and Sub