Closed
Bug 777174
Opened 12 years ago
Closed 12 years ago
3ms SunSpider regression on date-format-tofte
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
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.
Reporter | ||
Updated•12 years ago
|
status-firefox14:
--- → unaffected
status-firefox15:
--- → unaffected
status-firefox16:
--- → unaffected
status-firefox17:
--- → affected
tracking-firefox17:
--- → +
Comment 1•12 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.)
Updated•12 years ago
|
Whiteboard: [js:p1] → [js:p1:fx17]
Assignee | ||
Updated•12 years ago
|
QA Contact: jimb
Assignee | ||
Updated•12 years ago
|
Assignee: general → jimb
QA Contact: jimb
Assignee | ||
Comment 2•12 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•12 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•12 years ago
|
||
... in full Sunspider, not just the microbenchmark.
Assignee | ||
Comment 5•12 years ago
|
||
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•12 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•12 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•12 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•12 years ago
|
||
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•12 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•12 years ago
|
||
Try: https://tbpl.mozilla.org/?tree=Try&rev=ee66069fab3b
Assignee | ||
Comment 12•12 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•12 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•12 years ago
|
||
Patch revised. Try: https://tbpl.mozilla.org/?tree=Try&rev=a0366ff59304
Assignee | ||
Updated•12 years ago
|
Attachment #649501 -
Flags: review?(luke)
Updated•12 years ago
|
Attachment #649501 -
Flags: review?(luke) → review+
Assignee | ||
Comment 15•12 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
Comment 16•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/62342ad8d7fc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Updated•12 years ago
|
Reporter | ||
Comment 17•12 years ago
|
||
I don't see a change on AWFY. Any idea why not?
Comment 18•12 years ago
|
||
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•12 years ago
|
||
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•12 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•12 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•