Closed Bug 1047819 Opened 9 years ago Closed 6 years ago

make an async getAnnotationsForItem and deprecate the synchronous one

Categories

(Toolkit :: Places, defect, P2)

Product:
Toolkit
Component:
Places
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: mak, Assigned: standard8)

References

(Blocks 2 open bugs)

Details

(Keywords: perf, Whiteboard: [fxperf][fxsearch])

Attachments

(4 files)

Bug 1047819 - Add a basic test for PlacesUtils.getAnnotationsForItem.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1047819 - Add an extra parameter to nsIAnnotationService.getItemAnnotationInfo to avoid an extra sync database lookup in PlacesUtils.getAnnotationsForItem.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1047819 - Add an async version of PlacesUtils.getAnnotationsForItem.
59 bytes, text/x-review-board-request
mak
: review+
Details
Bug 1047819 - Remove unused nsIAnnotationService::copyPageAnnotations/copyItemAnnotations.
59 bytes, text/x-review-board-request
mak
: review+
Details 
we need an async version of getAnnotationsForItem and remove the synchronous version.
Blocks: 1071511
Blocks: OMTPlaces
Priority: -- → P3
Keywords: perf
Depends on: PlacesAsyncTransact
See Also: → 1312226
Blocks: 1047820
Priority: P3 → P2
Depends on: 1131491
No longer depends on: PlacesAsyncTransact
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]
Whiteboard: [fxperf] → [fxperf][fxsearch]
Assignee: nobody → standard8
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.
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+
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+
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+
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+
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
Blocks: 1450861
You need to log in before you can comment on or make changes to this bug.