Closed Bug 1417101 Opened 2 years ago Closed 2 years ago

Bookmark annotation observer doesn't need to fetch item info if we're not updating the modified date

Categories

(Toolkit :: Places, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: lina, Assigned: lina)

References

(Depends on 1 open bug)

Details

(Keywords: perf)

Attachments

(1 file)

https://searchfox.org/mozilla-central/rev/bab833ebeef6b2202e71f81d225b968283521fd6/toolkit/components/places/nsNavBookmarks.cpp#3324-3326,3335-3347 fetches item info synchronously, even if `aDontUpdateLastModified = true` and nothing changed. It's a small optimization, but I think we can skip fetching and notifying entirely for that case.
Component: Sync → Places
Product: Firefox → Toolkit
(In reply to Kit Cambridge (he/him) [:kitcambridge] from comment #0)
> https://searchfox.org/mozilla-central/rev/
> bab833ebeef6b2202e71f81d225b968283521fd6/toolkit/components/places/
> nsNavBookmarks.cpp#3324-3326,3335-3347 fetches item info synchronously, even
> if `aDontUpdateLastModified = true` and nothing changed. It's a small
> optimization, but I think we can skip fetching and notifying entirely for
> that case.

We looked at this before, and I think it broke some update notifications. I can't remember exactly what, but it meant the notifications of annotation changes weren't getting through. It could have been that the library window wasn't updating properly, xref:

https://bugzilla.mozilla.org/show_bug.cgi?id=1405317#c5
(In reply to Mark Banner (:standard8) from comment #2)
> We looked at this before, and I think it broke some update notifications. I
> can't remember exactly what, but it meant the notifications of annotation
> changes weren't getting through. It could have been that the library window
> wasn't updating properly, xref:
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=1405317#c5

I took a look at the logs, and the case we saw at the time it was the livemark cache service that we discussed being affected by this - however that has now been removed (bug 1405722).

We're then left with cases of our existing onItemChanged handers needing to know about the annotation change or not. At a short glance, there probably aren't, but Marco will likely know better than me.
Ah, that makes sense. Thanks for the additional context, Standard8!
There are a few things that are likely to break, nothing we couldn't change with some work, but likely we can't just stop notifying for now, and I'm not sure the patch as-is is right cause it would regress some of these.

The first thing is NodeAnnotationChanged, it's a notification handled by the UI views, currently for 2 things:
* description - would be solved by 1402890, description would pretty much become immutable at that point and maybe not even shown in the UI, unclear.
* feeduri - this is used to know when a livemark is added and appropriately mark a node as such. We currently have no other way to know that a just added node is a livemark (the new notifications could provide such a thing I suppose).

Then other consumers:
* load in sidebar anno, it is used by editBookmarkOverlay to keep the bookmark info checkbox in sync in Properties or the Library pane
* Sync is also observing these changes https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/services/sync/modules/engines/bookmarks.js#1037 and bumping the score, if I read that correctly

These notification needs are independent from aDontUpdateLastModified.

I was wondering if we could completely remove the annotations observer from the bookmarks service as well.

Avoiding the fetch is feasible, the annotations service already fetches the bookmark before setting the anno, it could fetch the missing info (lastMod, type, parentId, guid, parentGuid) and pass it out through OnItemAnnotationSet/Removed.
We could even go a bit further, add a NotifyItemChanged with a dontUpdateLastModified method to nsNavBookmarks, and directly invoke it from the annotations service (or use the existing getObservers(), not sure how nice it would be from cpp)

Then the bookmarks service could potentially stop observing annotations (win-win!).

BUT

There's the lastModified write problem.
The whole problem of "should we update lastModified when annotations change" is worth a discussion, I'm honestly not sure anymore. I'd say in general no, annotations are weak properties and don't influence the underlying object.
There are edge cases like modifying the "Open in sidebar" checkbox on a bookmark and noticing last modified doesn't change, we may not care. With description going away, add-ons not having access to old ugly annotations, and description going away, there isn't much left.
My assumption is that if we'd document in the idl that adding/removing annotations doesn't modify the underlying bookmark, we should be ok to proceed.

BUT

this change will also stop updating the syncChangeCounter field, and that may be a problem.
A first interim solution could be to make NotifyItemChanged get a syncDelta and do the update if necessary.

Thoughts are welcome.
Comment on attachment 8928201 [details]
Bug 1417101 - Remove the annotation observer from the bookmarks service.

https://reviewboard.mozilla.org/r/199416/#review204884

Per previous comment, I think this would regress some UI updating of livemarks in Places views.
Attachment #8928201 - Flags: review?(mak77)
No longer blocks: 1305563, 1412143
See Also: → 1305563, 1412143
(In reply to Marco Bonardo [::mak] from comment #5)

That all makes sense, thanks for the explanation!

> We could even go a bit further, add a NotifyItemChanged with a
> dontUpdateLastModified method to nsNavBookmarks, and directly invoke it from
> the annotations service...

This sounds like a good first step, if we want to minimize behavior changes. Looks like we already have https://searchfox.org/mozilla-central/rev/550148ab69b2879bfb82ffa698720ede1fa626f2/toolkit/components/places/nsNavBookmarks.cpp#3091 that we can use.

> The whole problem of "should we update lastModified when annotations change" is
> worth a discussion, I'm honestly not sure anymore. I'd say in general no,
> annotations are weak properties and don't influence the underlying object.

This sounds mostly reasonable. We do use the last modified time to resolve conflicts if the bookmark changes locally and remotely, but I don't think we need to worry about that here. I'd expect changing the description or "load in sidebar" checkbox is already fairly uncommon; changing another attribute, like the title or the URL, of the *same* bookmark on a *different* device even less common than that. However...

> this change will also stop updating the syncChangeCounter field, and that may
> be a problem.

This could be more of a problem if we pass `aDontUpdateLastModified` without adding or changing the bookmark in a way that already bumps its change counter. I think forwarding `aDontUpdateLastModified` and the source to `NotifyItemChanged`, and having it bump the change counter if `aDontUpdateLastModified && aSource != SOURCE_SYNC` makes sense.
Priority: -- → P2
Keywords: perf
I went with option 1 for now. Passing the `BookmarkData` up from `StartSetAnnotation` turned into a bit of a yak shave, since we also use it for page annos. Bonus: we can also fix bug 653910 while we're here.
Comment on attachment 8928201 [details]
Bug 1417101 - Remove the annotation observer from the bookmarks service.

https://reviewboard.mozilla.org/r/199416/#review206004

This is great, I'm very happy about it!

I have a few comments, but nothing blocking. Thank you a lot!

::: toolkit/components/places/nsAnnotationService.cpp:64
(Diff revision 2)
> +  changeData.isAnnotation = true;
> +  changeData.updateLastModified = !aDontUpdateLastModified;
> +  changeData.source = aSource;
> +  changeData.property = aName;
> +
> +  if (nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService()) {

not completely sure what's our coding style, but I'd prefer avoiding the if (something = other) syntax, I'd rather do it out of the if, or add another set of parenthesis to clarify it's not a typo

::: toolkit/components/places/nsAnnotationService.cpp:192
(Diff revision 2)
> +    StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration,
> -                                   nsIAnnotationService::TYPE_STRING,
> +                       nsIAnnotationService::TYPE_STRING,
> -                                   statement);
> +                       statement);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (result.isErr()) {
> +    return result;
> +  }

nit: let's just invert the condition and return result at the end.
You can remove some of the many newlines in this method and the code will end up being more compact.

::: toolkit/components/places/nsAnnotationService.cpp:421
(Diff revision 2)
> +    return result.unwrapErr();
> +  }
>  
>    NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aItemId, aName, aSource, aDontUpdateLastModified));
>  
> +  if (Maybe<BookmarkData> bookmark = result.unwrap()) {

ditto for if (a = b)

::: toolkit/components/places/nsAnnotationService.cpp:445
(Diff revision 2)
> +    StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration,
> -                                   nsIAnnotationService::TYPE_INT32,
> +                       nsIAnnotationService::TYPE_INT32,
> -                                   statement);
> +                       statement);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (result.isErr()) {
> +    return result;
> +  }

As before, I'd invert the condition and unify the return path, removing some pointless newlines.

::: toolkit/components/places/nsAnnotationService.cpp:531
(Diff revision 2)
> +    StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration,
> -                                   nsIAnnotationService::TYPE_INT64,
> +                       nsIAnnotationService::TYPE_INT64,
> -                                   statement);
> +                       statement);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (result.isErr()) {
> +    return result;
> +  }

ditto

::: toolkit/components/places/nsAnnotationService.cpp:617
(Diff revision 2)
> +    StartSetAnnotation(aURI, aItemId, aName, aFlags, aExpiration,
> -                                   nsIAnnotationService::TYPE_DOUBLE,
> +                       nsIAnnotationService::TYPE_DOUBLE,
> -                                   statement);
> +                       statement);
> -  NS_ENSURE_SUCCESS(rv, rv);
> +  if (result.isErr()) {
> +    return result;
> +  }

ditto

::: toolkit/components/places/nsAnnotationService.cpp:1535
(Diff revision 2)
> +
> +  if (Maybe<BookmarkData> bookmark = result.unwrap()) {
> +    NotifyItemChanged(bookmark.ref(), aName, aSource, false);
> +  }
>  
>    NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationRemoved(aItemId, aName, aSource));

The order of notifications look different, before we were notifying the anno change, and then the bookmark change. Is there a reason for this change?

::: toolkit/components/places/nsAnnotationService.cpp:1575
(Diff revision 2)
>  {
> +  nsresult rv = RemoveItemAnnotationsWithoutNotifying(aItemId);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationRemoved(aItemId, EmptyCString(),
> +                                                 aSource));

it's probably worth adding a note to the idl that this doesn't send itemChanged notifications, as well as annotate the other methods that will do.

::: toolkit/components/places/nsAnnotationService.cpp:1762
(Diff revision 2)
>      rv = copyStmt->Execute();
>      NS_ENSURE_SUCCESS(rv, rv);
>  
>      NOTIFY_ANNOS_OBSERVERS(OnItemAnnotationSet(aDestItemId, annoName, aSource, false));
> +
> +    if (nsNavBookmarks* bookmarks = nsNavBookmarks::GetBookmarksService()) {

ditto if (a=b)

::: toolkit/components/places/nsNavBookmarks.cpp:684
(Diff revision 2)
>    NS_ENSURE_SUCCESS(rv, rv);
>  
>    mozStorageTransaction transaction(mDB->MainConn(), false);
>  
> -  // First, if not a tag, remove item annotations.
> +  // First, if not a tag, remove item annotations. We remove without notifying
> +  // to avoid firing `onItemChanged` for an item that we're about to remove.

maybe you meant onItemAnnotationRemoved here? RemoveItemAnnotations, after these changes, doesn't notify onItemChanged already, the only difference with WithoutNotifying is onItemAnnotationRemoved.

::: toolkit/components/places/nsNavBookmarks.cpp:3093
(Diff revision 2)
>    MOZ_ASSERT(!aData.bookmark.guid.IsEmpty());
> +
> +  PRTime lastModified = aData.bookmark.lastModified;
> +  if (aData.updateLastModified) {
> +    lastModified = RoundedPRNow();
> +    Unused << NS_WARN_IF(NS_FAILED(

just use MOZ_ALWAYS_SUCCEEDS(

::: toolkit/components/places/tests/bookmarks/test_nsINavBookmarkObserver.js
(Diff revision 2)
>    let promise = Promise.all([
>      gBookmarkSkipObserver.setup([
> -      "onItemChanged", "onItemRemoved"
> +      "onItemRemoved"
>      ]),
>      gBookmarksObserver.setup([
> -      { name: "onItemChanged", // This is an unfortunate effect of bug 653910.

There are a few pointers to that bug also in test_bookmarks_notifications.js, mthat should be removed.
Attachment #8928201 - Flags: review?(mak77) → review+
Comment on attachment 8928201 [details]
Bug 1417101 - Remove the annotation observer from the bookmarks service.

https://reviewboard.mozilla.org/r/199416/#review206004

Thanks for the review!

> not completely sure what's our coding style, but I'd prefer avoiding the if (something = other) syntax, I'd rather do it out of the if, or add another set of parenthesis to clarify it's not a typo

Fixed, here and everywhere.

> The order of notifications look different, before we were notifying the anno change, and then the bookmark change. Is there a reason for this change?

Copy-pasta fail. Fixed to notify bookmarks after annos.

> maybe you meant onItemAnnotationRemoved here? RemoveItemAnnotations, after these changes, doesn't notify onItemChanged already, the only difference with WithoutNotifying is onItemAnnotationRemoved.

I meant `onItemAnnotationRemoved`, yes. Thanks!
Duplicate of this bug: 653910
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e0eab399db59
Remove the annotation observer from the bookmarks service. r=mak
Backout by nerli@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ab69dc1d4af3
Backed out changeset e0eab399db59 for Static errors in toolkit/components/places/target r=backout on a CLOSED TREE
Depends on: 1418587
Comment on attachment 8928201 [details]
Bug 1417101 - Remove the annotation observer from the bookmarks service.

Sounds like bug 1418587 is going to be very tricky, so I changed the patch to use pointers instead. Mak, please take another look at your convenience.
Attachment #8928201 - Flags: review+ → review?(mak77)
Attachment #8928201 - Flags: review?(mak77)
Comment on attachment 8928201 [details]
Bug 1417101 - Remove the annotation observer from the bookmarks service.

https://reviewboard.mozilla.org/r/199416/#review210640

LGTM, thanks
Attachment #8928201 - Flags: review?(mak77) → review+
Pushed by kcambridge@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dd84a4ad910e
Remove the annotation observer from the bookmarks service. r=mak
https://hg.mozilla.org/mozilla-central/rev/dd84a4ad910e
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.