Closed Bug 453529 Opened 12 years ago Closed 12 years ago

Retain embed visits and places into the temp table in memory

Categories

(Toolkit :: Places, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 3 obsolete files)

we are now saving embed visits into the disk visits table and deleting them after 24 horus, mainly for link coloring, if we partition visits into memory we could then hold embed visits in memory, and they would become session persistent.

less disk writes, less table hog.
Does that cause problems for things such as from_visit if entries go missing?
i doubt, we use from_visit for bookmarks and favicons (mainly favicons), embed visits should not be linked there and since we are joining we will only end up with a less redirect without corruption. Probably we could not save from_visit at all if a visit is embed for db consistency.
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 3.1
Attached patch wip (obsolete) — Splinter Review
something like this, notice we don't need to expire embed visits anymore but old ones could be still there.
i would cleanup them with an additional statement in preventive maintenance, hwv those could also expire naturally as other visits do.
needs tests, for sure.
Blocks: 442967
Blocks: 223476
Attached patch patch v1 (obsolete) — Splinter Review
This is slightly different from original patch, the unit test has shocked me,  indeed i can't directly change the trigger because being it a before delete, as soon as it gets executed embed visits would be lost. Instead we should filter out at DBFlush level, so we simply don't delete(sync) those at all.

For expiring old embed visits i suggest to let them expire naturally, or file a followup so that as soon as we have preventive maintenance patch in, we can add a new task to get rid of them.
Attachment #349870 - Attachment is obsolete: true
Attachment #352162 - Flags: review?(sdwilsh)
Attachment #352162 - Flags: review?(sdwilsh) → review+
Comment on attachment 352162 [details] [diff] [review]
patch v1

r=sdwilsh

Can you file a bug about caching the result of _getSyncTableStatement please?
Requesting blocking since this helps reduce the number of writes we do (both in storing these visits, and in expiring them).

We may also want to file a bug about expiring this stuff in case the browse has been open for a long time.
Flags: blocking-firefox3.1?
Whiteboard: [has patch][has review][can land]
filed bug 468705 for caching statements
we will investigate holding places in memory too, so this will not land immediately unless we see if that is feasible and fast
Whiteboard: [has patch][has review][can land] → [investigate holding places in memory too]
Blocks: 468724
Blocks: 453178
Flags: blocking-firefox3.1?
Product: Firefox → Toolkit
QA Contact: places → places
Target Milestone: Firefox 3.1 → mozilla1.9.1
re-requested blocking.  See comment 6 for reasoning.
Flags: blocking1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Attached patch patch v2 (obsolete) — Splinter Review
this adds support fomr moz_places embeds in temp table.

we could also actually do "DELETE ... WHERE hidden <> 1 OR SUBSTR(url,0,6) = 'place:'" without looking at history table, but i'm worried by the fact in future we could use hidden for other cases different than embeds and place: uris. What do you think of that?
Attachment #352162 - Attachment is obsolete: true
Attachment #352422 - Flags: review?(sdwilsh)
Comment on attachment 352422 [details] [diff] [review]
patch v2

(In reply to comment #10)
> we could also actually do "DELETE ... WHERE hidden <> 1 OR SUBSTR(url,0,6) =
> 'place:'" without looking at history table, but i'm worried by the fact in
> future we could use hidden for other cases different than embeds and place:
> uris. What do you think of that?
I think that if we change what hidden means down the line, we have to change other code anyway, so we shoudln't worry about that here.

>+++ b/toolkit/components/places/src/nsPlacesDBFlush.js
>+    var condition = "";
Use let like the rest of the file please.

>+    if (aTableName == "historyvisits") {
>+      // For history table we want to leave embed visits in memory, since those
>+      // are expired with current session, so we are filtering them out.
>+      condition = "WHERE visit_type <> " + Ci.nsINavHistoryService.TRANSITION_EMBED;
>+    }
>+    else if (aTableName == "places") {
>+      // For places table we want to leave places associated with embed visits
>+      // in memory, they usually have hidden = 1 and at least an embed visit in
>+      // historyvisits_temp table.
>+      condition = "WHERE id IN (SELECT id FROM moz_places_temp h " +
>+                                "WHERE h.hidden <> 1 OR NOT EXISTS ( " +
>+                                  "SELECT id FROM moz_historyvisits_temp " +
>+                                  "WHERE place_id = h.id AND visit_type = " +
>+                                  Ci.nsINavHistoryService.TRANSITION_EMBED +
>+                                  " LIMIT 1) " +
>+                                ")";
>+    }
Can we turn this into a switch statement please?  I think it'll be easier to read.

Do we have any data on how much longer these queries take to execute now?  Assuming it's not bad, r=sdwilsh
Attachment #352422 - Flags: review?(sdwilsh) → review+
Attached patch patch v2.1Splinter Review
comments addressed, i retained my original query, but tomorrow i'll do some perf test on it
Attachment #352422 - Attachment is obsolete: true
Priority: -- → P2
ok i did a perf test syncing about 20000 places entries from memory to disk with old query and new query. results are quite similar, with the new trigger they are fluctuating a bit more, but the difference with these big numbers (notice we will most likely never reach a similar value) goes from 30ms better to 70ms worse. Being this executed async should not affect us in a perceivable way.
Whiteboard: [investigate holding places in memory too] → [has review][can land]
Flags: in-testsuite?
Summary: Retain embed visits into the temp table in memory → Retain embed visits and places into the temp table in memory
Whiteboard: [has review][can land] → [has review]
http://hg.mozilla.org/mozilla-central/rev/57469d8d6668
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Whiteboard: [has review]
Target Milestone: mozilla1.9.1 → mozilla1.9.2a1
Depends on: 470455
Flags: in-testsuite? → in-testsuite+
You need to log in before you can comment on or make changes to this bug.