Last Comment Bug 690354 - Idle expiration never runs for clean databases
: Idle expiration never runs for clean databases
Product: Toolkit
Classification: Components
Component: Places (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: mozilla10
Assigned To: Marco Bonardo [::mak]
: Marco Bonardo [::mak]
Depends on:
Blocks: 683876 PlacesJank
  Show dependency treegraph
Reported: 2011-09-29 07:53 PDT by Marco Bonardo [::mak]
Modified: 2013-12-27 14:21 PST (History)
9 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1.0 (25.63 KB, patch)
2011-09-30 06:07 PDT, Marco Bonardo [::mak]
dietrich: review+
Details | Diff | Splinter Review
patch v1.1 (26.92 KB, patch)
2011-10-07 15:37 PDT, Marco Bonardo [::mak]
dietrich: review+
asa: approval‑mozilla‑aurora+
asa: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2011-09-29 07:53:08 PDT
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] 2011-09-29 17:34:42 PDT
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] 2011-09-30 06:07:47 PDT
Created attachment 563719 [details] [diff] [review]
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) 2011-10-06 17:08:57 PDT
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] 2011-10-07 02:05:51 PDT
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] 2011-10-07 12:37:21 PDT
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] 2011-10-07 15:37:53 PDT
Created attachment 565676 [details] [diff] [review]
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] 2011-10-08 00:47:01 PDT
talos numbers from try are in the noise, should be fine.
Comment 8 Dietrich Ayala (:dietrich) 2011-10-08 10:26:54 PDT
Comment on attachment 565676 [details] [diff] [review]
patch v1.1

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

yeah, cleaner! r=me
Comment 10 Marco Bonardo [::mak] 2011-10-10 06:57:12 PDT
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) 2011-10-10 11:13:05 PDT
Comment 13 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-10-13 11:22:26 PDT
Is there something QA can do to verify this fix?
Comment 14 Marco Bonardo [::mak] 2011-10-13 15:48:06 PDT
(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #13)
> Is there something QA can do to verify this fix?

Comment 15 christian 2011-10-18 13:15:10 PDT
This doesn't seem to just transplant to beta...can you please land it there?
Comment 16 Marco Bonardo [::mak] 2011-10-18 15:12:09 PDT
yes, in my queue
Comment 17 Marco Bonardo [::mak] 2011-10-18 17:11:15 PDT
Comment 18 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2011-12-08 11:29:23 PST
(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

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

Note You need to log in before you can comment on or make changes to this bug.