Closed Bug 822710 Opened 7 years ago Closed 7 years ago

Reduce queries load on Downloads view opening

Categories

(Firefox :: Downloads Panel, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 20

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(2 files, 9 obsolete files)

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.
Regardless, I think it's a good idea to introduce a multiple annotations getter. Even better, a lzay all-annotation getter on result nodes.
By the way, i'm working on the IO part
By the way #2, if it turns out annotations are an overhead here. I could easily keep just destinationFileURI.
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.
There is this getPagesWithAnnotation API that we could easily extend to also return the values.
Priority: -- → P1
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.
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.
Attached patch wip (obsolete) — Splinter Review
Attached patch wip (obsolete) — Splinter Review
Attachment #693816 - Attachment is obsolete: true
Attached patch wip (obsolete) — Splinter Review
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.
Attachment #693840 - Attachment is obsolete: true
Yes, I definitely prefer nsIVariant (remember, Mano is always right!)
Attachment #693854 - Attachment is obsolete: true
Some obvious fixes.
This should be ready for review, next part will hook this into the view.
Attachment #693862 - Attachment is obsolete: true
Attachment #693872 - Flags: review?(mano)
This also fixes some minor warnings
Attachment #693950 - Flags: review?(mano)
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
Attachment #693872 - Flags: review?(mano) → review+
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.
Attachment #693950 - Flags: review?(mano) → review+
(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.
Gavin do you have any cycles for a quick SR soon?

it's about the changes in nsIAnnotationService.idl
Attachment #693872 - Attachment is obsolete: true
Attachment #694349 - Flags: superreview?(gavin.sharp)
* @param aName

fixed locally to @param name
Bah, there was a spurious printf, so takes less time to just update the attachment.
Attachment #694349 - Attachment is obsolete: true
Attachment #694349 - Flags: superreview?(gavin.sharp)
Attachment #694350 - Flags: superreview?(gavin.sharp)
address comments, trivial cleanup
Attachment #693950 - Attachment is obsolete: true
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.
Attachment #694350 - Flags: superreview?(gavin.sharp) → superreview+
Attachment #694350 - Attachment is obsolete: true
Attachment #694356 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.