Attachment 362698 Details for Bug 194935 – Patch

[patch] Patch

bug-194935-20190221232322.patch (text/plain), 6.90 KB, created by Yusuke Suzuki on 2019-02-21 23:23:23 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Yusuke Suzuki

Created: 2019-02-21 23:23:23 PST

Size: 6.90 KB

patch

obsolete

>Subversion Revision: 241924
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index c068b843e90fc0753834d145cbefe12e820585b2..712220a5a28dbc1688983ff7006de4a4cd41d7d5 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,34 @@
>+2019-02-21  Yusuke Suzuki  <ysuzuki@apple.com>
>+
>+        [JSC] putNonEnumerable in JSWrapperMap is too costly
>+        https://bugs.webkit.org/show_bug.cgi?id=194935
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When we convert Objective-C blocks to JS objects, we need to set up 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 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 though [JSValue defineProperty:descriptor] is rewritten,
>+        we still want to call JSC C++ code 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-21  Yusuke Suzuki  <ysuzuki@apple.com>
> 
>         [JSC] Use Fast Malloc as much as possible
>diff --git a/Source/JavaScriptCore/API/APIUtils.h b/Source/JavaScriptCore/API/APIUtils.h
>index 272639611eae441f5782d4bb47e7a955b4642833..7a5e8ba8b8263fd585a7e4b1e6ba8fab0af0e1b2 100644
>--- a/Source/JavaScriptCore/API/APIUtils.h
>+++ b/Source/JavaScriptCore/API/APIUtils.h
>@@ -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
> }
>diff --git a/Source/JavaScriptCore/API/JSWrapperMap.mm b/Source/JavaScriptCore/API/JSWrapperMap.mm
>index 05ad1c4819a3da7209094f00e210026ac11a3911..26898c89cf2054a8522ffe82d08a3a892c6dfbbc 100644
>--- a/Source/JavaScriptCore/API/JSWrapperMap.mm
>+++ b/Source/JavaScriptCore/API/JSWrapperMap.mm
>@@ -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::JSLockHolder locker(exec);
>+    JSC::VM& vm = exec->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 @@ - (ConstructorPrototypePair)allocateConstructorAndPrototypeInContext:(JSContext
> 
>         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 @@ - (JSC::JSObject*)wrapperForObject:(id)object inContext:(JSContext *)context
>         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);
>         }
>     }

Flags:

mark.lam: review+
ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 194935: 362698 | 362720