Last Comment Bug 731398 - Realtime raytracer regression
: Realtime raytracer regression
Status: RESOLVED FIXED
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla14
Assigned To: Brian Hackett (:bhackett)
:
: Jason Orendorff [:jorendorff]
Mentors:
http://29a.ch/2010/6/2/realtime-raytr...
Depends on: 755750
Blocks: 744727
  Show dependency treegraph
 
Reported: 2012-02-28 13:48 PST by Gregor Wagner [:gwagner]
Modified: 2012-05-17 13:35 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
wontfix
-
wontfix


Attachments
patch (24.45 KB, patch)
2012-03-14 07:45 PDT, Brian Hackett (:bhackett)
dvander: review+
Details | Diff | Splinter Review

Description Gregor Wagner [:gwagner] 2012-02-28 13:48:13 PST
I see 2 regressions here.
First, from the current FF release version to Aurora 12, FPS drops from about 28 to 18 on my MP.

Second, From Aurora to Nightly the GC pause time seems to increase.
The GC log shows only *RESET* GCs.
Comment 1 Bill McCloskey (:billm) 2012-02-28 14:05:14 PST
Resets definitely cause longer GCs. I have a patch in bug 731052 that should at least show why the resets are happening.
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-02-29 07:51:03 PST
Gregor, how do beta builds do?
Comment 3 Gregor Wagner [:gwagner] 2012-02-29 10:00:11 PST
Beta (FF11) gets about 22 FPS and around 80ms GC pause time.
Nightly gets 18 FPS and the average maxGC pause time of 135ms.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-02-29 11:03:22 PST
OK, so we have an fps regression on beta too...
Comment 5 Alex Keybl [:akeybl] 2012-02-29 11:52:51 PST
Tracking for FF11 and FF12. Since this is very late in Beta 11, how can we come to an understanding of the total user effect of this issue? Presumably a 50% regression in some JS is the worst case here - but how can we get to an understanding of what that set is?
Comment 6 Terrence Cole [:terrence] 2012-02-29 15:38:30 PST
The fps regression range is 9740118b9dcc:c2102c45c8da.  This is ObjShrink: flagging Brian.  I will open a new bug for the GC pause times.
Comment 7 Brian Hackett (:bhackett) 2012-03-14 07:45:49 PDT
Created attachment 605748 [details] [diff] [review]
patch

Assign singleton types to object literals created outside loops in global/eval scripts.  This is a fix that is also necessary for good v8-raytrace perf (part of the patch in bug 638794).  Improves my FPS from ~20 to ~30.  Without this the vector objects which are continuously created by this page do not have a prototype with singleton type, which inhibits optimization of CreateThis.  CreateThis is slower post-objshrink due to the need to do hashtable lookups to get data which was previously clogging up JSObject itself.
Comment 8 David Mandelin [:dmandelin] 2012-03-14 10:34:56 PDT
Thanks for the prompt fix! Very cool that it will also help v8-raytrace.
Comment 9 David Anderson [:dvander] 2012-03-14 17:48:42 PDT
Comment on attachment 605748 [details] [diff] [review]
patch

Review of attachment 605748 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsinferinlines.h
@@ +576,5 @@
> +    if (!cx->typeInferenceEnabled())
> +        return true;
> +
> +    if (UseNewTypeForInitializer(cx, script, pc)) {
> +        obj->setSingletonType(cx);

Does this need to propagate a false return?
Comment 10 Brian Hackett (:bhackett) 2012-03-21 06:38:18 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/149eff9b7b92
Comment 11 Marco Bonardo [::mak] 2012-03-22 06:22:14 PDT
https://hg.mozilla.org/mozilla-central/rev/149eff9b7b92
Comment 12 Alex Keybl [:akeybl] 2012-03-28 17:00:07 PDT
This is tracked for FF12 - is there any reason to continue to do so? If we'd like to uplift to Aurora or Beta, please nominate with a risk/reward evaluation.
Comment 13 David Mandelin [:dmandelin] 2012-03-28 17:08:32 PDT
(In reply to Alex Keybl [:akeybl] from comment #12)
> This is tracked for FF12 - is there any reason to continue to do so? If we'd
> like to uplift to Aurora or Beta, please nominate with a risk/reward
> evaluation.

AFAIK, it significantly affects exactly one site, a raytracer demo.

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