Closed Bug 1379139 Opened 7 years ago Closed 7 years ago

32.36% glvideo Mean tick time across 100 ticks: (osx-cross) regression on push b84a4439260322c260fc272e3b8a3537b616d3f1 (Fri Jul 7 2017)

Categories

(Core :: Memory Allocator, defect)

Unspecified
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox-esr52 --- unaffected
firefox54 --- unaffected
firefox55 --- unaffected
firefox56 --- fixed

People

(Reporter: igoldan, Assigned: glandium)

References

Details

(Keywords: perf, regression, talos-regression, Whiteboard: [MemShrink:P1])

Attachments

(1 file)

Talos has detected a Firefox performance regression from push:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=b84a4439260322c260fc272e3b8a3537b616d3f1

As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

 32%  glvideo Mean tick time across 100 ticks:  osx-cross opt e10s     20.40 -> 27.01


You can find links to graphs and comparison views for each of the above tests at: https://treeherder.mozilla.org/perf.html#/alerts?id=7737

On the page above you can see an alert for each affected platform as well as a link to a graph showing the history of scores for this test. There is also a link to a treeherder page showing the Talos jobs in a pushlog format.

To learn more about the regressing test(s), please see: https://wiki.mozilla.org/Buildbot/Talos/Tests

For information on reproducing and debugging the regression, either on try or locally, see: https://wiki.mozilla.org/Buildbot/Talos/Running

*** Please let us know your plans within 3 business days, or the offending patch(es) will be backed out! ***

Our wiki page outlines the common responses and expectations: https://wiki.mozilla.org/Buildbot/Talos/RegressionBugsHandling
Blocks: 1378658
:glandium This is related to bug 1378658. Could you look over it and estimate a fix time for the regression?
Flags: needinfo?(mh+mozilla)
Component: Untriaged → Memory Allocator
Product: Firefox → Core
Another regression showed up and it's related to this bug:

 11%  basic_compositor_video summary osx-cross opt e10s                10.89 -> 12.11

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7737
Assignee: nobody → mh+mozilla
Flags: needinfo?(mh+mozilla)
Comment on attachment 8885052 [details]
Bug 1379139 - Instead of not recycling huge chunks, zero them.

https://reviewboard.mozilla.org/r/155918/#review161032

::: memory/mozjemalloc/mozjemalloc.cpp:1927
(Diff revision 1)
>          MOZ_ASSERT(ret);
>          return (ret);
>  }
>  
>  bool
> -pages_purge(void *addr, size_t length)
> +pages_purge(void *addr, size_t length, bool force)

Please add a comment about the meaning of |force|. And would |force_zero| be a better name?
Attachment #8885052 - Flags: review?(n.nethercote) → review+
(In reply to Nicholas Nethercote [:njn] from comment #4)
> Comment on attachment 8885052 [details]
> Bug 1379139 - Instead of not recycling huge chunks, zero them.
> 
> https://reviewboard.mozilla.org/r/155918/#review161032
> 
> ::: memory/mozjemalloc/mozjemalloc.cpp:1927
> (Diff revision 1)
> >          MOZ_ASSERT(ret);
> >          return (ret);
> >  }
> >  
> >  bool
> > -pages_purge(void *addr, size_t length)
> > +pages_purge(void *addr, size_t length, bool force)
> 
> Please add a comment about the meaning of |force|. And would |force_zero| be
> a better name?

I'll change the variable name before landing, and will leave the comment to a rebased bug 1360772.
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/autoland/rev/729a49478e7a
Instead of not recycling huge chunks, zero them. r=njn
https://hg.mozilla.org/mozilla-central/rev/729a49478e7a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Great news: the last pushed resolved the regression. Thank you!

== Change summary for alert #7822 (as of July 11 2017 04:01 UTC) ==

Improvements:

 23%  glvideo Mean tick time across 100 ticks:  osx-cross opt e10s     27.34 -> 20.95

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7822
:glandium Please also look over the 2nd regression I mentioned in comment 2.
Flags: needinfo?(mh+mozilla)
:glandium There is alot of noise for the basic_compositor_video alert. I want to know if your push has anything to do with it, or actually the previous changeset, from bug 1367900, right before bug 1378658, may be the source of the regression.

I did many retriggers now, and looks like I may have mistaken comment 2.
Setting Beta/ESR52 to affected for now so it doesn't fall off the radar should bug 1378658 end up getting uplifted.
(In reply to Ionuț Goldan [:igoldan], Performance Sheriffing from comment #10)
> :glandium Please also look over the 2nd regression I mentioned in comment 2.

It went down with the same push.
Flags: needinfo?(mh+mozilla)
:glandium Great to see! Thanks
Just for the record, bug 1378658 caused this AWSY regresssion also:

== Change summary for alert #7739 (as of July 07 2017 00:22 UTC) ==

Regressions:

108%  Resident Memory summary osx-cross opt      691,257,509.89 -> 1,434,526,909.55

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7739


But the fix pushed (from comment 7), resolved it too:

== Change summary for alert #7823 (as of July 11 2017 04:01 UTC) ==

Improvements:

 53%  Resident Memory summary osx-cross opt      1,437,716,343.72 -> 682,446,716.37

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=7823
Whiteboard: [MemShrink:P1]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: