456029 - optimize the temp tables triggers

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

Attachment: patch v1

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.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.2

comments on why we use OR IGNORE conflict

Comment 2

[Shawn Wilsher :sdwilsh]

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

r=sdwilsh

Comment 3

[Marco Bonardo [:mak]]

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

Comment 4

[Shawn Wilsher :sdwilsh]

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)

Comment 5

[Marco Bonardo [:mak]]

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.

Comment 6

[Marco Bonardo [:mak]]

Attachment: patch v1.3

updated with optimization from comment 5

Comment 7

[Shawn Wilsher :sdwilsh]

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.

Comment 8

[Marco Bonardo [:mak]]

(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

Comment 9

[Shawn Wilsher :sdwilsh]

(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).

Comment 10

[Marco Bonardo [:mak]]

(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.

Comment 11

[Shawn Wilsher :sdwilsh]

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

Comment 12

[Marco Bonardo [:mak]]

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 13

[Shawn Wilsher :sdwilsh]

Comment on attachment 340665 [details] [diff] [review]
patch v1.3

Oh...right.  That makes complete sense.  r=sdwilsh

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Marco Bonardo [:mak]]

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.

Comment 16

[Marco Bonardo [:mak]]

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.

Comment 17

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/684c39876ec5

Comment 18

[Tracy Walker [:tracy]]

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

Comment 19

[Gervase Markham [:gerv]]

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