386711 - Use single queries in nsNavHistoryExpire methods

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Marco Bonardo [:mak]]

Actually the following functions use a for cycle, and make a select and a delete on each iteration (2 sql queries per iteration).

That could be avoided creating a string with the list of IDs to delete and using a subquery. So instead of a for cycle made of queries we end up with a for cycle appending to a string and a single final query.

functions are:
nsNavHistoryExpire::EraseVisits
nsNavHistoryExpire::EraseHistory
nsNavHistoryExpire::EraseFavicons
Also EraseAnnotations (not yet implemented) could use the same method

I attach a patch to show the alternative method purposed, it should have less sqlite overhead cause it makes a single call insted of a call in every iteration.

These are my first patches and i'm learning, so please doublecheck.
I also need someone to checkin (if approved).

Comment 1

[Marco Bonardo [:mak]]

Attachment: Alternative method to do expire queries

Comment 2

[(not reading, please use [e-mail] instead)]

Sorry for the delay here, Marco.

Any reason why you re-assigned to nobody?

Comment 3

[Marco Bonardo [:mak]]

the re-assign has been made from Gavin sharp, i think he has his reasons (but don't know them)

Comment 4

[:Gavin Sharp [email: [e-mail]]]

Sorry, "re-assign to defaults" is selected automatically when you change components, and I forgot to unselect it.

Comment 5

[Marco Bonardo [:mak]]

Comment on attachment 270712 [details] [diff] [review]
Alternative method to do expire queries

there is possible problem here.
doing:
IN (1,2,3)
is different from doing:
IN ("1,2,3")

need to check how mozStorage handle deleteStatement->BindUTF8StringParameter
the string should be simply added to the string and not parsed anyway

Comment 6

[Marco Bonardo [:mak]]

Attachment: Use ExecuteSimpleSQL in nsNavHistoryExpire methods

i've rewritten the patch following naming and formatting conventions from the Dietrich's Annotation Expiry patch of Bug 319455.
it is now using ExecuteSimpleSQL instead of a useless statement, because it doesn't need anymore bindings.

This does not compile against current Trunk because it uses the nsNavHistoryRecord pageID -> placeID fix of Bug 319455, so this should land immediately after, or i can create a patch with pageID but then Dietrich should correct functions to use placeID when he lands his patch. 

Or maybe the best could be merge this patch into his work.

Comment 7

[Dietrich Ayala (:dietrich)]

thanks marco! let's keep this patch separate just to keep the patch in 319455 to a manageable size.

Comment 8

[Marco Bonardo [:mak]]

Attachment: updated patch

patch updated after landing of Dietrich's annotations patch

i'd like if Dietrich could check this before asking review to Seth

Comment 9

[Dietrich Ayala (:dietrich)]

Comment on attachment 274460 [details] [diff] [review]
updated patch

this looks good, thanks.

Comment 10

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.12; previous revision: 1.11
done

Comment 11

[John P Baker]

It is hard to check without more thorough investigation but, if EraseHistory or EraseFavicons can ever end up with no deletions to check, this could add a query execution compared with the previous code - ... WHERE id IN () ... - in which case

if (deletedPlaceIds.IsEmpty())
    return NS_OK;

may be a sensible early return.

Comment 12

[Marco Bonardo [:mak]]

Attachment: followup, early returns

followup patch, early returns as per previous comment (good catch)

Comment 13

[Marco Bonardo [:mak]]

Attachment: followup, early returns, fixes some space

fixes some space too

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 277697 [details] [diff] [review]
followup, early returns, fixes some space



>     // Do not add comma separator for the first entry
>-    if(! deletedFaviconIds.IsEmpty() )
>+    if (! deletedFaviconIds.IsEmpty() )
>       deletedFaviconIds.AppendLiteral(", ");
>     deletedFaviconIds.AppendInt(aRecords[i].faviconID);
>   }
> 

no need for the extra space at the close parens either actually, i'll remove them before checkin. r=me otherwise.

Comment 15

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistoryExpire.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistoryExpire.cpp,v  <--  nsNavHistoryExpire.cpp
new revision: 1.13; previous revision: 1.12
done

Comment 16

[John P Baker]

(In reply to comment #14)
> (From update of attachment 277697 [details] [diff] [review])
> >     // Do not add comma separator for the first entry
> >-    if(! deletedFaviconIds.IsEmpty() )
> >+    if (! deletedFaviconIds.IsEmpty() )
> >       deletedFaviconIds.AppendLiteral(", ");
> >     deletedFaviconIds.AppendInt(aRecords[i].faviconID);
> >   }
> 
> no need for the extra space at the close parens either actually, i'll remove
> them before checkin. r=me otherwise.

[Sorry I'm nit-picking after check-in]
Presumably the space in ", " is just cosmetic as well.
Probably not worth writing home about unless the lists can run to thousands of ids...

Comment 17

[Dietrich Ayala (:dietrich)]

Verified, there are no more query-in-loop patterns in these methods.

Comment 18

[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