Attachment 359239 Details for Bug 193423 – patch for landing.

[patch] patch for landing.

bug-193423.patch (text/plain), 8.74 KB, created by Mark Lam on 2019-01-15 18:44:10 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-01-15 18:44:10 PST

Size: 8.74 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 240024)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,16 @@
>+2019-01-15  Mark Lam  <mark.lam@apple.com>
>+
>+        JSFunction::canUseAllocationProfile() should account for builtin functions with no own prototypes.
>+        https://bugs.webkit.org/show_bug.cgi?id=193423
>+        <rdar://problem/46209355>
>+
>+        Reviewed by Saam Barati.
>+
>+        * microbenchmarks/sinkable-new-object-with-builtin-constructor.js: Added.
>+        * stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-1.js: Added.
>+        * stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js: Added.
>+        * stress/jsfunction-cannot-use-allocation-profile-with-builtin-functions-with-no-prototype.js: Added.
>+
> 2019-01-15  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
>Index: JSTests/microbenchmarks/sinkable-new-object-with-builtin-constructor.js
>===================================================================
>--- JSTests/microbenchmarks/sinkable-new-object-with-builtin-constructor.js	(nonexistent)
>+++ JSTests/microbenchmarks/sinkable-new-object-with-builtin-constructor.js	(working copy)
>@@ -0,0 +1,28 @@
>+function sumOfArithSeries(limit) {
>+    return limit * (limit + 1) / 2;
>+}
>+
>+var n = 10000000;
>+
>+function bar() { }
>+
>+function foo(b) {
>+    var result = 0;
>+    for (var i = 0; i < n; ++i) {
>+        var iter = ([i, i + 1])[Symbol.iterator]();
>+        if (b) {
>+            bar(o, p);
>+            return;
>+        }
>+        for (x of iter)
>+            result += x;
>+    }
>+    return result;
>+}
>+
>+noInline(bar);
>+noInline(foo);
>+
>+var result = foo(false);
>+if (result != sumOfArithSeries(n - 1) + sumOfArithSeries(n))
>+    throw "Error: bad result: " + result;
>Index: JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-1.js
>===================================================================
>--- JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-1.js	(nonexistent)
>+++ JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-1.js	(working copy)
>@@ -0,0 +1,15 @@
>+var invokeCount = 0;
>+
>+Object.defineProperty(Function.prototype, 'prototype', {
>+    get: function () {
>+        invokeCount++;
>+    }
>+});
>+
>+new Promise(resolve => {
>+    for (var i = 0; i < 10000; ++i)
>+        new resolve();
>+
>+    if (invokeCount != 10000)
>+        $vm.crash();
>+});
>Index: JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js
>===================================================================
>--- JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js	(nonexistent)
>+++ JSTests/stress/constructing-builtin-functions-with-getter-prototype-should-only-call-getter-once-per-new-2.js	(working copy)
>@@ -0,0 +1,15 @@
>+new Promise(resolve => {
>+    var invokeCount = 0;
>+
>+    Object.defineProperty(resolve, 'prototype', {
>+        get: function () {
>+            invokeCount++;
>+        }
>+    });
>+
>+    for (var i = 0; i < 10000; ++i)
>+        new resolve();
>+
>+    if (invokeCount != 10000)
>+        $vm.crash();
>+});
>Index: JSTests/stress/jsfunction-cannot-use-allocation-profile-with-builtin-functions-with-no-prototype.js
>===================================================================
>--- JSTests/stress/jsfunction-cannot-use-allocation-profile-with-builtin-functions-with-no-prototype.js	(nonexistent)
>+++ JSTests/stress/jsfunction-cannot-use-allocation-profile-with-builtin-functions-with-no-prototype.js	(working copy)
>@@ -0,0 +1,9 @@
>+Object.defineProperty(Function.prototype, 'prototype', {
>+    get: function () {
>+        throw new Error('hello');
>+    }
>+});
>+
>+new Promise(resolve => {
>+    new resolve();
>+});
>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 240024)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,26 @@
>+2019-01-15  Mark Lam  <mark.lam@apple.com>
>+
>+        JSFunction::canUseAllocationProfile() should account for builtin functions with no own prototypes.
>+        https://bugs.webkit.org/show_bug.cgi?id=193423
>+        <rdar://problem/46209355>
>+
>+        Reviewed by Saam Barati.
>+
>+        JSFunction::canUseAllocationProfile() should return false for most builtins
>+        because the majority of them have no prototype property.  The only exception to
>+        this is the few builtin functions that are explicitly used as constructors.
>+
>+        For these builtin constructors, JSFunction::canUseAllocationProfile() should also
>+        return false if the prototype property is a getter or custom getter because
>+        getting the prototype would then be effectful.
>+
>+        * dfg/DFGOperations.cpp:
>+        * runtime/CommonSlowPaths.cpp:
>+        (JSC::SLOW_PATH_DECL):
>+        * runtime/JSFunctionInlines.h:
>+        (JSC::JSFunction::canUseAllocationProfile):
>+        * runtime/PropertySlot.h:
>+
> 2019-01-15  Yusuke Suzuki  <yusukesuzuki@slowstart.org>
> 
>         [JSC] Use KnownStringUse for GetByVal(Array::String) since AI would offer wider type information and offer non-string type after removing Check(String)
>Index: Source/JavaScriptCore/dfg/DFGOperations.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOperations.cpp	(revision 240024)
>+++ Source/JavaScriptCore/dfg/DFGOperations.cpp	(working copy)
>@@ -301,7 +301,7 @@ JSCell* JIT_OPERATION operationCreateThi
>     auto scope = DECLARE_THROW_SCOPE(vm);
>     if (constructor->type() == JSFunctionType && jsCast<JSFunction*>(constructor)->canUseAllocationProfile()) {
>         auto rareData = jsCast<JSFunction*>(constructor)->ensureRareDataAndAllocationProfile(exec, inlineCapacity);
>-        RETURN_IF_EXCEPTION(scope, nullptr);
>+        scope.releaseAssertNoException();
>         ObjectAllocationProfile* allocationProfile = rareData->objectAllocationProfile();
>         Structure* structure = allocationProfile->structure();
>         JSObject* result = constructEmptyObject(exec, structure);
>Index: Source/JavaScriptCore/runtime/CommonSlowPaths.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/CommonSlowPaths.cpp	(revision 240024)
>+++ Source/JavaScriptCore/runtime/CommonSlowPaths.cpp	(working copy)
>@@ -243,6 +243,7 @@ SLOW_PATH_DECL(slow_path_create_this)
> 
>         size_t inlineCapacity = bytecode.inlineCapacity;
>         ObjectAllocationProfile* allocationProfile = constructor->ensureRareDataAndAllocationProfile(exec, inlineCapacity)->objectAllocationProfile();
>+        throwScope.releaseAssertNoException();
>         Structure* structure = allocationProfile->structure();
>         result = constructEmptyObject(exec, structure);
>         if (structure->hasPolyProto()) {
>Index: Source/JavaScriptCore/runtime/JSFunctionInlines.h
>===================================================================
>--- Source/JavaScriptCore/runtime/JSFunctionInlines.h	(revision 240024)
>+++ Source/JavaScriptCore/runtime/JSFunctionInlines.h	(working copy)
>@@ -110,8 +110,16 @@ inline bool JSFunction::hasReifiedName()
> 
> inline bool JSFunction::canUseAllocationProfile()
> {
>-    if (isHostFunction())
>-        return false;
>+    if (isHostOrBuiltinFunction()) {
>+        if (isHostFunction())
>+            return false;
>+
>+        VM& vm = globalObject()->vm();
>+        unsigned attributes;
>+        JSValue prototype = getDirect(vm, vm.propertyNames->prototype, attributes);
>+        if (!prototype || (attributes & PropertyAttribute::AccessorOrCustomAccessorOrValue))
>+            return false;
>+    }
> 
>     // If we don't have a prototype property, we're not guaranteed it's
>     // non-configurable. For example, user code can define the prototype
>Index: Source/JavaScriptCore/runtime/PropertySlot.h
>===================================================================
>--- Source/JavaScriptCore/runtime/PropertySlot.h	(revision 240024)
>+++ Source/JavaScriptCore/runtime/PropertySlot.h	(working copy)
>@@ -45,6 +45,7 @@ enum class PropertyAttribute : unsigned 
>     CustomAccessor    = 1 << 5,
>     CustomValue       = 1 << 6,
>     CustomAccessorOrValue = CustomAccessor | CustomValue,
>+    AccessorOrCustomAccessorOrValue = Accessor | CustomAccessor | CustomValue,
> 
>     // Things that are used by static hashtables are not in the attributes byte in PropertyMapEntry.
>     Function          = 1 << 8,  // property is a function - only used by static hashtables

Actions: View | Formatted Diff | Diff

Attachments on bug 193423: 359116 | 359137 | 359239