731398 - Realtime raytracer regression

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gregor Wagner [:gwagner]]

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 [inactive unless it's an emergency] (:billm)]

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 [:bzbarsky]]

Gregor, how do beta builds do?

Comment 3

[Gregor Wagner [:gwagner]]

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 [:bzbarsky]]

OK, so we have an fps regression on beta too...

Comment 5

[Alex Keybl [:akeybl]]

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]]

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 [Laid off!]]

Attachment: 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]]

Thanks for the prompt fix! Very cool that it will also help v8-raytrace.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/149eff9b7b92

Comment 11

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/149eff9b7b92

Comment 12

[Alex Keybl [:akeybl]]

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]]

(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.