Actual triggers are really slow, i saw clearing my history moving from a tenth of seconds to more than 5 minutes. The main reason is the use of the update trigger inside the insert/delete trigger. Actually a first step is changing triggers to use OR IGNORE conflict when inserting into temp table, also move place to the temp table before updating it. an easy test to see timings is adding a bunch of visits (measuring time elapsed to do it) then calling removeAllPages() (again measuring timing). With 1000 visits i got 8s for inserts and 11s for deletes with current triggers. With this patch i got 4s for inserts and 250ms for deletes, so it's looking better.

comments on why we use OR IGNORE conflict

Comment on attachment 339441 [details] [diff] [review] patch v1.2 r=sdwilsh

some timing: add 1000 visits and then removeAllPages() | without partitioning | new triggers | old triggers ======================================================================== insert | 40s | 4s | 8s insert batched | 750ms | 4s | 8s delete | 180ms | 250ms | 11s

for the record, mconnor said on irc that I was suitable to review this patch (he also said he posted in the bug, but I don't know why it's not here)

an additional optimization would be to not copy places to temp table if the visit_type is 0,4,7 that moved inserting 1000 embed visits from 4s to 500ms. this means doing "INSERT OR IGNORE INTO moz_places_temp " \ "SELECT * " \ "FROM moz_places " \ "WHERE id = NEW.place_id " \ "AND NEW.visit_type NOT IN (0, 4, 7); " /* invalid, EMBED, DOWNLOAD */ \ good for a follow-up, if the fsync stuff checkin goes well we will take this optimization later.

updated with optimization from comment 5

Comment on attachment 340665 [details] [diff] [review] patch v1.3 Let's do this in a follow-up. I'm still concerned about the implications this may have.

(In reply to comment #7) > (From update of attachment 340665 [details] [diff] [review]) > Let's do this in a follow-up. I'm still concerned about the implications this > may have. which implications? this is simply faster

(In reply to comment #8) > which implications? this is simply faster We end up changing how that data is used though. That has the impliction (I think at least) that visit counts would not be updated for those data points anymore. I really think we should do it in a follow-up (and I'd like to hear from dietrich on it too).

(In reply to comment #9) > We end up changing how that data is used though. That has the impliction (I > think at least) that visit counts would not be updated for those data points > anymore. I really think we should do it in a follow-up (and I'd like to hear > from dietrich on it too). mh no, notice that when updating visit count we do "AND NEW.visit_type NOT IN (0, 4, 7);" so the change is simply avoiding copy a data that later will NOT be used anyway.

We don't use it? Ok, so dietrich needs to look at it then since I didn't even know that was the case :/

maybe you msunderstood me, + "INSERT OR IGNORE INTO moz_places_temp " \ + "SELECT * " \ + "FROM moz_places " \ + "WHERE id = NEW.place_id " \ + "AND NEW.visit_type NOT IN (0, 4, 7); " \ this does not copy disk place to memory place if new visit is 0,4,7... + "UPDATE moz_places_temp " \ "SET visit_count = visit_count + 1 " \ "WHERE id = NEW.place_id " \ "AND NEW.visit_type NOT IN (0, 4, 7); " ...because here we update the place only if new visit is not 0,4,7 so without the change we copy the place from disk to memory if visit is 0,4,7, but we will not update visit_count for that place because visit is 0,4,7. that's why i say that the optimization avoid to copy a useless data.

Comment on attachment 340665 [details] [diff] [review] patch v1.3 Oh...right. That makes complete sense. r=sdwilsh

Comment on attachment 339441 [details] [diff] [review] patch v1.2 obsoleting 1.2, the functionality in 1.3 is exactly the same, producing same results, only faster for 0,4,7 visits. Thanks

an additional optimization that would take away 1 query #define CREATE_HISTORYVISITS_VIEW_INSERT_TRIGGER NS_LITERAL_CSTRING( \ "CREATE TEMPORARY TRIGGER moz_historyvisits_view_insert_trigger " \ "INSTEAD OF INSERT " \ "ON moz_historyvisits_view " \ "BEGIN " \ "INSERT INTO moz_historyvisits_temp ( " \ "id, from_visit, place_id, visit_date, visit_type, session " \ ") " \ "VALUES (MAX((SELECT IFNULL(MAX(id), 0) FROM moz_historyvisits_temp), " \ "(SELECT IFNULL(MAX(id), 0) FROM moz_historyvisits)) + 1, " \ "NEW.from_visit, NEW.place_id, NEW.visit_date, NEW.visit_type, " \ "NEW.session); " \ "INSERT OR REPLACE INTO moz_places_temp " \ "SELECT id, url, title, rev_host, visit_count + 1, hidden, typed, favicon_id, frecency " \ "FROM moz_places " \ "WHERE id = NEW.place_id " \ "AND NEW.visit_type NOT IN (0, 4, 7); " \ "END" \ ) and the same for DELETE trigger delete timings with this are aligned to the pre-fsync (180ms), time for embed inserts is about the same, for visit_count inserts i see a little gain regard the previous triggers.

mh, nevermind comment 15, that does not take in count that we could have updated the temp table, so importing always values from the disk table is a bad idea.

verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv