453529 - Retain embed visits and places into the temp table in memory

Status: RESOLVED FIXED

(Toolkit :: Places, defect, P2)

Description

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

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.

Comment 1

[Shawn Wilsher :sdwilsh]

Does that cause problems for things such as from_visit if entries go missing?

Comment 2

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

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.

Comment 3

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

Attachment: wip

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.

Comment 4

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

Attachment: patch v1

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.

Comment 5

[Shawn Wilsher :sdwilsh]

Comment on attachment 352162 [details] [diff] [review]
patch v1

r=sdwilsh

Can you file a bug about caching the result of _getSyncTableStatement please?

Comment 6

[Shawn Wilsher :sdwilsh]

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.

Comment 7

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

filed bug 468705 for caching statements

Comment 8

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

we will investigate holding places in memory too, so this will not land immediately unless we see if that is feasible and fast

Comment 9

[Shawn Wilsher :sdwilsh]

re-requested blocking.  See comment 6 for reasoning.

Comment 10

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

Attachment: patch v2

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?

Comment 11

[Shawn Wilsher :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

Comment 12

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

Attachment: patch v2.1

comments addressed, i retained my original query, but tomorrow i'll do some perf test on it

Comment 13

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

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.

Comment 14

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

http://hg.mozilla.org/mozilla-central/rev/57469d8d6668

Comment 15

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

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/174f769a8f48