Closed Bug 482346 Opened 13 years ago Closed 13 years ago

mDBFindURIBookmarks can't use indices to sort

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: perf, Whiteboard: [TSnappiness])

Attachments

(2 files, 4 obsolete files)

with bug 481261 this query will warn due to the fact it cannot use an index to sort.

current query is

214       "SELECT b.id "
215       "FROM moz_bookmarks b "
216       "JOIN ( "
217         "SELECT id FROM moz_places_temp "
218         "WHERE url = ?1 "
219         "UNION ALL "
220         "SELECT id FROM moz_places "
221         "WHERE url = ?1 "
222         "AND +id NOT IN (SELECT id FROM moz_places_temp) "
223       ") AS h ON b.fk = h.id "
224       "WHERE b.type = ?2 "
225       "ORDER BY MAX(IFNULL(b.lastModified, 0), b.dateAdded) DESC, b.id DESC"),

if we suppose that lastModified > dateAdded and dateAdded can't be null we could do

SELECT b.id
FROM moz_bookmarks b
WHERE b.type = 1 AND b.fk = (
    SELECT id FROM moz_places_temp
    WHERE url = "chrome://mcd/content/s3.xul"
    UNION
    SELECT id FROM moz_places
    WHERE url = "chrome://mcd/content/s3.xul"
    LIMIT 1
)
ORDER BY IFNULL(b.lastModified, b.dateAdded) DESC, b.id DESC

this has a better explain, but won't stop warn, since we can't have an index on the first order by clause
Attached patch patch v1.0Splinter Review
and eventually, that's it
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #366425 - Flags: review?(sdwilsh)
Mardak suggests setting lastModified to dateAdded on items' creation.
Attachment #366425 - Attachment is patch: true
Attachment #366425 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #2)
> Mardak suggests setting lastModified to dateAdded on items' creation.
That seems sensible to me.  Does doing this get rid of the warning?  If so, we should totally do this!
Comment on attachment 366425 [details] [diff] [review]
patch v1.0

canceling request until comment 3 is addressed
Attachment #366425 - Flags: review?(sdwilsh)
that will require a schema update to fix all empty dates, we wanted already to do this in the past, i can't recall why we didn't atm.
disabled the warning for this query for now:
http://hg.mozilla.org/mozilla-central/rev/8f59f15f01a0
i just notice actually most bookmarks have lastModified filled, because after adding the bookmark we set description, that is an annotation, and that causes lastModified to be set.
Attached patch wip (obsolete) — Splinter Review
1. set lastModified = dateAdded
2. update treeView to avoid showing lastModified if it's the same as dateAdded
3. try to use mDBInsertBookmark globally, to use prepared statement
4. try to set lastModified immediately, without having to run 2 queries.

needs cleanup and a small test.
I think i'm not going to apply any "trick" to hide lastModified if the item has not been modified.
This was a design choice, but has not real benefits imo. Hiding could be done at 2 levels:
- at a query level with an IFNULL we could return 0 if the 2 dates are equal. This would add complication to a major number of queries, is worth doing it?
- at a result level, we could set an empty value on nodes, but this would be out of sync with real value
- at a view level, but then when sorting we would use the node's value, so sorting results would appear "strange"

I think there's no point in hiding lastModified because:
- thinking to lastModified use cases, it's quite clear user expects to see last changes he did to bookmarks, and adding a bookmark is a change.
- when we add a bookmark for a page we usually set also description taking it from the page if it's available, this means the main creation point for bookmarks will already set lastModified just some ms after dateAdded.

Thoughts?
Notice on Windows for example you see both dateAdded and lastModified, where lastMod is set to dateAdded if no further changes have been made.
Attached patch patch v2.0 (obsolete) — Splinter Review
asking code review to Shawn. btw i'll ask an additional review to Dietrich for the approach change (having lastModified always filled)
Attachment #383818 - Attachment is obsolete: true
Attachment #383941 - Flags: review?(sdwilsh)
Attachment #383941 - Flags: review?(dietrich)
Comment on attachment 383941 [details] [diff] [review]
patch v2.0

>+++ b/toolkit/components/places/public/nsINavBookmarksService.idl
>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.  To know if an item has been really modified is possible
>+   *        to compare these 2 attributes.
two things:
1) I'm not really sure what the second sentence is trying to say.
2) spell out numbers <= 10 please

>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>   // mDBSetItemDateAdded
>+  // Notice we want to set dateAdded and lastModified to the same value, we do
>+  // this for performance reasons since will allow to use an index.
>+  // Moreover having (lastModified < dateAdded) would not make sense.
>   rv = mDBConn->CreateStatement(NS_LITERAL_CSTRING(
>-      "UPDATE moz_bookmarks SET dateAdded = ?1 WHERE id = ?2"),
>+      "UPDATE moz_bookmarks SET dateAdded = ?1, lastModified = ?1 "
>+      "WHERE id = ?2"),
This comment is also not clear

>+  PRTime dateAdded = PR_Now();
>   {
>     mozStorageStatementScoper scope(mDBInsertBookmark);
>-    rv = mDBInsertBookmark->BindInt64Parameter(0, childID);
>+    rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_Id);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt32Parameter(1, TYPE_BOOKMARK);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_PlaceId, childID);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt64Parameter(2, aFolder);
>+    rv = mDBInsertBookmark->BindInt32Parameter(kInsertBookmarkIndex_Type, TYPE_BOOKMARK);
>     NS_ENSURE_SUCCESS(rv, rv);
>-    rv = mDBInsertBookmark->BindInt32Parameter(3, index);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_Parent, aFolder);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>+    rv = mDBInsertBookmark->BindInt32Parameter(kInsertBookmarkIndex_Position, index);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    // Support NULL titles.
>     if (aTitle.IsVoid())
>-      rv = mDBInsertBookmark->BindNullParameter(4);
>+      rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_Title);
>     else
>-      rv = mDBInsertBookmark->BindUTF8StringParameter(4, aTitle);
>+      rv = mDBInsertBookmark->BindUTF8StringParameter(kInsertBookmarkIndex_Title, aTitle);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>-    rv = mDBInsertBookmark->BindInt64Parameter(5, PR_Now());
>+    rv = mDBInsertBookmark->BindNullParameter(kInsertBookmarkIndex_ServiceContractId);
>     NS_ENSURE_SUCCESS(rv, rv);
>-  
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_DateAdded, dateAdded);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+    rv = mDBInsertBookmark->BindInt64Parameter(kInsertBookmarkIndex_LastModified, dateAdded);
>+    NS_ENSURE_SUCCESS(rv, rv);
>+
>     rv = mDBInsertBookmark->Execute();
>     NS_ENSURE_SUCCESS(rv, rv);
Can we pull all this out into a helper method that takes all the arguments we'll bind?  We use this logic in at least two places :/

>+  // XXX: do we really need to notify lastModified has changed? since the item
>+  // has changed, it's quite clear that lastModified has changed.
I feel like we shouldn't notify here

>+  // XXX: do we really need to notify lastModified has changed? since the item
>+  // has changed, it's quite clear that lastModified has changed.
>+  NOTIFY_DATE_CHANGED(aBookmarkId, "lastModified", lastModified);
likewise here

>+  // XXX: Do we really need to notify that lastModified has changed? since the
>+  // item has changed, it's quite clear that lastModified has changed.
>+  NOTIFY_DATE_CHANGED(aBookmarkId, "lastModified", lastModified);
ditto
Attachment #383941 - Flags: review?(sdwilsh) → review-
Attached patch patch v2.1 (obsolete) — Splinter Review
Attachment #383941 - Attachment is obsolete: true
Attachment #384034 - Flags: review?(sdwilsh)
Attachment #383941 - Flags: review?(dietrich)
Comment on attachment 384034 [details] [diff] [review]
patch v2.1

>+++ b/toolkit/components/places/public/nsINavBookmarksService.idl
>+   * @note: This is the only method that will send an itemChanged notification
nit: no : after note.

>+   *        for property "lastModified".  lastModified will still be updated in
nit: "for the property".

>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.
ditto, and capitalize the 'w' please.  also, comma after "is added"

>+++ b/toolkit/components/places/public/nsINavHistoryService.idl
>+   * @note: when an item is added lastModified is set to the same value as
>+   *        dateAdded.
ditto here

>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>   // mDBSetItemDateAdded
>+  // lastModified is set to the same value as dateAdded, we do this for
I think we want a period, and not a comma after dataAdded.

>+  // performance reasons, since will allow to use an index to sort items by
nit: "since it will allow us to use"


>+nsresult
>+nsNavBookmarks::InsertBookmarkInDB(PRInt64 aItemId,
>+                                   PRInt64 aPlaceId,
>+                                   PRUint16 aItemType,
>+                                   PRInt64 aParentId,
>+                                   PRInt32 aIndex,
>+                                   const nsACString &aTitle,
>+                                   const nsAString &aServiceContractId,
>+                                   PRTime aDateAdded,
>+                                   PRTime aLastModified,
>+                                   PRInt64 *_retval)
_retval should be something more descriptive like _itemId

>+{
>+  NS_ENSURE_ARG_POINTER(_retval);
This isn't a public API, so it should be an assertion (we don't need to check it)

>+    NS_ASSERTION(hasResult, "hasResult is false but the call succeeded?");
We actually want to return an error code here.  I know I've argued the opposite in the past, and something would be really wrong if that happened.

>-  return CreateContainerWithID(-1, aParent, aName, EmptyString(), PR_TRUE,
>+  nsString voidString;
>+	voidString.SetIsVoid(PR_TRUE);
nit: tab

>+++ b/toolkit/components/places/src/nsNavBookmarks.h
>+   * @param aItemId
>+   *        itemId to insert, pass -1 or nsnull to generate a new one.
nit: "The itemId..."
I feel like we should only allow -1 here actually.

>+   * @param aPlaceId
>+   *        placeId to which this bookmark refers to, pass nsnull for
>+   *        items that don't refer to an URI (eg. folders, separators, ...).
nit: "The palceId..."
again  -1

>+   * @param aItemType
>+   *        type of the new bookmark, see TYPE_* constants.
nit: "The type..." I'm going to stop saying this one, but it applies for this whole method.
I kind want an enum here just for extra type checking and making sure we handle possible new ones
>+   * @param aTitle
>+   *        title for the new bookmark, pass a void string to set a NULL title.
nit: Use a new sentance for the second part.

>+   * @param aServiceContractId
>+   *        the contract id for a dynamic container, pass a void string for
>+   *        other types of items.
>+   * @param aDateAdded
>+   *        date for the insertion.
>+   * @param aLastModified
>+   *        last modified date for the insertion, usually equal to aDateAdded.
I think we should re-order these last three, and make aLastModified and aServiceContractId optional.  The order should then be aDataAdded, aLastModified, and then aServiceContractId.  aServiceContractId would just check for IsEmpty instead of IsVoid, and the default value can be EmptyCString();

>+   * @note: This will also update last modified date of the parent folder.
nit: no colon

r=sdwilsh with these changes
Attachment #384034 - Flags: review?(sdwilsh) → review+
Attached patch patch v2.2 (obsolete) — Splinter Review
asking for a quick code check, and approval for the behavior change (always show last modified)
Attachment #384034 - Attachment is obsolete: true
Attachment #384405 - Flags: review?(dietrich)
Attached patch patch v2.3Splinter Review
Attachment #384405 - Attachment is obsolete: true
Attachment #384462 - Flags: review?(dietrich)
Attachment #384405 - Flags: review?(dietrich)
Keywords: perf
Comment on attachment 384462 [details] [diff] [review]
patch v2.3

r=me
Attachment #384462 - Flags: review?(dietrich) → review+
Whiteboard: [tsnap]
http://hg.mozilla.org/mozilla-central/rev/efbde7c87cdf
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [tsnap] → [TSnappiness]
Target Milestone: --- → mozilla1.9.2a1
Flags: in-testsuite+
Depends on: 507058
You need to log in before you can comment on or make changes to this bug.