Bugzilla

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

I like your idea so let me try it.

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