Closed Bug 690354 Opened 8 years ago Closed 8 years ago

Idle expiration never runs for clean databases

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10
Tracking Status
firefox8 + fixed
firefox9 + fixed
firefox10 --- fixed

People

(Reporter: mak, Assigned: mak)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 1 obsolete file)

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.
Blocks: 683876
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.
Attached patch patch v1.0 (obsolete) — Splinter Review
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.
Attachment #563719 - Flags: review?(dietrich)
Blocks: PlacesJank
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.
Attachment #563719 - Flags: review?(dietrich) → review+
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.
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.
Attached patch patch v1.1Splinter Review
This is a better approach, I like it more and should solve the Ts_shutdown regression (pushing to try now to ensure that).
Attachment #563719 - Attachment is obsolete: true
Attachment #565676 - Flags: review?(dietrich)
talos numbers from try are in the noise, should be fine.
Comment on attachment 565676 [details] [diff] [review]
patch v1.1

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

yeah, cleaner! r=me
Attachment #565676 - Flags: review?(dietrich) → review+
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.
Attachment #565676 - Flags: approval-mozilla-beta?
Attachment #565676 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/3176f14f0483
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Attachment #565676 - Flags: approval-mozilla-beta?
Attachment #565676 - Flags: approval-mozilla-beta+
Attachment #565676 - Flags: approval-mozilla-aurora?
Attachment #565676 - Flags: approval-mozilla-aurora+
Is there something QA can do to verify this fix?
Whiteboard: [qa?]
(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
This doesn't seem to just transplant to beta...can you please land it there?
yes, in my queue
(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.
Whiteboard: [qa?] → [qa-]
You need to log in before you can comment on or make changes to this bug.