1057393 - 5.7% regression in octane-zlib on MacOSX 32bit

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Hannes Verschore [:h4writer]]

AWFY is reporting a 5.7% regression in octane-zlib on MacOSX 32bit
http://arewefastyet.com/#machine=11&view=single&suite=octane&subtest=zlib&start=1408594811&end=1408670507

The regression range is:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=1cbd24c6e684&tochange=604c002aeeb0

Can you take a look terrence?

Comment 1

[Terrence Cole [:terrence]]

I've reproduced locally. Bisecting the patch now.

Comment 2

[Terrence Cole [:terrence]]

Attachment: fix_calloc_regression-v0.diff

I am dumb: calloc != malloc+memset(0) when we are just shimming to the system's calloc. This fixes the current regression, a second one in pod_calloc_with_extra, a warning in jsgc.cpp, and shares even more code so that we can keep our current svelt size.

Comment 3

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8477466 [details] [diff] [review]
fix_calloc_regression-v0.diff

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

::: js/src/vm/MallocProvider.h
@@ +59,5 @@
>  
>      template <class T>
>      T *pod_malloc(size_t numElems) {
> +        MOZ_ASSERT(numElems > 0);
> +        return pod_calloc_with_extra<T, T>(numElems - 1);

You meant pod_malloc_with_extra here, right?

Comment 4

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4f7c6bf2b8fe

(In reply to Steve Fink [:sfink] from comment #3)
> Comment on attachment 8477466 [details] [diff] [review]
> fix_calloc_regression-v0.diff
> 
> Review of attachment 8477466 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/vm/MallocProvider.h
> @@ +59,5 @@
> >  
> >      template <class T>
> >      T *pod_malloc(size_t numElems) {
> > +        MOZ_ASSERT(numElems > 0);
> > +        return pod_calloc_with_extra<T, T>(numElems - 1);
> 
> You meant pod_malloc_with_extra here, right?

Indeed I did! Thanks for the catch.

Comment 5

[Terrence Cole [:terrence]]

Backed out for bustage. Does appear to have fixed the regression in the meantime though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3d4bac7468

Comment 6

[Hannes Verschore [:h4writer]]

What is the status of this? Merge-day is today.
So we might want to get this fixed so we don't regress zlib performance in FF34.

Comment 7

[Terrence Cole [:terrence]]

Turns out we have sites that want 0-sized malloc, so we need to not share code with the _with_extra variants.

https://tbpl.mozilla.org/?tree=Try&rev=b598cf3d889c
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd8609eb0179

Comment 8

[Luke Wagner [:luke]]

This fixed produced a 1.9% speedup, which is less than the 5.7% regression.  However, just recently there was some upgrade to the Octane harness which gave all engines a big boost, so it's hard to compare apples to apples.  Is there an easy way we can run this fix with the old harness to confirm it fully fixes the regression?

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bd8609eb0179

Comment 10

[Terrence Cole [:terrence]]

(In reply to Luke Wagner [:luke] from comment #8)
> This fixed produced a 1.9% speedup, which is less than the 5.7% regression. 
> However, just recently there was some upgrade to the Octane harness which
> gave all engines a big boost, so it's hard to compare apples to apples.  Is
> there an easy way we can run this fix with the old harness to confirm it
> fully fixes the regression?

When I landed it the first time, it did fully fix the regression.

Comment 11

[Hannes Verschore [:h4writer]]

(In reply to Luke Wagner [:luke] from comment #8)
> This fixed produced a 1.9% speedup, which is less than the 5.7% regression. 
> However, just recently there was some upgrade to the Octane harness which
> gave all engines a big boost, so it's hard to compare apples to apples.  Is
> there an easy way we can run this fix with the old harness to confirm it
> fully fixes the regression?

It made it indeed hard to see if this is totally fixed. I triggered a new compile of the "before regression" revision with "octane 2.0.1" which will give us a number to compare.

Comment 12

[Hannes Verschore [:h4writer]]

This indeed looks fixed. Thanks terrence.