690354 - Idle expiration never runs for clean databases

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

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

idle expiration runs just for dirty databases, thus databases over the history capacity, this means some tasks don't run for some users.
These are usually minor tasks (orphan annotations or input history, analyze), but it's preferrable to run them on idle rather than on shutdown. So I'll add a couple new expiration targets, one that runs every N timed expirations and one that runs on idle-daily.

Comment 1

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

After implementing it and thinking a bit about the approach, the second target (N timed expirations) is not much useful, since users not hitting idle are likely working in small sessions, so are as well unlikely to hit that. I'll reduce the patch to the idle-daily task, that is enough to fix most cases, other cases will be well handled by bug 683876.

Comment 2

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

Attachment: patch v1.0

A brief explanation of what this does:
- runs expiration at idle-daily
- renamed ACTION.IDLE to ACTION.IDLE_DIRTY to clarify when it runs
- run analyze when the number of pages increases considerably (import, sync)
- don't run analyze on clear history: even if history becomes empty, the user's browsing habits are hardly changing, so statistics are still good to keep. This covers the slow-growth case.

As it is, should cover all cases I may think of, included improving the mobile situation when Sync adds a bunch of pages.
Both additions have a test, since I would like to backport this patch to beta and aurora.

Comment 3

[Dietrich Ayala (:dietrich)]

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

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

looks good, r=me. little concerned about 100 being too small, but lets give it a try and tweak from there.

Comment 4

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

looking at my try results, looks like this may cause a Ts_shutdown regression. I have 2 hypothesis, either we wrongly run idle-daily tasks during that test, or it's just the effect of adding the initial query to count pages. I'll have to split the patch into these 2 parts and figure the culprit, then I'll have to figure out a solution.

Comment 5

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

Ok, I really don't like what I did since requires me to count pages regardless the fact I will expire or less.
So I just had this crazy idea to compare current number of pages to the value stored in sqlite_stat1, that will give me an idea whether it needs an update or not, it's much simpler and less hairy.

Comment 6

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

Attachment: patch v1.1

This is a better approach, I like it more and should solve the Ts_shutdown regression (pushing to try now to ensure that).

Comment 7

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

talos numbers from try are in the noise, should be fine.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 565676 [details] [diff] [review]
patch v1.1

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

yeah, cleaner! r=me

Comment 9

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/3176f14f0483

Comment 10

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

Comment on attachment 565676 [details] [diff] [review]
patch v1.1

And this is the last piece needed to fix the periodic hangs on stable branches (along with bug 691509 and bug 686025)
This change ensures to ANALYZE on idle-daily or on the timed expiration when the distance between the statistics and the database is over a certain threshold.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/3176f14f0483

Comment 12

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/5f1156bde9ef

Comment 13

[u279076]

Is there something QA can do to verify this fix?

Comment 14

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

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Is there something QA can do to verify this fix?

see https://bugzilla.mozilla.org/show_bug.cgi?id=686025#c119

Comment 15

[LegNeato]

This doesn't seem to just transplant to beta...can you please land it there?

Comment 16

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

yes, in my queue

Comment 17

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

https://hg.mozilla.org/releases/mozilla-beta/rev/77d2910ba514

Comment 18

[u279076]

(In reply to Marco Bonardo [:mak] from comment #14)
> (In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> > Is there something QA can do to verify this fix?
> 
> see https://bugzilla.mozilla.org/show_bug.cgi?id=686025#c119

Given that, I think this is not something QA can verify specifically.