A function object constructed using Function#bind must retain the [[Prototype]] of its target function. Currently, it gets the default one, namely Function.prototype. This is a breaking change introduced in ES6. For the reference, see steps 2 and 8 of BoundFunctionCreate() in the spec: https://people.mozilla.org/~jorendorff/es6-draft.html#sec-boundfunctioncreate For the justification, see https://bugs.ecmascript.org/show_bug.cgi?id=3819#c0 Here is a test case: ``` function getProtoBound(proto) { var f = Object.setPrototypeOf(function() { }, proto) var boundF = Function.prototype.bind.call(f, null) return Object.getPrototypeOf(boundF) } for (var proto of [ Function.prototype, function(){}, [], null ]) { console.log(Object.is(getProtoBound(proto), proto)) // should always log true } ```
I'd like to do this. Seems like it should be easy enough for a first contrib.
Sounds good. Indeed, it should be a simple patch.
So I'm trying to run the tests, and there seem to already be (failing) tests cases for this bug. After making some changes, when I run `Tools/Scripts/run-javascriptcore-tests`, I get the following output: ```` ........cat: test_fail_24491: No such file or directory cat: test_fail_24492: No such file or directory cat: test_fail_24493: No such file or directory es6.yaml/es6/prototype_of_bound_functions_arrow_functions.js.default: ERROR: Unexpected exit code: es6.yaml/es6/prototype_of_bound_functions_basic_functions.js.default: ERROR: Unexpected exit code: es6.yaml/es6/prototype_of_bound_functions_classes.js.default: ERROR: Unexpected exit code: 24493/24976 (failed 3) .....cat: test_fail_24495: No such file or directory es6.yaml/es6/prototype_of_bound_functions_subclasses.js.default: ERROR: Unexpected exit code: 24976/24976 (failed 4) ** The following JSC stress test failures have been introduced: es6.yaml/es6/prototype_of_bound_functions_arrow_functions.js.default es6.yaml/es6/prototype_of_bound_functions_basic_functions.js.default es6.yaml/es6/prototype_of_bound_functions_classes.js.default es6.yaml/es6/prototype_of_bound_functions_subclasses.js.default Results for JSC stress tests: 4 failures found. ```` Note the `cat` errors. I presume those files (test_fail_xxx) should contain the test output. How do I stop `cat` from err'ing out? The files it's referring to are empty
Or do the empty test output files indicate that the exit code was 0?
I'm not sure why your test output looks like that. Still, you should be able to fix it by changing those tests from expected failures to expected passes, by editing es6.yaml.
Created attachment 260799 [details] Patch
Thank you -- that did the trick. Please see the attached patch. One thing I'm unusure of is how I should implement #3 of the `BoundFunctionCreate` spec, wrt `ReturnIfAbrupt`. How is this handled in JSC? ```` ... #2 Let proto be targetFunction.[[GetPrototypeOf]](). #3 ReturnIfAbrupt(proto). ... ````
> Please see the attached patch. One thing I'm unusure of is how I should > implement #3 of the `BoundFunctionCreate` spec, wrt `ReturnIfAbrupt`. How is > this handled in JSC? > > ```` > ... > #2 Let proto be targetFunction.[[GetPrototypeOf]](). > #3 ReturnIfAbrupt(proto). > ... I think this just means that you should check for an exception when accessing the prototype. That isn't relevant until we implement proxies, since only proxies can throw when accessing their prototypes.
Comment on attachment 260799 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260799&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:83 > JSBoundFunction* function = new (NotNull, allocateCell<JSBoundFunction>(vm.heap)) JSBoundFunction(vm, globalObject, globalObject->boundFunctionStructure(), targetFunction, boundThis, boundArgs); You can see that this code believed it could share a structure between all bound functions in a global object. Your patch makes that fact no longer true. It would be preferable to remove the old code, which attempted to share a structure, and simply allocate a new structure explicitly here.
Created attachment 260806 [details] Patch
Thanks, I think I've done that now. Are structures used for IC, like v8 hidden classes?
> Are structures used for IC, like v8 hidden classes? Yes.
Comment on attachment 260806 [details] Patch r=me
Comment on attachment 260806 [details] Patch Clearing flags on attachment: 260806 Committed r189522: <http://trac.webkit.org/changeset/189522>
All reviewed patches have been landed. Closing bug.
It looks like this may have caused a 4.3% regression on speedometer benchmark.
(In reply to comment #16) > It looks like this may have caused a 4.3% regression on speedometer > benchmark. Tracked by <rdar://problem/22629950>.
Let's roll out this change until we can fix the regression. It seems like we need a way to avoid allocating all those structures. A simple option is to use the default structure in cases where the target function has not yet allocated its .prototype property. Come to think of it, we should also take care not to force the allocation of the .prototype property just to figure out if it exists. Most functions do not have a .prototype property because they are not called as constructors.
Re-opened since this is blocked by bug 149017
Created attachment 260899 [details] A/B testing results It looks like ReactJS got 15% slower with this patch.
@Geoffrey: Isn't the structure system designed to create one empty object structure per prototype? If so, how would the number of structures being allocated cause this regression? And I don't see the relevance of the `prototype` property to a `bind` call? If my above assumption re. structures is incorrect, would it not make more sense to check whether [[Prototype]] is not Function.prototype, and only then create a new structure.
@Ryosuke How can I run these tests locally?
(In reply to comment #22) > @Ryosuke > > How can I run these tests locally? Yes, run-perf-tests PerformanceTests/Speedometer/
Speedometer recovered after the roll out.
> @Geoffrey: > > Isn't the structure system designed to create one empty object structure per > prototype? If so, how would the number of structures being allocated cause > this regression? When you call Structure::create (via JSBoundFunction::createStucture), you allocate a structure. There is no default mechanism to avoid or de-duplicate that allocation. Allocation takes time and increases GC pressure. > And I don't see the relevance of the `prototype` property to a `bind` call? You're right: I misspoke when I mentioned .prototype. > If my above assumption re. structures is incorrect, would it not make more > sense to check whether [[Prototype]] is not Function.prototype, and only > then create a new structure. Yes, I think that's the right short-term solution: Maintain the per-global-object bound function structure, and only allocate a new structure if the per-global-object structure will not do.
Created attachment 260962 [details] Patch
Comment on attachment 260962 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260962&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:81 > + JSValue targetPrototype = targetFunction->prototype(); > + bool hasDefaultPrototype = &targetPrototype == (JSValue *)globalObject->functionPrototype(); This is wrong. You're taking the address of a value on the stack, which will never be equal to the address of a value on the heap. You should notice in testing that the hasDefaultPrototype condition never holds true.
You're right. What should I do? I originally felt I should use `JSValue::strictEqual` (which takes JSValue operands) but that requires an ExecState.
Created attachment 260999 [details] Patch
The code *should* be correct now. The reason I thought Structures for empty objects with a given prototype were cached was because I saw `PrototypeMap::emptyObjectStructureForPrototype`... any reason we don't use that here? It seems a waste to re-create the same Structure more than once. I'm sure this code will still perform poorly if someone has an array of functions with the same non-Function.prototype [[Prototype]] when they come to bind them.
(In reply to comment #30) > The code *should* be correct now. > > The reason I thought Structures for empty objects with a given prototype > were cached was because I saw > `PrototypeMap::emptyObjectStructureForPrototype`... any reason we don't use > that here? It seems a waste to re-create the same Structure more than once. > I'm sure this code will still perform poorly if someone has an array of > functions with the same non-Function.prototype [[Prototype]] when they come > to bind them. "Empty object" means an instance of JSFinalObject. We can't use emptyObjectStructureForPrototype because we're creating an instance of JSBoundFunction.
Comment on attachment 260999 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=260999&action=review > Source/JavaScriptCore/runtime/JSBoundFunction.cpp:81 > + bool hasDefaultPrototype = targetPrototype.isObject() && asObject(targetPrototype) == (JSObject *)globalObject->functionPrototype(); There's no need to cast functionPrototype() to JSObject*: FunctionPrototype inherits from JSObject. WebKit style uses C++ cast syntax rather than C syntax. WebKit style puts the * next to the type name, as in "JSObject*". The clearest and most efficient way to write this is "targetPrototype == globalObject->functionPrototype()". That's only one compare-and-branch instead of two.
Thanks Geoffrey. The reason for the cast was that it seemed to be necessary (see below). ```` bool hasDefaultPrototype = targetPrototype == globalObject->functionPrototype(); ```` doesn't compile. ```` run-javascriptcore-tests ... error: invalid operands to binary expression ('JSC::JSValue' and 'JSC::FunctionPrototype *') bool hasDefaultPrototype = targetPrototype == globalObject->functionPrototype(); ~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ```` Even ```` asObject(targetPrototype) == globalObject->functionPrototype(); ```` doesn't compile with ```` error: comparison of distinct pointer types ('JSC::JSObject *' and 'JSC::FunctionPrototype *') bool hasDefaultPrototype = asObject(targetPrototype) == globalObject->functionPrototype(); ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ```` which I don't understand because as you rightly say, FunctionPrototype inherits from JSObject
> error: invalid operands to binary expression ('JSC::JSValue' and > 'JSC::FunctionPrototype *') > bool hasDefaultPrototype = targetPrototype == > globalObject->functionPrototype(); To provide the right type, use JSValue(globalObject->functionPrototype()). > error: comparison of distinct pointer types ('JSC::JSObject *' and > 'JSC::FunctionPrototype *') > bool hasDefaultPrototype = asObject(targetPrototype) == > globalObject->functionPrototype(); > ~~~~~~~~~~~~~~~~~~~~~~~~~ ^ > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ```` > > which I don't understand because as you rightly say, FunctionPrototype > inherits from JSObject In this formulation we have the right type but the compiler doesn't know it. So, you need to #include FunctionPrototype.h. We shouldn't use this formulation, though, unless we can guarantee that the target callee in fact has a [[Prototype]]. I don't think we can guarantee that.
Thank you for the effort, Matthew. The issue was fixed in https://trac.webkit.org/changeset/196956. *** This bug has been marked as a duplicate of bug 154195 ***