3ms SunSpider regression on date-format-tofte

RESOLVED FIXED in Firefox 17

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dmandelin, Assigned: jimb)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox14 unaffected, firefox15 unaffected, firefox16 unaffected, firefox17+ fixed)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → unaffected
status-firefox17: --- → affected
tracking-firefox17: --- → +
(Reporter)

Updated

5 years ago
Blocks: 776233

Comment 1

5 years ago
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]
(Assignee)

Updated

5 years ago
QA Contact: jimb
(Assignee)

Updated

5 years ago
Assignee: general → jimb
QA Contact: jimb
(Assignee)

Comment 2

5 years ago
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
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
... in full Sunspider, not just the microbenchmark.
(Assignee)

Comment 5

5 years ago
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
Attachment #648913 - Flags: feedback?(luke)
(Assignee)

Comment 6

5 years ago
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

5 years ago
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.
(Reporter)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
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.
Attachment #648913 - Attachment is obsolete: true
Attachment #648913 - Flags: feedback?(luke)
Attachment #649501 - Flags: review?(luke)
(Assignee)

Comment 10

5 years ago
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.
(Assignee)

Comment 11

5 years ago
Try: https://tbpl.mozilla.org/?tree=Try&rev=ee66069fab3b
(Assignee)

Comment 12

5 years ago
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)
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
Patch revised. Try:

https://tbpl.mozilla.org/?tree=Try&rev=a0366ff59304
(Assignee)

Updated

5 years ago
Attachment #649501 - Flags: review?(luke)

Updated

5 years ago
Attachment #649501 - Flags: review?(luke) → review+
(Assignee)

Comment 15

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
status-firefox17: affected → fixed
Depends on: 783505
(Reporter)

Comment 17

5 years ago
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.
(Assignee)

Comment 20

5 years ago
(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.
(Assignee)

Comment 21

5 years ago
(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.