822710 - Reduce queries load on Downloads view opening

Status: RESOLVED FIXED

(Firefox :: Downloads Panel, defect, P1)

Description

[Marco Bonardo [:mak]]

Currently we read 3 annos per download, in my profile I have 500 downloads, that means on the view startup we run 1500 queries.

I'll hack up a sucky workaround to reduce that to 1 query and see how much this helps.

Comment 1

[Asaf Romano (gone)]

Regardless, I think it's a good idea to introduce a multiple annotations getter. Even better, a lzay all-annotation getter on result nodes.

Comment 2

[Asaf Romano (gone)]

By the way, i'm working on the IO part

Comment 3

[Asaf Romano (gone)]

By the way #2, if it turns out annotations are an overhead here. I could easily keep just destinationFileURI.

Comment 4

[Marco Bonardo [:mak]]

I think I'll go deeper, and pre-cache all annotations in the map on view startup. Even if we put a getter in the node, it's still 500 queries.

Comment 5

[Asaf Romano (gone)]

There is this getPagesWithAnnotation API that we could easily extend to also return the values.

Comment 6

[Marco Bonardo [:mak]]

I somehow like the getPagesWithAnnotation suggestion, though this API has some (7 according to mxr) add-ons using it. Since it's likely soon we'll have to make an async annotations module, I'd probably avoid double breakage.
Maybe for now we should just add a further GetPagesAndValuesForAnnotation. We likely want this to be js-only and return a { string url, jsval value } (we can't predict the value type and I'd prefer to avoid nsISupports types)

If we do this, we have 1 query per anno, that is about 3 queries.

The alternative is a Map in the view populated with a direct query, that doesn't involve special jsval management or API changes, though it's less re-usable and a bit more hackish.

I'll probably start with the former and see how it proceeds.

Comment 7

[Asaf Romano (gone)]

Yeah, I didn't mean to suggest altering the current signatures.

You don't need to use jsvals. This api uses nsIVariants and there's plenty of code to reuse for that. I think the easiest way to achieve our goal is to return an array of property bags, each with a uri (nsIURI) property and a value (nsIVaraint) property.

Comment 8

[Marco Bonardo [:mak]]

Attachment: wip

Comment 9

[Marco Bonardo [:mak]]

Attachment: wip

Comment 10

[Marco Bonardo [:mak]]

Attachment: wip

this is with jsval, unfortunately if I use a jsval in the object, I must also return a jsval from the method, instead of a native array, otherwise jsapi asserts badly trying to GC the context when I exit the method call.

I think I'll do a second try with nsIVariant and see what I like more.

Comment 11

[Marco Bonardo [:mak]]

Attachment: part1 v1.0 - Implement getAnnotationsHavingName

Yes, I definitely prefer nsIVariant (remember, Mano is always right!)

Comment 12

[Marco Bonardo [:mak]]

Attachment: part1 v1.1 - Implement getAnnotationsHavingName

Some obvious fixes.
This should be ready for review, next part will hook this into the view.

Comment 13

[Marco Bonardo [:mak]]

Attachment: part2 v1.1 - Reduce queries load on first opening

This also fixes some minor warnings

Comment 14

[Asaf Romano (gone)]

Comment on attachment 693872 [details] [diff] [review]
part1 v1.1 - Implement getAnnotationsHavingName

Review of attachment 693872 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/places/nsAnnotationService.cpp
@@ +90,5 @@
> +}
> +
> +NS_IMETHODIMP
> +Annotation::GetName(nsACString& _title)
> +{

"title"?

@@ +98,5 @@
> +
> +NS_IMETHODIMP
> +Annotation::GetValue(nsIVariant** _value)
> +{
> +   NS_ADDREF(*_value = mValue);

NS_IF_ADDREF, and fix indent.

@@ +1265,5 @@
> +    int32_t type = stmt->AsInt32(3);
> +
> +    nsCOMPtr<nsIWritableVariant> variant = new nsVariant();
> +    switch (type) {
> +      case nsIAnnotationService::TYPE_INT32: {

Now that you switched to nsIVariant you can "support" int-64 as well as the rest of this service.

@@ +1292,5 @@
> +    nsCOMPtr<nsIURI> uri;
> +    rv = NS_NewURI(getter_AddRefs(uri), url);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    nsCOMPtr<mozIAnnotation> anno = new Annotation(guid, uri, itemId,
> +                                                   aName, variant);

out-of-memory check for the new anno object.

::: toolkit/components/places/nsIAnnotationService.idl
@@ +429,5 @@
> +  /**
> +   * The URI of the place, if available, null otherwise.
> +   */
> +  readonly attribute nsIURI uri;
> +

It cannot be null as far as I can tell (and if it can, you'd need to change NS_ADDREF to NS_IF_ADDREF in the getter and remove the NS_PRECONDITION from the constructor).

@@ +434,5 @@
> +  /**
> +   * The bookmark id of the place, if available, -1 otherwise.
> +   *
> +   * @note if itemId is -1, doesn't mean the page is not bookmarked, just that
> +   *       this annotation is relative to the page, not to the bookmark.

, *it* doesn't mean

Comment 15

[Asaf Romano (gone)]

Comment on attachment 693950 [details] [diff] [review]
part2 v1.1 - Reduce queries load on first opening


>+          if (!this._cachedAnnotations.has(url)) {
>+            this._cachedAnnotations.set(url, new Map());
>+          }

nit: remove the braces.

>+  _place: null,

nit: "" as it's a string.


Many many thanks for your work here.

Comment 16

[Marco Bonardo [:mak]]

(In reply to Mano from comment #14)
> out-of-memory check for the new anno object.

new operator is infallible, no need to check it anymore


> > +  readonly attribute nsIURI uri;
> > +
> 
> It cannot be null as far as I can tell (and if it can, you'd need to change
> NS_ADDREF to NS_IF_ADDREF in the getter and remove the NS_PRECONDITION from
> the constructor).

hm, well it can be null for folders, separators.  Will fix those and maybe add a further test for those.

Comment 17

[Marco Bonardo [:mak]]

Attachment: part1 v1.2 - Implement getAnnotationsHavingName

Gavin do you have any cycles for a quick SR soon?

it's about the changes in nsIAnnotationService.idl

Comment 18

[Marco Bonardo [:mak]]

* @param aName

fixed locally to @param name

Comment 19

[Marco Bonardo [:mak]]

Attachment: part1 v1.3 - Implement getAnnotationsHavingName

Bah, there was a spurious printf, so takes less time to just update the attachment.

Comment 20

[Marco Bonardo [:mak]]

Attachment: part2 v1.2 - Reduce queries load on first opening

address comments, trivial cleanup

Comment 21

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 694350 [details] [diff] [review]
part1 v1.3 - Implement getAnnotationsHavingName

Let's go with mozIAnnotationResult for the interface name, as discussed on IRC.

Let's add a comment describing what it's supposed to represent, something like:

"Represents a place annotated with a given annotation. If a place has multiple annotations, it can be represented by multiple mozIAnnotationResults."

And I think its "name"/"value" attributes should be "annotationName"/"annotationValue", to indicate that it's representing an attribute of a different "thing" than the guid/itemId/URI attributes. The documentation for these attributes could also probably make that clear, as in e.g. "The URI of the place with this annotation.".

For the method, perhaps getAnnotationsWithName to match getItemsWithAnnotation?

sr=me with those addressed.

Comment 22

[Marco Bonardo [:mak]]

Attachment: part1 v1.4 - Implement getAnnotationsWithName

Comment 23

[Marco Bonardo [:mak]]

Attachment: part2 v1.4 - Reduce queries load on first opening

Comment 24

[Marco Bonardo [:mak]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f112f6b4d753
https://hg.mozilla.org/integration/mozilla-inbound/rev/c5a0052af398

Comment 25

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/f112f6b4d753
https://hg.mozilla.org/mozilla-central/rev/c5a0052af398