1417688 - MotionMark Focus20 is script bound when it should be graphics bound

Status: NEW

(Core :: JavaScript Engine, defect, P3)

Description

[Jeff Muizelaar [:jrmuizel]]

http://browserbench.org/MotionMark/developer.html?test-interval=30&display=minimal&tiles=big&controller=fixed&frame-rate=50&kalman-process-error=1&kalman-measurement-error=4&time-measurement=performance&suite-name=HTMLsuite&test-name=Focus20&complexity=64

https://perfht.ml/2z5QSV2

We spend 13ms in script. We don't have time for that.

Comment 1

[Jason Orendorff [:jorendorff]]

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.

Comment 2

[Brian Hackett [Laid off!]]

(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.

Comment 3

[Tom Schuster [:evilpie]]

Attachment: Don't call ArraySpeciesCreate for DOM proxies

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 ...

Comment 4

[Tom Schuster [:evilpie]]

We probably need to inline ElementAdder::append into CSS2Properties somehow.

Comment 5

[Tom Schuster [:evilpie]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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...

Comment 7

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 8

[André Bargull [:anba]]

(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 9

[André Bargull [:anba]]

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.

Comment 10

[Tom Schuster [:evilpie]]

Attachment: 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 :/

Comment 11

[Tom Schuster [:evilpie]]

MOZ_ALWAYS_INLINE makes us faster of course. Really annoying we have to keep doing this.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77b286ab4131
Do not call ArraySpeciesCreate for DOM proxies. r=anba,bz

Comment 13

[Tom Schuster [:evilpie]]

Jeff, what does the performance look like with this patch?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
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

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/77b286ab4131
https://hg.mozilla.org/mozilla-central/rev/cf4a2c13e779

Comment 16

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 17

[Steven DeTar [:sdetar]]

Jan, should this bug be closed?

Comment 18

[Jan de Mooij [:jandem]]

(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.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> 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...

Comment 20

[Boris Zbarsky [:bzbarsky]]

I guess custom property names don't have to be ASCII either, though.  :(

Comment 21

[Jeff Muizelaar [:jrmuizel]]

This is not high priority for us right now.

Comment 22

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 23

[Steven DeTar [:sdetar]]

We still leave open for now

Comment 24

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 25

[Steven DeTar [:sdetar]]

We will still leave open for now

Comment 26

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 27

[Steven DeTar [:sdetar]]

After some discussion we, we decided to still leave this bug open for now.

Comment 28

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 29

[Release mgmt bot [:suhaib / :marco/ :calixte]]

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

Comment 30

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?
For more information, please visit auto_nag documentation.