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

Status: RESOLVED FIXED

(Core :: Memory Allocator, defect)

Description

[Ionuț Goldan [:igoldan]]

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

Comment 1

[Ionuț Goldan [:igoldan]]

:glandium This is related to bug 1378658. Could you look over it and estimate a fix time for the regression?

Comment 2

[Ionuț Goldan [:igoldan]]

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

Comment 3

[Mike Hommey [:glandium]]

Attachment: Bug 1379139 - Instead of not recycling huge chunks, zero them.

It turns out that not recycling some kinds of chunk can lead to the
recycle queue being starved in some scenarios. When that happens, we end
up mmap()ing new memory, but that turns out to be significantly slower.

So instead of not recycling huge chunks, we force-clean them, before
madvising so that the pages can still be reclaimed in case of memory
pressure.

Review commit: https://reviewboard.mozilla.org/r/155918/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/155918/

Comment 4

[Nicholas Nethercote [inactive]]

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?

Comment 5

[Mike Hommey [:glandium]]

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

Comment 6

[Mike Hommey [:glandium]]

Comment on attachment 8885052 [details]
Bug 1379139 - Instead of not recycling huge chunks, zero them.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/155918/diff/1-2/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/729a49478e7a
Instead of not recycling huge chunks, zero them. r=njn

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/729a49478e7a

Comment 9

[Ionuț Goldan [:igoldan]]

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

Comment 10

[Ionuț Goldan [:igoldan]]

:glandium Please also look over the 2nd regression I mentioned in comment 2.

Comment 11

[Ionuț Goldan [:igoldan]]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Setting Beta/ESR52 to affected for now so it doesn't fall off the radar should bug 1378658 end up getting uplifted.

Comment 13

[Mike Hommey [:glandium]]

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

Comment 14

[Ionuț Goldan [:igoldan]]

:glandium Great to see! Thanks

Comment 15

[Ionuț Goldan [:igoldan]]

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