Open Bug 1417688 Opened 3 years ago Updated 3 months ago

MotionMark Focus20 is script bound when it should be graphics bound

Categories

(Core :: JavaScript Engine, defect, P2)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox59 --- affected

People

(Reporter: jrmuizel, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: leave-open)

Attachments

(2 files)

Don't call ArraySpeciesCreate for DOM proxies
1.19 KB, patch
anba
: review+
bzbarsky
: review+
Details | Diff | Splinter Review
slice benchmark
841 bytes, application/x-javascript
Details
Blocks: 1417616
40% of the total time of that 535ms span is in Array.prototype.slice working on a CSS2Properties proxy.

Brian, do we have ICs for this case? (Thinking that self-hosting this case of Array.p.slice might be an easy win.)

Jeff, I don't see 13ms/frame in script (?), but Array.p.slice does seem like the thing to work on in that profile.
Flags: needinfo?(bhackett1024)
status-firefox59: --- → affected
Priority: -- → P2
(In reply to Jason Orendorff [:jorendorff] from comment #1)
> 40% of the total time of that 535ms span is in Array.prototype.slice working
> on a CSS2Properties proxy.
> 
> Brian, do we have ICs for this case? (Thinking that self-hosting this case
> of Array.p.slice might be an easy win.)
> 
> Jeff, I don't see 13ms/frame in script (?), but Array.p.slice does seem like
> the thing to work on in that profile.

We don't have ICs for any native calls AFAIK.  Ion optimizes slice(), but only if the sliced object is a normal array and only by setting up a call to a specialized slice implementation, array_slice_dense().  I agree that self hosting seems like a good strategy for avoiding performance cliffs on uses of slice() like this.
Flags: needinfo?(bhackett1024)
We don't need to call the slow self-hosted ArraySpeciesCreate function when we already know that |this| is not an array or proxy wrapping an array. I think this is something we can assume for DOM proxies. This makes the slice call about 50% faster. Maybe there is still something we could optimize in Proxy::getElements?
I don't actually see a any difference on the MotionMark score ...
Attachment #8929849 - Flags: review?(bzbarsky)
Attachment #8929849 - Flags: review?(andrebargull)
We probably need to inline ElementAdder::append into CSS2Properties somehow.
And copying the strings for the different properties is also dominating.
Comment on attachment 8929849 [details] [diff] [review]
Don't call ArraySpeciesCreate for DOM proxies

>+            // We assume DOM proxies never return true for IsArray.

This is certainly a correct assumption.

That said, could we just call JS::IsArray here and have this work generally for all non-array cases?  It does involve an extra virtual call in the DOM proxy case, plus a bit of non-inline function calls to get that far, I guess, but should be clearer code...
Attachment #8929849 - Flags: review?(bzbarsky) → review+
> We probably need to inline ElementAdder::append into CSS2Properties somehow.

So one option is to make the protocol sort of like this:

1) Ask for how much length you will need.
2) Preallocate that much length.
3) Pass in the buffer to be filled in.

This works well as long as we know none of the bits involved can have side-effects, so not so hot with native objects, but might work well with DOM proxies... sort of.  If someone redefines the length getter, then we could start indexing outside the "real" length and having side-effects at that point, right?  :(

> And copying the strings for the different properties is also dominating.

This is allocating all the values for the array?  So stringifying all the CSS property names?

I wonder whether we could on-demand atomize them in the bindings or something and then just keep returning those atoms.
(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> That said, could we just call JS::IsArray here and have this work generally
> for all non-array cases?  It does involve an extra virtual call in the DOM
> proxy case, plus a bit of non-inline function calls to get that far, I
> guess, but should be clearer code...

I had the same idea in bug 1362753, but then :jandem asked me to measure the impact of calling JS::IsArray (bug 1362753, comment #6) and unfortunately in turned out that calling JS::IsArray leads to a noticeable performance regression (~8% slower) in certain µ-benchmarks (bug 1362753, comment #9 and bug 1362753, comment #10). :-(
Comment on attachment 8929849 [details] [diff] [review]
Don't call ArraySpeciesCreate for DOM proxies

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

If it doesn't regress the performance when calling Array.p.slice/splice with normal arrays, sure why not. I don't know how DOM objects/proxies work, so I can't really give a review for the |if (origArray->getClass()->isDOMClass()) { ... }| part of the patch, but I assume :bz checked that this part is okay.
Attachment #8929849 - Flags: review?(andrebargull) → review+
Attached file slice benchmark — 
Before:
arguments 1372.382080078125
empty array 1213.845947265625
object 1105.526123046875
bigger array 2968.282958984375

After:
arguments 1385.14697265625
empty array 1211.843994140625
object 1128.817138671875
bigger array 2986.899169921875

So it does get a bit slower :/
MOZ_ALWAYS_INLINE makes us faster of course. Really annoying we have to keep doing this.
Keywords: leave-open
Pushed by evilpies@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b286ab4131
Do not call ArraySpeciesCreate for DOM proxies. r=anba,bz
Jeff, what does the performance look like with this patch?
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf4a2c13e779
followup - Use #ifdef DEBUG instead of DebugOnly<> to work around MSVC bustage. r=red CLOSED TREE
The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
Flags: needinfo?(sdetar)
Jan, should this bug be closed?
Flags: needinfo?(sdetar) → needinfo?(jdemooij)
(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #7)
> > And copying the strings for the different properties is also dominating.
> 
> This is allocating all the values for the array?  So stringifying all the
> CSS property names?

Here's an updated profile: https://perfht.ml/2KAvEB1 I think we spend a bunch of time going from an ASCII CSS property name to UTF16 and then allocating a JS string for it and deflating this again...

> I wonder whether we could on-demand atomize them in the bindings or something
> and then just keep returning those atoms.

This could help a lot here I expect.

(In reply to Steven DeTar [:sdetar] from comment #17)
> Jan, should this bug be closed?

Jeff, is this still high priority? It runs pretty well for me on Mac with WR enabled.
Flags: needinfo?(jdemooij) → needinfo?(jmuizelaar)
> I think we spend a bunch of time going from an ASCII CSS property name to UTF16 and then allocating a JS
> string for it and deflating this again

Yes, we do.

The atomization thing would not play nice with custom properties.  But I wonder whether we could just add a "guaranteed ASCII" type to bindings or something to avoid the various round-tripping...
I guess custom property names don't have to be ASCII either, though.  :(
This is not high priority for us right now.
Flags: needinfo?(jmuizelaar)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

We still leave open for now

Flags: needinfo?(sdetar)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Flags: needinfo?(sdetar)

We will still leave open for now

Flags: needinfo?(sdetar)
You need to log in before you can comment on or make changes to this bug.