1235408 - 2-10% sessionrestore/tart regression on inbound (v.46) on Dec 23 from revision

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

We got some talos alerts, and backfilling data yielded results:
https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=4a967e08dff7&newProject=mozilla-inbound&newRevision=bb23aa681e99&framework=1&filter=session&showOnlyConfident=1

and a win8 tart one:
https://treeherder.allizom.org/perf.html#/comparesubtest?originalProject=mozilla-inbound&originalRevision=4a967e08dff7&newProject=mozilla-inbound&newRevision=bb23aa681e99&originalSignature=b43983bbf31fc4b74b4a65775bc8eb416f6aa82e&newSignature=b43983bbf31fc4b74b4a65775bc8eb416f6aa82e

looking over these with the retriggers for more data, this set of patches from bug 1233111 look to be the culprit.

To test a fix out on try you can:
mach try -b o -p linux64,macosx64,win32,win64 -u none -t other

to learn more about the tests, here is a link to the wiki:
https://wiki.mozilla.org/Buildbot/Talos/Tests#sessionrestore.2Fsessionrestore_no_auto_restore

and to run these locally:
https://wiki.mozilla.org/Buildbot/Talos/Running#Running_locally_-_Source_Code

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

:jolesen, can you take a look at this and determine if this is expected and if there is anything we can reasonably do to reduce these regressions?

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

:jandem, :bbouvier, these bugs (SIMD.Bool, SIMD.Uint) added many new functions to the SIMD objects, and I assume that is why session restores are slower. I haven't verified this yet.

Is there some way we can initialize the SIMD objects lazily? Most webpages never access these SIMD functions, so it seems like a waste of time to configure the SIMD.* objects with all the predefined functions until they are accessed.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

Attachment: MozReview Request: Bug 1235408: Lazily resolve SIMD types; r=jandem

Review commit: https://reviewboard.mozilla.org/r/29271/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/29271/

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

This patch above defines the SIMD types (SIMD.*) lazily, using the resolve hook. Jan told me we wanted to avoid using them as much as we can, but I already (almost) made a working patch. The patch is a bit big because I added some macros to reduce boilerplate in SIMD.cpp; in case the entire patch isn't good enough, I could split it out so as to still get the refactor, which reduces the number of lines of code.

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> This patch above defines the SIMD types (SIMD.*) lazily, using the resolve
> hook. Jan told me we wanted to avoid using them as much as we can, but I
> already (almost) made a working patch.

Can you do a Try push with and without the patch (see comment 0) to see if it improves Talos?

Also, does this avoid the following issue (typical for resolve hooks):

SIMD.foo.bar;
delete SIMD.foo;
SIMD.foo -> resolve hooks adds it again

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Benjamin Bouvier [:bbouvier] from comment #4)
> > This patch above defines the SIMD types (SIMD.*) lazily, using the resolve
> > hook. Jan told me we wanted to avoid using them as much as we can, but I
> > already (almost) made a working patch.
> 
> Can you do a Try push with and without the patch (see comment 0) to see if
> it improves Talos? 
Good idea, will do!

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=3f1440b27add&newProject=try&newRevision=62e7fa67650f&framework=1

> Also, does this avoid the following issue (typical for resolve hooks):
> 
> SIMD.foo.bar;
> delete SIMD.foo;
> SIMD.foo -> resolve hooks adds it again
SIMD.foo is set as a permanent property, so delete SIMD.foo doesn't do anything.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #6)
> https://treeherder.mozilla.org/perf.html#/
> compare?originalProject=try&originalRevision=3f1440b27add&newProject=try&newR
> evision=62e7fa67650f&framework=1

Perfherder seems to like this patch.

For both of linux64 and win64, on sessionrestore, this patch doesn't only fix the regression but actually enhances the results just a bit (probably due to former regressions that we accepted to have, as SIMD is nightly only).

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

Thanks, Ben! I think this is the right approach.

I am not sure if it is necessary to be able to delete SIMD.Foo objects, but we should make sure that these objects can be replaced by polyfills.

Comment 9

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Jakob Stoklund Olesen [:jolesen] from comment #8)
> Thanks, Ben! I think this is the right approach.
> 
> I am not sure if it is necessary to be able to delete SIMD.Foo objects, but
> we should make sure that these objects can be replaced by polyfills.

These props can still be replaced (it is done in some tests, probably in asm.js/testSIMD.js to test linking of SIMD properties/operations) but can't be deleted.

Comment 10

[Jan de Mooij [:jandem]]

Comment on attachment 8703114 [details]
MozReview Request: Bug 1235408: Lazily resolve SIMD types; r=jandem

https://reviewboard.mozilla.org/r/29271/#review26939

Sorry for the delay!

::: js/src/builtin/SIMD.cpp:159
(Diff revision 1)
>  // These classes just exist to group together various properties and so on.

Nice deduplication!

Maybe change this comment a bit to make it more clear what the class name is:

// Define classes (Int8x16Defn, Int32x4Defn, etc) to group together ...

::: js/src/builtin/SIMD.cpp:311
(Diff revision 1)
> +      case SimdTypeDescr::Type:   return FillLanes< ::Type>(cx, result, args);

Can we remove the space between < and ::?

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51e26ad49ed2

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Thanks for the review! I've changed the comment; indeed we could remove the space, so did that. Also moved the #undef FOR_EACH_SIMD to be closer to the last use.

Clearing your needinfo, as this should solve the issue.

Comment 13

[Nigel Babu [:nigelb]]

Bustage!


/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:52: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:128: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:204: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:281: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:359: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:437: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:516: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:596: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:675: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:753: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:831: error: '<::' cannot begin a template-argument list [-fpermissive]
/builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.cpp:315:909: error: '<::' cannot begin a template-argument list [-fpermissive]
gmake[5]: *** [Unified_cpp_js_src2.o] Error 1
gmake[5]: *** Waiting for unfinished jobs....
gmake[4]: *** [js/src/target] Error 2
gmake[4]: *** Waiting for unfinished jobs....
gmake[3]: *** [compile] Error 2
gmake[2]: *** [default] Error 2
gmake[1]: *** [realbuild] Error 2
gmake: *** [build] Error 2

Comment 14

[Nigel Babu [:nigelb]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/18b1262883cdf2c95293d43fd8e468fe279d697f
Backed out changeset 51e26ad49ed2 (bug 1235408) for build bustage on a CLOSED TREE

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8703114 [details]
MozReview Request: Bug 1235408: Lazily resolve SIMD types; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29271/diff/1-2/

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Benjamin Bouvier [:bbouvier] from comment #12)
> indeed we could remove the space, so did that. 

(In reply to Nigel Babu [:nigelb] from comment #13)
> Bustage!
> 
> 
> /builds/slave/m-in-lx-0000000000000000000000/build/src/js/src/builtin/SIMD.
> cpp:315:52: error: '<::' cannot begin a template-argument list [-fpermissive]

Actually I could not, with an older compiler than mine. Pushed again to mozreview and launched a try run just to double-check. Sorry for bustage.

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8703114 [details]
MozReview Request: Bug 1235408: Lazily resolve SIMD types; r=jandem

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29271/diff/2-3/

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84e9f1429125

Comment 20

[Benjamin Bouvier [:bbouvier] (inactive)]

Improvement: Mozilla-Inbound-Non-PGO - Session Restore Test - Ubuntu HW 12.04 x64 - 7.34% decrease
--------------------------------------------------------------------------------------------------
    Previous: avg 2292.258 stddev 12.519 of 12 runs up to revision 8dcf131a146d0e759263b6ada2d19e8fed55f5fd
    New     : avg 2123.972 stddev 7.182 of 12 runs since revision 84e9f142912599540e4b32e9055cddd33cc8c969
    Change  : -168.287 (7.34% / z=13.442)
    Graph   : http://mzl.la/1mRv2dD

Improvement: Mozilla-Inbound-Non-PGO - Session Restore Test - WINNT 6.2 x64 - 5.25% decrease
--------------------------------------------------------------------------------------------
    Previous: avg 2235.762 stddev 40.377 of 12 runs up to revision 7b8798d105058913e623c0bbb6fc15827c2b75ea
    New     : avg 2118.416 stddev 11.405 of 12 runs since revision 84e9f142912599540e4b32e9055cddd33cc8c969
    Change  : -117.346 (5.25% / z=2.906)
    Graph   : http://mzl.la/1mRuUKV

Improvement: Mozilla-Inbound-Non-PGO - Session Restore Test - WINNT 6.2 x64 (e10s) - 7.31% decrease
---------------------------------------------------------------------------------------------------
    Previous: avg 2131.802 stddev 55.413 of 12 runs up to revision 8dcf131a146d0e759263b6ada2d19e8fed55f5fd
    New     : avg 1975.882 stddev 10.984 of 12 runs since revision 84e9f142912599540e4b32e9055cddd33cc8c969
    Change  : -155.920 (7.31% / z=2.814)
    Graph   : http://mzl.la/1SKqq4O

Improvement: Mozilla-Inbound-Non-PGO - Session Restore Test - Ubuntu HW 12.04 x64 (e10s) - 7.51% decrease
---------------------------------------------------------------------------------------------------------
    Previous: avg 2276.101 stddev 11.814 of 12 runs up to revision 8dcf131a146d0e759263b6ada2d19e8fed55f5fd
    New     : avg 2105.194 stddev 8.131 of 12 runs since revision 84e9f142912599540e4b32e9055cddd33cc8c969
    Change  : -170.907 (7.51% / z=14.466)
    Graph   : http://mzl.la/1RH91ZY

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84e9f1429125

Comment 22

[Joel Maher ( :jmaher ) (UTC -8)]

all the session restore tests are fixed!  I did note a TART regression as caused by this bug
https://treeherder.allizom.org/perf.html#/graphs?series=[mozilla-inbound,b43983bbf31fc4b74b4a65775bc8eb416f6aa82e,1]&highlightedRevisions=4a967e08dff7&highlightedRevisions=bb23aa681e99&timerange=2592000

which isn't fixed- I will look into it and file another bug if it is really this changeset.  Thanks for getting all these regressions fixed!