Last Comment Bug 777174 - 3ms SunSpider regression on date-format-tofte
: 3ms SunSpider regression on date-format-tofte
Status: RESOLVED FIXED
[js:p1:fx17][qa-]
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal with 2 votes (vote)
: mozilla17
Assigned To: Jim Blandy :jimb
:
:
Mentors:
Depends on: 783505
Blocks: 773850 776233
  Show dependency treegraph
 
Reported: 2012-07-24 17:11 PDT by David Mandelin [:dmandelin]
Modified: 2012-10-16 16:30 PDT (History)
11 users (show)
jimb: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
unaffected
unaffected
+
fixed


Attachments
Change CallNonGenericMethod to take the predicate and implementation as template arguments, not function arguments. (50.65 KB, patch)
2012-08-03 17:17 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Change CallNonGenericMethod to take the predicate and implementation as template arguments, not function arguments. (50.61 KB, patch)
2012-08-06 18:07 PDT, Jim Blandy :jimb
luke: review+
Details | Diff | Splinter Review

Description David Mandelin [:dmandelin] 2012-07-24 17:11:15 PDT
Following backout/disabling of all the recent perf regressions, there is still a 3ms regression on SunSpider in ba804e7c1d64 relative to rev 7221c50cb5b4. Careful investigation found it to be a 3ms regression, which seems to mostly on tofte, due to this changeset:

changeset:   99484:5d00c508b09a
user:        Jeff Walden <jwalden@mit.edu>
date:        Tue Jul 03 17:44:22 2012 -0700
summary:     Bug 773850 - Refactor method guarding to be able to work for methods that must be able to accept a |this| which is a proxy.  r=luke

Ideally, we'd back it out, but that's nontrivial at this point. Maybe Jeff knows how to back it out, but failing that, we need to get it fixed.
Comment 1 Luke Wagner [:luke] 2012-07-24 21:37:22 PDT
I looked into this and date-format-tofte does a lot of calls into, not suprisingly, date natives.  The profile shows that the fast path (where the impl is called directly after the test succeeds) is not being inlined.  (In bug 773850 comment 2 I requested that this be investigated...)

A microbenchmark that just calls getFullYear gets about 10% slower.  Adding JS_ALWAYS_INLINE to date_getFullYear_impl has no effect (at least on OSX 10.6, gcc 4.2).  Changing CallNonGenericMethod to be templated on the the two function pointers does recover all the perf, so that seems like what we have to do.  (This is rather unfortunate b/c it requires modifying every call to CallNonGenericMethod (again).  Also, apparently function pointers used as template arguments must have external linkage, which is also annoying.)
Comment 2 Jim Blandy :jimb 2012-08-01 18:16:46 PDT
I can reproduce the regression with the following microbenchmark:

var iter = 1000000;
var start = Date.now();
var date = new Date(start);
for (var i = 0; i < iter; i++)
  date.getFullYear();
var end = Date.now();
print("" + (end - start) + " " + (end - start)/iter);

In the prior changeset, each loop iteration takes 12.17us (stdev 147ns): 

changeset:   99484:91f65d802d58
user:        Brian R. Bondy <netzen@gmail.com>
date:        Mon Jul 16 19:10:18 2012 -0400
summary:     Bug 770122 - Crash on websites withs ICO containing invalid GIFs. r=joe.

In the regressing changeset, each loop iteration takes 12.49us (stdev 140ns):

changeset:   99485:5d00c508b09a
user:        Jeff Walden <jwalden@mit.edu>
date:        Tue Jul 03 17:44:22 2012 -0700
summary:     Bug 773850 - Refactor method guarding to be able to work for methods that must be able to accept a |this| which is a proxy.  r=luke
Comment 3 Jim Blandy :jimb 2012-08-03 15:34:49 PDT
I've got a patch now that, for jsdate only: templatizes; gives the impls external linkage but puts them in an anonymous namespace; and marks them to be always inlined. It seems to gain back the performance.
Comment 4 Jim Blandy :jimb 2012-08-03 15:35:24 PDT
... in full Sunspider, not just the microbenchmark.
Comment 5 Jim Blandy :jimb 2012-08-03 17:17:21 PDT
Created attachment 648913 [details] [diff] [review]
Change CallNonGenericMethod to take the predicate and implementation as template arguments, not function arguments.

On x86_64 Fedora 17, this patch yields an 8ms (1.6%) speedup on SunSpider
compared to the tree just prior to the introduction of
CallNonGenericMethod, or a ~12ms (2.8%) speedup compared to the tree just
after the introduction. It changes only those uses of CallNonGenericMethod
in the js/src tree; I'll complete the patch next.

The changes to jstypedarray.cpp were larger than I expected; it seems that
one must qualify static member functions with the class name
("ThisTypeArray") in order to pass them as a template argument. I can't
imagine the reason for this requirement, lest G++ say something like this:

jstypedarray.cpp:1553:86: error: ‘static bool TypedArrayTemplate<NativeType>::IsThisClass(const JS::Value&) [with NativeType = signed char]’ cannot appear in a constant-expression
Comment 6 Jim Blandy :jimb 2012-08-03 17:18:25 PDT
Bad edit; I meant:

...

The changes to jstypedarray.cpp were larger than I expected; it seems that
one must qualify static member functions with the class name
("ThisTypeArray") in order to pass them as a template argument, lest G++
say something like this:

jstypedarray.cpp:1553:86: error: ‘static bool TypedArrayTemplate<NativeType>::IsThisClass(const JS::Value&) [with NativeType = signed char]’ cannot appear in a constant-expression

I can't imagine the reason for this requirement.
Comment 7 Luke Wagner [:luke] 2012-08-03 17:28:47 PDT
It would be kinda nice to not wrap everything in big namespace blocks, for a few reasons:
 - iirc, it is harder to put a breakpoint on it by name in gdb
 - if you want to define something in js::, you have to close the unnamed namespace, open js, then close js, open the unnamed namespace.  This is what annoyed me about wrapping whole .cpps in namespace js {}

Instead, I think it would be fine just to take off the 'static' (thereby making them plain old external symbols).  It's less abstractly clean, but (1) without JS_PUBLIC_API, this won't slow down dynamic linking/loading (2) in the unlikely event we were to have a linker name collision we could easily deal with it.
Comment 8 David Mandelin [:dmandelin] 2012-08-06 16:42:34 PDT
(In reply to Jim Blandy :jimb from comment #5)
> Created attachment 648913 [details] [diff] [review]
> Change CallNonGenericMethod to take the predicate and implementation as
> template arguments, not function arguments.
> 
> On x86_64 Fedora 17, this patch yields an 8ms (1.6%) speedup on SunSpider
> compared to the tree just prior to the introduction of
> CallNonGenericMethod, or a ~12ms (2.8%) speedup compared to the tree just
> after the introduction. It changes only those uses of CallNonGenericMethod
> in the js/src tree; I'll complete the patch next.

Very cool. It turns out that on AWFY, we were 12ms slower than V8 just after the introduction, but I had no idea why.
Comment 9 Jim Blandy :jimb 2012-08-06 18:07:30 PDT
Created attachment 649501 [details] [diff] [review]
Change CallNonGenericMethod to take the predicate and implementation as template arguments, not function arguments.

On x86_64 Fedora 17, this patch yields an 8ms (1.6%) speedup on SunSpider compared to the tree just prior to the introduction of CallNonGenericMethod, or a ~12ms (2.8%) speedup compared to the tree just after the introduction.

The changes to jstypedarray.cpp were larger than I expected; it seems that one must qualify static member functions with the class name ("ThisTypeArray") in order to pass them as a template argument, lest G++ say something like this:

jstypedarray.cpp:1553:86: error: ‘static bool TypedArrayTemplate<NativeType>::IsThisClass(const JS::Value&) [with NativeType = signed char]’ cannot appear in a constant-expression

I can't imagine the reason for this requirement.
Comment 10 Jim Blandy :jimb 2012-08-06 18:08:49 PDT
Sorry --- I should say: this version of the patch addresses comment 7 (I checked that the ELF symbols are still marked HIDDEN), and covers the (very few) uses outside js/src.
Comment 11 Jim Blandy :jimb 2012-08-06 18:20:30 PDT
Try: https://tbpl.mozilla.org/?tree=Try&rev=ee66069fab3b
Comment 12 Jim Blandy :jimb 2012-08-06 18:24:51 PDT
Comment on attachment 649501 [details] [diff] [review]
Change CallNonGenericMethod to take the predicate and implementation as template arguments, not function arguments.

Test failures on accessing float typed arrays.
Comment 13 Jim Blandy :jimb 2012-08-07 18:05:54 PDT
The failures accessing float-point typed arrays are unrelated to this patch: they occur even with this patch not applied, on M-C changesets going back at least as far as April. The tests pass in the same configuration on the TBPL build machines. Filed as bug 781064.
Comment 14 Jim Blandy :jimb 2012-08-10 14:37:34 PDT
Patch revised. Try:

https://tbpl.mozilla.org/?tree=Try&rev=a0366ff59304
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-08-16 17:57:12 PDT
https://hg.mozilla.org/mozilla-central/rev/62342ad8d7fc
Comment 17 David Mandelin [:dmandelin] 2012-08-17 15:23:47 PDT
I don't see a change on AWFY. Any idea why not?
Comment 18 Landry Breuil (:gaston) 2012-08-19 02:33:00 PDT
It seems (with hints from ms2ger) that commit broke my gcc 4.2 builds with :

js/src/jstypedarray.cpp:1427: error: template parameter 'ValueGetter' of type 'JS::Value (*)(JSObject*)' is not allowed in an integral constant expression because it is not of integral or enumeration type

Does that mean that 4.2 is now "officially" unsupported ? Just askin'... if it needs fixin on m-c side, or i now have to switch to a "better" compiler.
Comment 19 Jonathan Kew (:jfkthame) 2012-08-20 05:23:51 PDT
As of now, gcc 4.2 is still the default compiler for mozilla-central on darwin (see build/autoconf/compiler-opts.m4). That's now broken.
Comment 20 Jim Blandy :jimb 2012-09-07 12:38:00 PDT
(In reply to Jonathan Kew (:jfkthame) from comment #19)
> As of now, gcc 4.2 is still the default compiler for mozilla-central on
> darwin (see build/autoconf/compiler-opts.m4). That's now broken.

This was fixed by bug 783505.
Comment 21 Jim Blandy :jimb 2012-09-07 12:42:33 PDT
(In reply to David Mandelin [:dmandelin] from comment #17)
> I don't see a change on AWFY. Any idea why not?

No --- that's too bad.

The whole thing is compiler pacification from beginning to end, so I'm not surprised to see mixed results. :(

Perhaps the compiler used for AWFY isn't responsive to this fix.

Perhaps the compiler used for AWFY wasn't sensitive to the regression to begin with; in the original bug Waldo said he had benchmarked and seen no effect.

Note You need to log in before you can comment on or make changes to this bug.