1288456 - Wrong evaluation order in Function.prototype.bind

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[André Bargull [:anba]]

Test case:
---
var p = new Proxy(function(){}, new Proxy({}, {
    get(t, pk, r) {
        print("TRAP", pk);
    }
}));

Function.prototype.bind.call(p);
---

Expected: "getPrototypeOf" trap is called first
Actual: "getPrototypeOf" trap is called last

Comment 1

[André Bargull [:anba]]

Attachment: bug1288456.patch

Simply moved the [[GetPrototypeOf]] called to self-hosted code, so it gets called at the correct time.

Comment 2

[André Bargull [:anba]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86cf94147997f0676e8a12b2416eadd8dcc32676

Comment 3

[Till Schneidereit [:till]]

Comment on attachment 8802258 [details] [diff] [review]
bug1288456.patch

Review of attachment 8802258 [details] [diff] [review]:
-----------------------------------------------------------------

This is rather unfortunate: the reason for having it in C++ was that it avoided the overhead of another native call in JS. Function#bind is pretty performance-critical, so making it slower (probably by tens of %), sucks.

That said, we do already have a C++ call in there, for `std_Object_hasOwnProperty`, so theoretically, we could add a new intrinsic that combines the two things. However, it'd need to return more than one thing.

Alternatively, we could try moving everything after step 4 to C++. I'm pretty sure I remember that that caused a slowdown, but I also don't know whether I tried everything possible to avoid that in C++.

Clearing the review for now, as I'd rather not land this with a perf regression: it's quite unlikely to cause issues in real-world code. If you feel strongly about it, we should discuss it some more.

Comment 4

[André Bargull [:anba]]

Attachment: bug1288456.patch

New patch to avoid the 6-9% performance regression of the previous patch. Even provides better performance compared to tip, depending on the benchmark set-up:

```
function f() {} // Faster without patch.
// var f = function(){}; // Faster with patch.
function test() {
    // var f = function(){}; // Faster with patch.
    // function f() {} // Faster with patch.
    var t = dateNow();
    for (var i = 0; i < 1000000; ++i) f.bind();
    return dateNow() - t;
}
for (var i = 0; i < 10; ++i) print(test());
```

Comment 5

[Till Schneidereit [:till]]

Comment on attachment 8803412 [details] [diff] [review]
bug1288456.patch

Review of attachment 8803412 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you, very nice!

::: js/src/vm/SelfHosting.cpp
@@ +546,5 @@
> +    if (targetObj->is<JSFunction>() && !targetObj->as<JSFunction>().hasResolvedName())
> +        name = targetObj->as<JSFunction>().getUnresolvedName(cx);
> +
> +    RootedString rootedName(cx);
> +    if (name != nullptr) {

Nit: this can just be `if (name)`

@@ +564,4 @@
>      MOZ_ASSERT(!bound->hasGuessedAtom());
> +    if (rootedName && !rootedName->empty()) {
> +        StringBuffer sb(cx);
> +        if (!sb.append("bound ") || !sb.append(rootedName))

You should be able to use `cx->names().boundWithSpace` here, too, I think? I'm pretty sure it won't have a meaningful impact on perf, but it won't hurt, either.

Comment 6

[André Bargull [:anba]]

Attachment: bug1288456.patch

Applied suggested review comments, carrying r+ from Till.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/84300334b59c
Resolve the target's prototype earlier in Function.prototype.bind. r=till

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/84300334b59c