723044 - Don't trigger moz_hosts frecency update when updating frecency on idle

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

In idle we update frecency for ALL pages. This is an expensive operation that a new frecency implementation like the one suggested in bug 704025 would solve, though, for now we can't avoid that.
The problem this is trying to solve is that, on frecency update, we now go updating the moz_hosts frecencies, and doing this when we update ALL the frecencies values may be quite expensive. if it takes a long time, it may cause larger brackets, where thread contention may happen.

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: patch v1.0

The right long-term fix is bug 704025, in the meanwhile we have to keep updating all frecencies on idle.
This patch adds barriers to the frecency trigger, so that we update the host frecency only if a page frecency changes significantly (more than 5%). So, on idle we practically skip the useless complete update of all the frecencies.

This should fix all the recent frecency on idle hangs we saw, and should be ported to Aurora asap.

Comment 2

[Dietrich Ayala (:dietrich)]

Comment on attachment 594689 [details] [diff] [review]
patch v1.0

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

r=me, given that existing tests cover that ordering stays correct and that frecencies get updated often enough.

Comment 3

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8656d37fcebf

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/8656d37fcebf

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 594689 [details] [diff] [review]
patch v1.0

[Approval Request Comment]
Regression caused by (bug #): bug 566489
User impact if declined: Long UI hangs on idle, that also means watching videos and such.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): the patch reduces the amount of work we do by skipping some useless updates. risk is low and the affected feature is disabled for now
String changes made by this patch: no

Comment 6

[Alex Keybl [:akeybl]]

(In reply to Marco Bonardo [:mak] from comment #5)
> Risk to taking this patch (and alternatives if risky): the patch reduces the
> amount of work we do by skipping some useless updates. risk is low and the
> affected feature is disabled for now

If the feature is currently disabled, I'm not sure we need to fix on Aurora. Are there plans to enable for FF12, or have we promoted the pref externally?

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to Alex Keybl [:akeybl] from comment #6)
> (In reply to Marco Bonardo [:mak] from comment #5)
> > Risk to taking this patch (and alternatives if risky): the patch reduces the
> > amount of work we do by skipping some useless updates. risk is low and the
> > affected feature is disabled for now
> 
> If the feature is currently disabled, I'm not sure we need to fix on Aurora.
> Are there plans to enable for FF12, or have we promoted the pref externally?

the feature is disabled, but the part of the backend collecting data is not, for data coherence reasons, the fix is on that part.

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

To clarify, the fix is NEEDED on Aurora because only the frontend part of the feature is disabled, we cannot disable the backend part, that is what causes this bug.

Comment 10

[Alex Keybl [:akeybl]]

Comment on attachment 594689 [details] [diff] [review]
patch v1.0

[Triage Comment]
Thanks for the background - approved for Aurora 12.

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/96584d3595fa