1297945 - PlacesSyncUtils could be made more efficient when checking if a GUID exists.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

There are a couple of places where PlacesSyncUtils reads an entire bookmark object when it really only needs to know if the GUID exists and not any specific properties of it. We should just check the GUID is valid via promiseItemId() and in many cases this will require zero IO to return a result.

I'm marking this bug as depending on bug 1297941 - even though PlacesSyncUtils is already affected by that bug, this makes the situation worse (as it makes the module rely more heavily on the GuidHelpers cache.)

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1297945 - have PlacesSyncUtils use a more efficient technique for checking whether a GUID exists.

Review commit: https://reviewboard.mozilla.org/r/74022/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/74022/

Comment 2

[Lina Butler [:lina]]

Comment on attachment 8784682 [details]
Bug 1297945 - have PlacesSyncUtils use a more efficient technique for checking whether a GUID exists.

https://reviewboard.mozilla.org/r/74022/#review72056

Awesome, thanks! r+ with a couple of suggestions.

::: toolkit/components/places/PlacesSyncUtils.jsm:385
(Diff revision 1)
>      "Trying to update the GUIDs cache with an invalid GUID";
>  }
>  
> +// A helper for whenever we want to know if a GUID exists without actually
> +// knowing anything about it.
> +function *GUIDExists(guid) {

Nit: I've been using the `var name = Task.async(function* () { ... })` style in this file, but I'm not too bothered by this if you'd like to keep it.

::: toolkit/components/places/PlacesSyncUtils.jsm:694
(Diff revision 1)
>      if (requestedParentGuid != oldItem.parentGuid) {
>        let oldId = yield PlacesUtils.promiseItemId(oldItem.guid);
>        if (PlacesUtils.isRootItem(oldId)) {
>          throw new Error(`Cannot move Places root ${oldId}`);
>        }
> -      let parent = yield PlacesUtils.bookmarks.fetch(requestedParentGuid);
> +      isOrphan = !(yield GUIDExists(requestedParentGuid));

It looks like both uses of `GUIDExists` do `!(yield GUIDExists`. What do you think about renaming it to something like `isMissingGuid` and inverting its return value?

Comment 3

[Mark Hammond [:markh] [:mhammond]]

(In reply to Kit Cambridge [:kitcambridge] from comment #2)
> Awesome, thanks! r+ with a couple of suggestions.

Thanks Kit, I made both those changes and I'll push a new revision, but I wont land it until bug 1297941 lands.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8784682 [details]
Bug 1297945 - have PlacesSyncUtils use a more efficient technique for checking whether a GUID exists.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/74022/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ee96068b9523
have PlacesSyncUtils use a more efficient technique for checking whether a GUID exists. r=kitcambridge

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ee96068b9523