1803042 - Handle scripted getter/setter clones better

Status: RESOLVED DUPLICATE of bug 1703469

(Core :: JavaScript Engine, defect, P3)

Description

[Mayank Bansal]

Go to https://codepen.io/meodai/pen/KKPLbKq

https://share.firefox.dev/3Faa7gp

Chrome feels a bit faster.

Feel free to INVALID if this is the expected behaviour.

Comment 1

[Jon Coppeard (:jonco)]

(In reply to Mayank Bansal from comment #0)

spend 20% of the time in JS:GC

The attached profile shows 0.4% of time in GC. I ran the demo and took a profile, which gave the same result.

Are you seeing something different locally?

Comment 2

[Mayank Bansal]

(In reply to Jon Coppeard (:jonco) from comment #1)

(In reply to Mayank Bansal from comment #0)

spend 20% of the time in JS:GC

The attached profile shows 0.4% of time in GC. I ran the demo and took a profile, which gave the same result.

Are you seeing something different locally?

This bug is based on the profile which suggests that the top function (top as per "invert call stack") is js::gc::HeaderWord::get and then some js::gc::CellWithTenuredGCPointer::headerPtr.

My naive understanding of the profile says that the actual time spent in GC is quite small, but there is " hand-wavy stuff around GC" where a lot of time is spent. And this may be something to look at.

Comment 3

[Mayank Bansal]

Attachment: profile.png

This is what I was referring to

Comment 4

[Jon Coppeard (:jonco)]

(In reply to Mayank Bansal from comment #2)
That's the VM reading the first word of a GC cell and is not part of GC itself.

Comment 5

[Nicolas B. Pierron [:nbp]]

There is definitely an issue reported by this profile.
The problem is that js::jit::IonGetPropertyIC::update is called at least once per frame, to handle calling with a getter.

So either we have a fast path, and we never take it, because we get new inputs every time, in which case we should find a way to generalized these ICs.
Or, we do not yet have a fast path and we should add one for calling getters.

Jan, what do you think?

Comment 6

[Jan de Mooij [:jandem]]

I looked into this a bit. They use a lot of (different) getter/setter functions.

We probably have to fix this by guarding on the getter/setter script instead of function identity, similar to what we can do for normal calls. We could then load the actual getter/setter function from the slot's GetterSetter instance and pass that as callee. We could similarly change jit::ObjectHasGetterSetterPure to also guard on the script for scripted getters/setters and return the actual getter/setter JSFunction*.

I don't have time to work on this now, unless we see this show up on other workloads.

Comment 7

[Jan de Mooij [:jandem]]

This is kind of similar to bug 1703469.