1336579 - [meta] Attach SetProp Function prototype

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P1)

Description

[Tom S]

Attachment: Gdocs example code

When assigning to "prototype" on a new function object we will never attach a stub, because we have to resolve the property first. Technically this should not be observable and we can just add the prototype property without resolving anything.

This is quite common on google docs, which uses a custom class inheritance mechanism.

Comment 1

[Tom S]

I would suggest we fix this as soon as possible. On the Google Spreadsheet in bug 1338802 this happens over 2000 times.

Comment 2

[Jan de Mooij [:jandem]]

P1, I'll try to get to this later this month if nobody beats me to it.

For now I'm focusing on converting the existing Baseline/Ion SetProp/SetElem stubs, and hopefully after that we can fix a lot of these bugs in a few weeks.

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Patch

It took me a while to realize we can take advantage of ObjectGroups for interpreted functions being tied to a particular function (see group->maybeInterpretedFunction), so we don't need any additional guards here.

The micro-benchmark below (similar pattern to the one in GDocs) improves from 1162 ms to 33 ms. It's a big win because we can eliminate the default prototype object allocation.

function f() {
    var proto = {};
    var t = new Date;
    for (var i=0; i<1000000; i++) {
        var f = function() {};
        f.prototype = proto;
    }
    print(new Date - t);
}
f();

Comment 4

[Tom S]

So this only works for one specific function expression/statement?

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Tom Schuster [:evilpie] from comment #4)
> So this only works for one specific function expression/statement?

Yeah because of how we assign groups to functions, but we need to guard on the group anyway when setting/adding properties (for TI), so it's hard to avoid.

Comment 6

[Tom S]

Comment on attachment 8854859 [details] [diff] [review]
Patch

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

Amazing results! I am a bit sad we can only optimize a specific function per stub though.

::: js/src/jit-test/tests/cacheir/add-function-prototype.js
@@ +1,2 @@
> +function addPrototype(fun, proto, resolvesPrototype) {
> +    fun.prototype = proto;

I don't trust this test. We only allow 6 stubs, and test 7 different functions. (Okay 4 of those aren't going to be attached, but still) You should have a unique IC per function kind that is tested.

::: js/src/jit/CacheIR.cpp
@@ +3099,5 @@
> +        // this |prototype| set and eliminate the default object allocation.
> +        //
> +        // We check group->maybeInterpretedFunction() here and guard on the
> +        // group. This ensures we don't add the default prototype property to
> +        // functions that don't have it.

I would add a comment that this group is unique.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ba362aa753
Attach AddSlot stubs for setting function.prototype. r=evilpie

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/39ba362aa753