Closed Bug 777174 Opened 12 years ago Closed 12 years ago

3ms SunSpider regression on date-format-tofte

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla17
Tracking Status
firefox14 --- unaffected
firefox15 --- unaffected
firefox16 --- unaffected
firefox17 + fixed

People

(Reporter: dmandelin, Assigned: jimb)

References

Details

(Whiteboard: [js:p1:fx17][qa-])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 776233
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.)
Whiteboard: [js:p1] → [js:p1:fx17]
QA Contact: jimb
Assignee: general → jimb
QA Contact: jimb
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
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.
Status: NEW → ASSIGNED
... in full Sunspider, not just the microbenchmark.
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
Attachment #648913 - Flags: feedback?(luke)
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.
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.
(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.
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.
Attachment #648913 - Attachment is obsolete: true
Attachment #648913 - Flags: feedback?(luke)
Attachment #649501 - Flags: review?(luke)
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 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.
Attachment #649501 - Flags: review?(luke)
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.
Attachment #649501 - Flags: review?(luke)
Attachment #649501 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/62342ad8d7fc
Flags: in-testsuite-
OS: Windows 7 → All
Hardware: x86_64 → All
Target Milestone: --- → mozilla17
https://hg.mozilla.org/mozilla-central/rev/62342ad8d7fc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 783505
I don't see a change on AWFY. Any idea why not?
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.
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.
(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.
(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.
Whiteboard: [js:p1:fx17] → [js:p1:fx17][qa-]
You need to log in before you can comment on or make changes to this bug.