Attachment 359137 Details for Bug 193423 – proposed patch.

[patch] proposed patch.

bug-193423.patch (text/plain), 6.81 KB, created by Mark Lam on 2019-01-14 22:24:17 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-01-14 22:24:17 PST

Size: 6.81 KB

patch

obsolete

>Index: JSTests/ChangeLog
>===================================================================
>--- JSTests/ChangeLog	(revision 239975)
>+++ JSTests/ChangeLog	(working copy)
>@@ -1,3 +1,14 @@
>+2019-01-14  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 NOBODY (OOPS!).
>+
>+        * microbenchmarks/sinkable-new-object-with-builtin-constructor.js: Added.
>+        * stress/jsfunction-cannot-use-allocation-profile-with-builtin-functions-with-no-prototype.js: Added.
>+
> 2019-01-14  Keith Miller  <keith_miller@apple.com>
> 
>         Skip type-check-hoisting-phase-hoist... with no jit
>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/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 239936)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,26 @@
>+2019-01-14  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 NOBODY (OOPS!).
>+
>+        JSFunction::canUseAllocationProfile() should return false for most builtins
>+        because the majority of them has 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-14  Keith Miller  <keith_miller@apple.com>
> 
>         JSC should have a module loader API
>Index: Source/JavaScriptCore/dfg/DFGOperations.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGOperations.cpp	(revision 239936)
>+++ 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 239936)
>+++ 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 239936)
>+++ 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 239936)
>+++ 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

Flags:

saam: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 193423: 359116 | 359137 | 359239