Closed Bug 1235408 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: jmaher, Assigned: bbouvier)

References

Details

(Keywords: perf, regression, Whiteboard: [talos_regression])

Attachments

(1 file)

: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?
Flags: needinfo?(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.
Flags: needinfo?(jdemooij)
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.
(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
(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.
(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).
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.
Flags: needinfo?(jolesen)
See Also: → 1235412
(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.
Attachment #8703114 - Flags: review?(jdemooij) → review+
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 ::?
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.
Flags: needinfo?(jdemooij)
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 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/
Attachment #8703114 - Attachment description: MozReview Request: Bug 1235408: Lazily resolve SIMD types; r?jandem → MozReview Request: Bug 1235408: Lazily resolve SIMD types; r=jandem
(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.
Assignee: nobody → bbouvier
Status: NEW → ASSIGNED
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/
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
https://hg.mozilla.org/mozilla-central/rev/84e9f1429125
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
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!
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: