Closed Bug 1297945 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: Sync, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 51
Tracking Status
firefox51 --- fixed

People

(Reporter: markh, Assigned: markh)

References

Details

Attachments

(1 file)

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 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?
Attachment #8784682 - Flags: review?(kcambridge) → review+
Priority: -- → P1
(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.
QA Whiteboard: [sync-tracker]
Pushed by mhammond@skippinet.com.au:
https://hg.mozilla.org/integration/autoland/rev/ee96068b9523
have PlacesSyncUtils use a more efficient technique for checking whether a GUID exists. r=kitcambridge
https://hg.mozilla.org/mozilla-central/rev/ee96068b9523
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
You need to log in before you can comment on or make changes to this bug.