Idle expiration never runs for clean databases

RESOLVED FIXED in Firefox 8

Status

()

Toolkit
Places
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 1 bug)

Trunk
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox8+ fixed, firefox9+ fixed, firefox10 fixed)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 683876
(Assignee)

Comment 1

6 years ago
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.
(Assignee)

Comment 2

6 years ago
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.
Attachment #563719 - Flags: review?(dietrich)
(Assignee)

Updated

6 years ago
Blocks: 691507
tracking-firefox8: --- → ?
tracking-firefox9: --- → ?
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+
(Assignee)

Comment 4

6 years ago
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.
(Assignee)

Comment 5

6 years ago
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.
(Assignee)

Comment 6

6 years ago
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).
Attachment #563719 - Attachment is obsolete: true
Attachment #565676 - Flags: review?(dietrich)
(Assignee)

Comment 7

6 years ago
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+
(Assignee)

Comment 9

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/3176f14f0483
Flags: in-testsuite+
(Assignee)

Comment 10

6 years ago
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
Last Resolved: 6 years ago
status-firefox10: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla10

Updated

6 years ago
Attachment #565676 - Flags: approval-mozilla-beta?
Attachment #565676 - Flags: approval-mozilla-beta+
Attachment #565676 - Flags: approval-mozilla-aurora?
Attachment #565676 - Flags: approval-mozilla-aurora+

Updated

6 years ago
tracking-firefox8: ? → +
tracking-firefox9: ? → +
(Assignee)

Comment 12

6 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/5f1156bde9ef
status-firefox9: --- → fixed
Is there something QA can do to verify this fix?
Whiteboard: [qa?]
(Assignee)

Comment 14

6 years ago
(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

6 years ago
This doesn't seem to just transplant to beta...can you please land it there?
(Assignee)

Comment 16

6 years ago
yes, in my queue
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/releases/mozilla-beta/rev/77d2910ba514
status-firefox8: --- → fixed
(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.