Fix and adjust PageNavigationGC threshold

Fix and adjust PageNavigationGC threshold

We originally tried to estimate removal rate by counting the (nodes of the
detaching document) + (already deatached nodes).
Page navigation threshold was scheduled even when we were navigating from
about:blank because (already deatached nodes) count was inflated.
Nodes detached from JS is already accounted for in estimatedLiveSize through
collectedWrapperCount so we didn't need this.

Add estimatedRemovateRatio threshold of 1% because PageNavigationGC is for emergencies and normal situations should be handled by idle GC.

BUG=588029
Committed: https://crrev.com/f5ff205e2de07090a8dd52397b2573368fdc3229
Cr-Commit-Position: refs/heads/master@{#378973}

[keishi, 2016-03-03 02:52:03]

keishi@chromium.org changed reviewers:
+ haraken@chromium.org, oilpan-reviews@chromium.org

[keishi, 2016-03-03 02:52:04]

Fixing the estimatedRemovalRatio calculation reduces PageNavGC from around 10 to
5. (from about:blank to sfgate.com)
The estimatedRemovalRatio threshold removes the remaining PageNavGCs.

I've run perf tests for tests that were affected by disabling PageNavigationGC
and I don't think they will be a problem this time.

## Android
page_cycler.more_js
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
speedometer
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
page_cycler.baseic_oopif
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
memory_health
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
## Win
page_cycler.more_js
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
speedometer
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
page_cycler.baseic_oopif
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
## Linux
page_cycler.more_js
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
speedometer
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...
page_cycler.baseic_oopif
http://storage.googleapis.com/chromium-telemetry/html-results/results-2016-03...

[haraken, 2016-03-03 02:57:31]

LGTM to try.

> Fixing the estimatedRemovalRatio calculation reduces PageNavGC from around 10
to
5. (from about:blank to sfgate.com)

Do you have a collection rate of the 5 page navigation GCs? If it is low, it
means that we're triggering the navigation GCs too much. If it is high, the
navigation GCs would make sense.

[keishi, 2016-03-03 03:44:46]

On 2016/03/03 02:57:31, haraken wrote:
> LGTM to try.
> 
> > Fixing the estimatedRemovalRatio calculation reduces PageNavGC from around
10
> to
> 5. (from about:blank to http://sfgate.com)
> 
> Do you have a collection rate of the 5 page navigation GCs? If it is low, it
> means that we're triggering the navigation GCs too much. If it is high, the
> navigation GCs would make sense.

They are around 4%. Even if they are high, I think normal mechanisms like
conservative GC and idle GC should do their work.

[haraken, 2016-03-03 04:40:46]

On 2016/03/03 03:44:46, keishi wrote:
> On 2016/03/03 02:57:31, haraken wrote:
> > LGTM to try.
> > 
> > > Fixing the estimatedRemovalRatio calculation reduces PageNavGC from around
> 10
> > to
> > 5. (from about:blank to http://sfgate.com)
> > 
> > Do you have a collection rate of the 5 page navigation GCs? If it is low, it
> > means that we're triggering the navigation GCs too much. If it is high, the
> > navigation GCs would make sense.
> 
> They are around 4%. Even if they are high, I think normal mechanisms like
> conservative GC and idle GC should do their work.

Does 4% mean that 96% objects survived the navigation GCs? If that is the case,
I totally agree with you that navigation GCs should not come into a play.

Either way, I'm fine with landing this CL and see how it goes.

[keishi, 2016-03-03 05:26:22]

On 2016/03/03 04:40:46, haraken wrote:
> On 2016/03/03 03:44:46, keishi wrote:
> > On 2016/03/03 02:57:31, haraken wrote:
> > > LGTM to try.
> > > 
> > > > Fixing the estimatedRemovalRatio calculation reduces PageNavGC from
around
> > 10
> > > to
> > > 5. (from about:blank to http://sfgate.com)
> > > 
> > > Do you have a collection rate of the 5 page navigation GCs? If it is low,
it
> > > means that we're triggering the navigation GCs too much. If it is high,
the
> > > navigation GCs would make sense.
> > 
> > They are around 4%. Even if they are high, I think normal mechanisms like
> > conservative GC and idle GC should do their work.
> 
> Does 4% mean that 96% objects survived the navigation GCs? If that is the
case,
> I totally agree with you that navigation GCs should not come into a play.
> 
> Either way, I'm fine with landing this CL and see how it goes.

Yes. I'll go ahead and try this.

[keishi, 2016-03-03 05:26:27]

The CQ bit was checked by keishi@chromium.org

[commit-bot: I haz the power, 2016-03-03 05:26:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1763623002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1763623002/1

[commit-bot: I haz the power, 2016-03-03 06:44:41]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-03-03 06:45:53]

Description was changed from

==========
Fix and adjust PageNavigationGC threshold

We originally tried to estimate removal rate by counting the (nodes of the
detaching document) + (already deatached nodes).
Page navigation threshold was scheduled even when we were navigating from
about:blank because (already deatached nodes) count was inflated.
Nodes detached from JS is already accounted for in estimatedLiveSize through
collectedWrapperCount so we didn't need this.

Add estimatedRemovateRatio threshold of 1% because PageNavigationGC is for
emergencies and normal situations should be handled by idle GC.

BUG=588029
==========

to

==========
Fix and adjust PageNavigationGC threshold

We originally tried to estimate removal rate by counting the (nodes of the
detaching document) + (already deatached nodes).
Page navigation threshold was scheduled even when we were navigating from
about:blank because (already deatached nodes) count was inflated.
Nodes detached from JS is already accounted for in estimatedLiveSize through
collectedWrapperCount so we didn't need this.

Add estimatedRemovateRatio threshold of 1% because PageNavigationGC is for
emergencies and normal situations should be handled by idle GC.

BUG=588029

Committed: https://crrev.com/f5ff205e2de07090a8dd52397b2573368fdc3229
Cr-Commit-Position: refs/heads/master@{#378973}
==========

[commit-bot: I haz the power, 2016-03-03 06:45:54]

Patchset 1 (id:??) landed as
https://crrev.com/f5ff205e2de07090a8dd52397b2573368fdc3229
Cr-Commit-Position: refs/heads/master@{#378973}