Changeset 243672 in webkit

Timestamp:

Mar 29, 2019, 6:30:16 PM (7 years ago)

Author:

ysuzuki@apple.com

Message:

[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):

Location:

trunk/Source/JavaScriptCore

Files:

• API/JSContext.mm (modified) (1 diff)
• API/JSContextInternal.h (modified) (1 diff)
• API/JSValue.mm (modified) (2 diffs)
• API/JSWrapperMap.h (modified) (1 diff)
• API/JSWrapperMap.mm (modified) (4 diffs)
• API/tests/testapi.mm (modified) (1 diff)
• ChangeLog (modified) (1 diff)

trunk/Source/JavaScriptCore/API/JSContext.mm

@@ -370 +370 @@
 }
 
+- (void)removeWrapper:(JSValue *)value
+{
+    return [[self wrapperMap] removeWrapper:value];
+}
+
 + (JSContext *)contextWithJSGlobalContextRef:(JSGlobalContextRef)globalContext
 {

trunk/Source/JavaScriptCore/API/JSContextInternal.h

@@ -55 +55 @@
 - (JSValue *)wrapperForObjCObject:(id)object;
 - (JSValue *)wrapperForJSObject:(JSValueRef)value;
+- (void)removeWrapper:(JSValue *)value;
 
 @end

trunk/Source/JavaScriptCore/API/JSValue.mm

@@ -72 +72 @@
 - (void)dealloc
 {
+    [_context removeWrapper:self];
     JSValueUnprotect([_context JSGlobalContextRef], m_value);
     [_context release];
@@ -1076 +1077 @@
         return nil;
 
+    ASSERT(context);
     _context = [context retain];
     m_value = value;

trunk/Source/JavaScriptCore/API/JSWrapperMap.h

@@ -38 +38 @@
 - (JSValue *)objcWrapperForJSValueRef:(JSValueRef)value inContext:(JSContext *)context;
 
+- (void)removeWrapper:(JSValue *)wrapper;
+
 @end
 

trunk/Source/JavaScriptCore/API/JSWrapperMap.mm

@@ -582 +582 @@
 @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;
 }
 
@@ -593 +660 @@
     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());
@@ -608 +671 @@
 - (void)dealloc
 {
-    [m_cachedObjCWrappers release];
     [m_classMap release];
     [super dealloc];
@@ -663 +725 @@
 {
     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));
 }
 

trunk/Source/JavaScriptCore/API/tests/testapi.mm

@@ -566 +566 @@
 {
     @autoreleasepool {
-        JSVirtualMachine* vm = [[JSVirtualMachine alloc] init];
-        JSContext* context = [[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);
     }
 

trunk/Source/JavaScriptCore/ChangeLog

@@ +1 @@
+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>