Bug 145605

Summary: Function.prototype.bind: Bound functions must use the [[Prototype]] of their target function instead of Function.prototype
Product: WebKit Reporter: Claude Pache <claude.pache>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: REOPENED ---    
Severity: Normal CC: barraclough, cdumez, commit-queue, ggaren, kling, matthew.jh, rniwa
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 149017, 149020    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
A/B testing results
none
Patch
none
Patch ggaren: review-

Description Claude Pache 2015-06-03 08:58:38 PDT
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
}
```
Comment 1 Matthew Hill 2015-09-08 07:03:51 PDT
I'd like to do this. Seems like it should be easy enough for a first contrib.
Comment 2 Geoffrey Garen 2015-09-08 11:00:36 PDT
Sounds good. Indeed, it should be a simple patch.
Comment 3 Matthew Hill 2015-09-08 14:23:27 PDT
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
Comment 4 Matthew Hill 2015-09-08 14:24:39 PDT
Or do the empty test output files indicate that the exit code was 0?
Comment 5 Geoffrey Garen 2015-09-08 14:42:02 PDT
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.
Comment 6 Matthew Hill 2015-09-08 15:19:23 PDT
Created attachment 260799 [details]
Patch
Comment 7 Matthew Hill 2015-09-08 15:21:42 PDT
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).
...
````
Comment 8 Geoffrey Garen 2015-09-08 15:37:33 PDT
> 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 9 Geoffrey Garen 2015-09-08 15:40:02 PDT
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.
Comment 10 Matthew Hill 2015-09-08 16:28:39 PDT
Created attachment 260806 [details]
Patch
Comment 11 Matthew Hill 2015-09-08 16:42:48 PDT
Thanks, I think I've done that now. Are structures used for IC, like v8 hidden classes?
Comment 12 Geoffrey Garen 2015-09-08 17:13:20 PDT
> Are structures used for IC, like v8 hidden classes?

Yes.
Comment 13 Geoffrey Garen 2015-09-08 17:14:06 PDT
Comment on attachment 260806 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2015-09-08 18:01:16 PDT
Comment on attachment 260806 [details]
Patch

Clearing flags on attachment: 260806

Committed r189522: <http://trac.webkit.org/changeset/189522>
Comment 15 WebKit Commit Bot 2015-09-08 18:01:20 PDT
All reviewed patches have been landed.  Closing bug.
Comment 16 Chris Dumez 2015-09-09 16:12:45 PDT
It looks like this may have caused a 4.3% regression on speedometer benchmark.
Comment 17 Chris Dumez 2015-09-09 16:13:43 PDT
(In reply to comment #16)
> It looks like this may have caused a 4.3% regression on speedometer
> benchmark.

Tracked by <rdar://problem/22629950>.
Comment 18 Geoffrey Garen 2015-09-09 17:01:51 PDT
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.
Comment 19 WebKit Commit Bot 2015-09-09 17:21:04 PDT
Re-opened since this is blocked by bug 149017
Comment 20 Ryosuke Niwa 2015-09-09 18:27:47 PDT
Created attachment 260899 [details]
A/B testing results

It looks like ReactJS got 15% slower with this patch.
Comment 21 Matthew Hill 2015-09-10 01:15:34 PDT
@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.
Comment 22 Matthew Hill 2015-09-10 01:37:48 PDT
@Ryosuke 

How can I run these tests locally?
Comment 23 Ryosuke Niwa 2015-09-10 07:00:41 PDT
(In reply to comment #22)
> @Ryosuke 
> 
> How can I run these tests locally?

Yes, run-perf-tests PerformanceTests/Speedometer/
Comment 24 Chris Dumez 2015-09-10 09:16:59 PDT
Speedometer recovered after the roll out.
Comment 25 Geoffrey Garen 2015-09-10 11:36:04 PDT
> @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.
Comment 26 Matthew Hill 2015-09-10 16:16:40 PDT
Created attachment 260962 [details]
Patch
Comment 27 Geoffrey Garen 2015-09-10 16:21:07 PDT
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.
Comment 28 Matthew Hill 2015-09-10 16:31:44 PDT
You're right. What should I do?

I originally felt I should use `JSValue::strictEqual` (which takes JSValue operands) but that requires an ExecState.
Comment 29 Matthew Hill 2015-09-11 03:18:44 PDT
Created attachment 260999 [details]
Patch
Comment 30 Matthew Hill 2015-09-11 03:23:14 PDT
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.
Comment 31 Geoffrey Garen 2015-09-11 10:41:49 PDT
(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 32 Geoffrey Garen 2015-09-11 10:45:30 PDT
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.
Comment 33 Matthew Hill 2015-09-11 11:18:47 PDT
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
Comment 34 Geoffrey Garen 2015-09-11 14:21:24 PDT
> 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.