[JSC] putNonEnumerable in JSWrapperMap is too costly
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 20:20:49 +0000 (20:20 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 22 Feb 2019 20:20:49 +0000 (20:20 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194935

Reviewed by Mark Lam.

When we convert Objective-C blocks to JS objects, we need to set up a corresponding function object correctly.
During this allocation, we call [JSValue defineProperty:descriptor] to connect a "prototype" object and "constructor" object.
The problem is that this API has a particularly costly implementation:

    [[_context globalObject][@"Object"] invokeMethod:@"defineProperty" withArguments:@[ self, key, descriptor ]];

This wraps each JS objects appear in this code with Objective-C wrapper. And we convert a NSDictionary to JSObject, which
has "writable", "enumerable", "configurable", "value" fields, and call the "defineProperty" JS function through Objective-C wrapper.
This allocates many Objective-C wrappers and JS objects for descriptors. Since JSC has a direct C++ API "defineOwnProperty", we should
bypass these Objective-C APIs and call JSC's code directly.

This patch changes `putNonEnumerable` implementation, from calling [JSValue defineProperty:descriptor] to calling JSC C++ code directly.
We do not change [JSValue defineProperty:descriptor] implementation for now because of two reasons. (1) This is not used in our benchmarks
except for this (converting an Objective-C block to a JS object) one path. And (2) even if we were to re-write [JSValue defineProperty:descriptor]
to be more optimized, we would still want to call the JSC C++ version of defineProperty directly here to avoid NSDictionary allocation for a descriptor.

* API/APIUtils.h:
(setException):
* API/JSWrapperMap.mm:
(putNonEnumerable):
(copyMethodsToObject):
(-[JSObjCClassInfo allocateConstructorAndPrototypeInContext:]):
(-[JSObjCClassInfo wrapperForObject:inContext:]):

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

Source/JavaScriptCore/API/APIUtils.h
Source/JavaScriptCore/API/JSWrapperMap.mm
Source/JavaScriptCore/ChangeLog

index 2726396..7a5e8ba 100644 (file)
@@ -57,7 +57,7 @@ inline void setException(JSC::ExecState* exec, JSValueRef* returnedExceptionRef,
     if (returnedExceptionRef)
         *returnedExceptionRef = toRef(exec, exception);
 #if ENABLE(REMOTE_INSPECTOR)
-    VM& vm = exec->vm();
+    JSC::VM& vm = exec->vm();
     vm.vmEntryGlobalObject(exec)->inspectorController().reportAPIException(exec, JSC::Exception::create(vm, exception));
 #endif
 }
index 05ad1c4..201cb1d 100644 (file)
@@ -28,6 +28,7 @@
 
 #if JSC_OBJC_API_ENABLED
 #import "APICast.h"
+#import "APIUtils.h"
 #import "JSAPIWrapperObject.h"
 #import "JSCInlines.h"
 #import "JSCallbackObject.h"
@@ -178,14 +179,31 @@ static NSMutableDictionary *createRenameMap(Protocol *protocol, BOOL isInstanceM
     return renameMap;
 }
 
-inline void putNonEnumerable(JSValue *base, NSString *propertyName, JSValue *value)
+inline void putNonEnumerable(JSContext *context, JSValue *base, NSString *propertyName, JSValue *value)
 {
-    [base defineProperty:propertyName descriptor:@{
-        JSPropertyDescriptorValueKey: value,
-        JSPropertyDescriptorWritableKey: @YES,
-        JSPropertyDescriptorEnumerableKey: @NO,
-        JSPropertyDescriptorConfigurableKey: @YES
-    }];
+    if (![base isObject])
+        return;
+    JSC::ExecState* exec = toJS([context JSGlobalContextRef]);
+    JSC::VM& vm = exec->vm();
+    JSC::JSLockHolder locker(vm);
+    auto scope = DECLARE_CATCH_SCOPE(vm);
+
+    JSC::JSObject* baseObject = JSC::asObject(toJS(exec, [base JSValueRef]));
+    auto name = OpaqueJSString::tryCreate(propertyName);
+    if (!name)
+        return;
+
+    JSC::PropertyDescriptor descriptor;
+    descriptor.setValue(toJS(exec, [value JSValueRef]));
+    descriptor.setEnumerable(false);
+    descriptor.setConfigurable(true);
+    descriptor.setWritable(true);
+    bool shouldThrow = false;
+    baseObject->methodTable(vm)->defineOwnProperty(baseObject, exec, name->identifier(&vm), descriptor, shouldThrow);
+
+    JSValueRef exception = 0;
+    if (handleExceptionIfNeeded(scope, exec, &exception) == ExceptionStatus::DidThrow)
+        [context valueFromNotifyException:exception];
 }
 
 static bool isInitFamilyMethod(NSString *name)
@@ -254,7 +272,7 @@ static void copyMethodsToObject(JSContext *context, Class objcClass, Protocol *p
                 return;
             JSObjectRef method = objCCallbackFunctionForMethod(context, objcClass, protocol, isInstanceMethod, sel, types);
             if (method)
-                putNonEnumerable(object, name, [JSValue valueWithJSValueRef:method inContext:context]);
+                putNonEnumerable(context, object, name, [JSValue valueWithJSValueRef:method inContext:context]);
         }
     });
 
@@ -484,8 +502,9 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
 
         JSValue* prototype = [JSValue valueWithJSValueRef:toRef(jsPrototype) inContext:context];
         JSValue* constructor = [JSValue valueWithJSValueRef:toRef(jsConstructor) inContext:context];
-        putNonEnumerable(prototype, @"constructor", constructor);
-        putNonEnumerable(constructor, @"prototype", prototype);
+
+        putNonEnumerable(context, prototype, @"constructor", constructor);
+        putNonEnumerable(context, constructor, @"prototype", prototype);
 
         Protocol *exportProtocol = getJSExportProtocol();
         forEachProtocolImplementingProtocol(m_class, exportProtocol, ^(Protocol *protocol, bool&){
@@ -511,8 +530,8 @@ typedef std::pair<JSC::JSObject*, JSC::JSObject*> ConstructorPrototypePair;
         if (JSObjectRef method = objCCallbackFunctionForBlock(context, object)) {
             JSValue *constructor = [JSValue valueWithJSValueRef:method inContext:context];
             JSValue *prototype = [JSValue valueWithNewObjectInContext:context];
-            putNonEnumerable(constructor, @"prototype", prototype);
-            putNonEnumerable(prototype, @"constructor", constructor);
+            putNonEnumerable(context, constructor, @"prototype", prototype);
+            putNonEnumerable(context, prototype, @"constructor", constructor);
             return toJS(method);
         }
     }
index 708dc2e..41003ad 100644 (file)
@@ -1,5 +1,36 @@
 2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
 
+        [JSC] putNonEnumerable in JSWrapperMap is too costly
+        https://bugs.webkit.org/show_bug.cgi?id=194935
+
+        Reviewed by Mark Lam.
+
+        When we convert Objective-C blocks to JS objects, we need to set up a corresponding function object correctly.
+        During this allocation, we call [JSValue defineProperty:descriptor] to connect a "prototype" object and "constructor" object.
+        The problem is that this API has a particularly costly implementation:
+
+            [[_context globalObject][@"Object"] invokeMethod:@"defineProperty" withArguments:@[ self, key, descriptor ]];
+
+        This wraps each JS objects appear in this code with Objective-C wrapper. And we convert a NSDictionary to JSObject, which
+        has "writable", "enumerable", "configurable", "value" fields, and call the "defineProperty" JS function through Objective-C wrapper.
+        This allocates many Objective-C wrappers and JS objects for descriptors. Since JSC has a direct C++ API "defineOwnProperty", we should
+        bypass these Objective-C APIs and call JSC's code directly.
+
+        This patch changes `putNonEnumerable` implementation, from calling [JSValue defineProperty:descriptor] to calling JSC C++ code directly.
+        We do not change [JSValue defineProperty:descriptor] implementation for now because of two reasons. (1) This is not used in our benchmarks
+        except for this (converting an Objective-C block to a JS object) one path. And (2) even if we were to re-write [JSValue defineProperty:descriptor]
+        to be more optimized, we would still want to call the JSC C++ version of defineProperty directly here to avoid NSDictionary allocation for a descriptor.
+
+        * API/APIUtils.h:
+        (setException):
+        * API/JSWrapperMap.mm:
+        (putNonEnumerable):
+        (copyMethodsToObject):
+        (-[JSObjCClassInfo allocateConstructorAndPrototypeInContext:]):
+        (-[JSObjCClassInfo wrapperForObject:inContext:]):
+
+2019-02-22  Yusuke Suzuki  <ysuzuki@apple.com>
+
         Unreviewed, build fix after r241954
         https://bugs.webkit.org/show_bug.cgi?id=194939