Closed Bug 1057393 Opened 5 years ago Closed 5 years ago

5.7% regression in octane-zlib on MacOSX 32bit

Categories

(Core :: JavaScript: GC, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: h4writer, Assigned: terrence)

References

Details

(Keywords: regression)

Attachments

(1 file)

Flags: needinfo?(terrence)
I've reproduced locally. Bisecting the patch now.
Assignee: nobody → terrence
Blocks: 1033442
Flags: needinfo?(terrence)
Keywords: regression
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.
Attachment #8477466 - Flags: review?(sphink)
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?
Attachment #8477466 - Flags: review?(sphink) → review+
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.
Backed out for bustage. Does appear to have fixed the regression in the meantime though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bb3d4bac7468
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.
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
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?
Flags: needinfo?(hv1989)
https://hg.mozilla.org/mozilla-central/rev/bd8609eb0179
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
(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.
(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.
This indeed looks fixed. Thanks terrence.
Flags: needinfo?(hv1989)
You need to log in before you can comment on or make changes to this bug.