Closed Bug 1336579 Opened 7 years ago Closed 7 years ago

[meta] Attach SetProp Function prototype


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




Performance Impact ?
Tracking Status
firefox55 --- fixed


(Reporter: evilpie, Assigned: jandem)


(Blocks 1 open bug)


(Keywords: meta)


(2 files)

Attached file 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.
Blocks: 1326344
I would suggest we fix this as soon as possible. On the Google Spreadsheet in bug 1338802 this happens over 2000 times.
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]
Attached patch PatchSplinter Review
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);
Assignee: nobody → jdemooij
Attachment #8854859 - Flags: review?(evilpies)
So this only works for one specific function expression/statement?
(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 on attachment 8854859 [details] [diff] [review]

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+
Pushed by
Attach AddSlot stubs for setting function.prototype. r=evilpie
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Performance Impact: --- → ?
Keywords: meta
Whiteboard: [qf:meta]
Summary: Attach SetProp Function prototype → [meta] Attach SetProp Function prototype
You need to log in before you can comment on or make changes to this bug.