make an async getAnnotationsForItem and deprecate the synchronous one

RESOLVED FIXED in Firefox 61

Status

()

defect
P2
normal
RESOLVED FIXED
5 years ago
11 months ago

People

(Reporter: mak, Assigned: standard8)

Tracking

(Blocks 2 bugs, {perf})

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [fxperf][fxsearch])

Attachments

(4 attachments)

(Reporter)

Description

5 years ago
we need an async version of getAnnotationsForItem and remove the synchronous version.
(Reporter)

Updated

5 years ago
Blocks: 1071511
(Reporter)

Updated

4 years ago
Blocks: OMTPlaces
(Reporter)

Updated

4 years ago
Priority: -- → P3
(Reporter)

Updated

4 years ago
Keywords: perf
(Reporter)

Updated

2 years ago
See Also: → 1312226
(Reporter)

Updated

2 years ago
Blocks: 1047820
(Reporter)

Updated

2 years ago
Priority: P3 → P2
(Reporter)

Updated

2 years ago
Depends on: 1131491
(Reporter)

Updated

2 years ago
No longer depends on: PlacesAsyncTransact
(Reporter)

Comment 1

a year ago
This seems to be a common cause of long janks on arewesmoothyet.com
I'm not sure we can completely remove the sync APIs due to D&D code, but we can probably provide an async alternative at least, for all the code points that can use it.
Whiteboard: [fxperf]
(Reporter)

Updated

a year ago
Whiteboard: [fxperf] → [fxperf][fxsearch]
(Assignee)

Updated

a year ago
Assignee: nobody → standard8
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
I think we could go further on the sync version of getAnnotationsForItem to improve performance better by merging the two calls within it. Since I've just found bug 1047820, I think we can take it further there.
(Reporter)

Comment 7

a year ago
mozreview-review
Comment on attachment 8963522 [details]
Bug 1047819 - Add a basic test for PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232460/#review237896
Attachment #8963522 - Flags: review?(mak77) → review+
(Reporter)

Comment 8

a year ago
mozreview-review
Comment on attachment 8963523 [details]
Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232462/#review237900

I suspect the original shape was due to the presence of binary annotations, for which you likely didn't want to fetch the value, plus the variant complication.
But this is a fine interim change while we move on with removing useless annos.

::: toolkit/components/places/nsAnnotationService.cpp:995
(Diff revision 1)
>  
>  
>  NS_IMETHODIMP
>  nsAnnotationService::GetItemAnnotationInfo(int64_t aItemId,
>                                             const nsACString& aName,
> +                                           nsIVariant** aValue,

nit: usually per xpcom convention output pointers use _lowerCamelCase instead of aCamelCase
Attachment #8963523 - Flags: review?(mak77) → review+
(Reporter)

Comment 9

a year ago
mozreview-review
Comment on attachment 8963524 [details]
Bug 1047819 - Add an async version of PlacesUtils.getAnnotationsForItem.

https://reviewboard.mozilla.org/r/232464/#review237908

::: toolkit/components/places/PlacesUtils.jsm:106
(Diff revision 1)
> + * @param aItemId
> + *        The identifier of the itme for which annotations are to be
> + *        retrieved.
> + * @return Array of objects, each containing the following properties:
> + *         name, flags, expires, mimeType, type, value
> + */

Let's add a @note that one should instead use the async version unless there's absolutely no way to make the caller async.

::: toolkit/components/places/PlacesUtils.jsm:107
(Diff revision 1)
> + *        The identifier of the itme for which annotations are to be
> + *        retrieved.
> + * @return Array of objects, each containing the following properties:
> + *         name, flags, expires, mimeType, type, value
> + */
> +function getAnnotationsForItem(aItemId) {

Oh, I love that this is no more exposed!

::: toolkit/components/places/PlacesUtils.jsm:110
(Diff revision 1)
> + *         name, flags, expires, mimeType, type, value
> + */
> +function getAnnotationsForItem(aItemId) {
> +  var annos = [];
> +  var annoNames = PlacesUtils.annotations.getItemAnnotationNames(aItemId);
> +  for (var i = 0; i < annoNames.length; i++) {

can use for...of

::: toolkit/components/places/PlacesUtils.jsm:1214
(Diff revision 1)
>     * @return Array of objects, each containing the following properties:
>     *         name, flags, expires, mimeType, type, value
>     */
> -  getAnnotationsForItem: function PU_getAnnotationsForItem(aItemId) {
> -    var annosvc = this.annotations;
> -    var annos = [];
> +  async promiseAnnotationsForItem(aItemId) {
> +    let db =  await PlacesUtils.promiseDBConnection();
> +    let rows = await db.execute(

executeCached

::: toolkit/components/places/PlacesUtils.jsm:1217
(Diff revision 1)
> -  getAnnotationsForItem: function PU_getAnnotationsForItem(aItemId) {
> -    var annosvc = this.annotations;
> -    var annos = [];
> -    var annoNames = annosvc.getItemAnnotationNames(aItemId);
> -    for (var i = 0; i < annoNames.length; i++) {
> -      let value = {}, flags = {}, exp = {}, storageType = {};
> +  async promiseAnnotationsForItem(aItemId) {
> +    let db =  await PlacesUtils.promiseDBConnection();
> +    let rows = await db.execute(
> +      `SELECT n.name, a.content, a.expiration, a.flags
> +       FROM moz_anno_attributes n
> +       JOIN moz_items_annos a ON a.anno_attribute_id = n.id

nit: It doesn't matter in practice, but I'd probably invert the from and join tables, since logically what we are trying to select are single annos, not attributes

::: toolkit/components/places/PlacesUtils.jsm:1219
(Diff revision 1)
> -    var annos = [];
> -    var annoNames = annosvc.getItemAnnotationNames(aItemId);
> -    for (var i = 0; i < annoNames.length; i++) {
> -      let value = {}, flags = {}, exp = {}, storageType = {};
> -      annosvc.getItemAnnotationInfo(aItemId, annoNames[i], value, flags, exp, storageType);
> -      annos.push({name: annoNames[i],
> +    let rows = await db.execute(
> +      `SELECT n.name, a.content, a.expiration, a.flags
> +       FROM moz_anno_attributes n
> +       JOIN moz_items_annos a ON a.anno_attribute_id = n.id
> +       WHERE a.item_id = :item_id
> +      `, { item_id: aItemId });

just name both the argument and the param "itemId" and make this a shorthand
Attachment #8963524 - Flags: review?(mak77) → review+
(Reporter)

Comment 10

a year ago
mozreview-review
Comment on attachment 8963525 [details]
Bug 1047819 - Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations.

https://reviewboard.mozilla.org/r/232466/#review237918
Attachment #8963525 - Flags: review?(mak77) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

a year ago
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2b99ec4dca6
Add a basic test for PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/cb8cbe3c000f
Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/6bc3bc48fc87
Add an async version of PlacesUtils.getAnnotationsForItem. r=mak
https://hg.mozilla.org/integration/autoland/rev/729748e603e5
Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations. r=mak
You need to log in before you can comment on or make changes to this bug.