Attach SetProp Function prototype

RESOLVED FIXED in Firefox 55

Status

()

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: evilpie, Assigned: jandem)

Tracking

(Blocks: 2 bugs)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [qf:meta])

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Created attachment 8833477 [details]
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.
(Reporter)

Updated

2 years ago
Blocks: 1326344
(Reporter)

Comment 1

2 years ago
I would suggest we fix this as soon as possible. On the Google Spreadsheet in bug 1338802 this happens over 2000 times.
(Assignee)

Comment 2

2 years ago
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.
Priority: -- → P1
Whiteboard: [qf:meta]
(Assignee)

Comment 3

2 years ago
Created attachment 8854859 [details] [diff] [review]
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();
Assignee: nobody → jdemooij
Status: NEW → ASSIGNED
Attachment #8854859 - Flags: review?(evilpies)
(Reporter)

Comment 4

2 years ago
So this only works for one specific function expression/statement?
(Assignee)

Comment 5

2 years ago
(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.
(Reporter)

Comment 6

2 years ago
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.
Attachment #8854859 - Flags: review?(evilpies) → review+

Comment 7

2 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39ba362aa753
Attach AddSlot stubs for setting function.prototype. r=evilpie

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/39ba362aa753
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.