1818881 - Evaluate an alternative to polling for frecency recalcs

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P3)

Description

[Marco Bonardo [:mak]]

Even if we tried to limit the impact, polling still has an energy impact.
An alternative may be

1. Store a time instead of a boolean
2. when recalc_frecency changes, check if current_time - stored_time > 1 minute, if so dispatch a runnable to the main-thread
3. set stored_time = current_time
4. the main thread runnable uses a 1 minute timer before starting the work.

This will be a bit more expensive every 1 minute to dispatch, and we need an efficient way to get the current time. Overall it may not be horrible.

Comment 1

[Marco Bonardo [:mak]]

Using PR_IntervalNow() may be reasonable for this use case.

Comment 2

[Florian Quèze [:florian]]

Thanks for looking into this!

I don't understand why this code will need to store the current time, but I'm not sure I understand the current behavior completely, so I'll try to explain what I understand, and if I'm missing something you can explain it.

• Whenever something (eg. a page load?) happens, nsNavHistory::sShouldStartFrecencyRecalculation is set to true in SetShouldStartFrecencyRecalculationFunction::OnFunctionCall (and that could happen from multiple threads).
• A repeating 1 minute timer checks the value of sShouldStartFrecencyRecalculation and if it is true, arms a 2 minutes deferred task and sets sShouldStartFrecencyRecalculation back to false.
• Actual work is performed when the deferred task executes, in chunks of limited size. If work is left after processing a chunk, the task is re-armed.

Assuming all the above is accurate, would the following simplification work?

1. In SetShouldStartFrecencyRecalculationFunction::OnFunctionCall, instead of nsNavHistory::sShouldStartFrecencyRecalculation = true;, do if (nsNavHistory::sShouldStartFrecencyRecalculation.exchange(true) == false) /* dispatch a(n idle) main thread runnable */;
2. From that main thread runnable, arm the deferred task.
3. When recalculateSomeFrecencies has finished a chunk, queue processing of the next chunk immediately in an idle runnable.
4. Set nsNavHistory::sShouldStartFrecencyRecalculation back to false only when recalculateSomeFrecencies has determined there's nothing left to recalculate.

It might be possible to simplify even more by replacing "dispatch a(n idle) main thread runnable" in #1 with "start a 3 minutes timer targeting the main thread" and then get rid of the deferred task entirely. But that would likely be more refactoring for very little actual improvements.

Comment 3

[Marco Bonardo [:mak]]

(In reply to Florian Quèze [:florian] from comment #2)

Thanks for looking into this!

I don't understand why this code will need to store the current time

Pretty much just an idea to limit the number of runnables dispatched (max 1 runnable every minute).

1. In SetShouldStartFrecencyRecalculationFunction::OnFunctionCall, instead of nsNavHistory::sShouldStartFrecencyRecalculation = true;, do if (nsNavHistory::sShouldStartFrecencyRecalculation.exchange(true) == false) /* dispatch a(n idle) main thread runnable */;
2. From that main thread runnable, arm the deferred task.
3. When recalculateSomeFrecencies has finished a chunk, queue processing of the next chunk immediately in an idle runnable.
4. Set nsNavHistory::sShouldStartFrecencyRecalculation back to false only when recalculateSomeFrecencies has determined there's nothing left to recalculate.

This may work, the only downside I can think of is that between the time we complete the calculation, and the time we set the bool to false, there may be some time where sShouldStartFrecencyRecalculation is still true and a further try to set it to true will be ignored.
In practice it may not be a problem, it just means we'll wait for the next operation affecting frecency or the next session in the worst case, to recalculate.

It might be possible to simplify even more by replacing "dispatch a(n idle) main thread runnable" in #1 with "start a 3 minutes timer targeting the main thread" and then get rid of the deferred task entirely.

The scope of the deferred task is not just to run after N minutes, but also to try to run during idle intervals, if possible (Ah I see, we could dispatch an idle runnable and be done with it... I still prefer DeferredTask for the simplicity to handle it on js side).

I like your idea so let me try it.

Comment 4

[Marco Bonardo [:mak]]

Attachment: Bug 1818881 - Avoid polling for frecency recalcs. r=florian,daisuke

Rather than checking every minute for work to do, use .exchange() to check
if sShouldStartFrecencyRecalculation switches to true and if so dispatch a
runnable to notify PlacesFrecencyRecalculator. The latter sets back
sShouldStartFrecencyRecalculation to false once the recalculation is complete.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6596733ddc41
Avoid polling for frecency recalcs. r=florian,daisuke

Comment 6

[Sandor Molnar[:smolnar]]

Backed out for causing for causing xpc failures in toolkit/components/search/tests/xpcshell/test_searchSuggest_cookies.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/af5417c4556aa4e16a82d4f0d5be42652be36cf8

Push with failures

Failure log: TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/search/tests/xpcshell/test_searchSuggest_cookies.js | setup - [setup : 1] Unexpected console message received during test: Unknown "places-init-complete" topic - false == true

Comment 7

[Marco Bonardo [:mak]]

ah yes, I was overzealous with an error check, it may actually happen in reality due to how categories work.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/838e43449b31
Avoid polling for frecency recalcs. r=florian,daisuke

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/838e43449b31