Closed Bug 531151 Opened 15 years ago Closed 15 years ago

Increase annotations service robustness

Categories

(Toolkit :: Places, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 3 obsolete files)

I noticed these things:
- annotations APIs are not reliable on VMs (due to UNION ALL, bug 507790), this seem to cause strange failures in my bug 520165 tests.
- annotations service runs more queries than needed, especially when setting annos
- annotations service code is unclean
- bug 484026 demonstrates we don't return good errors

So, i'll cleanup this code without touching the idl, trying to make it faster and not dependent on UNION ALL.
Blocks: 500306
I support this endeavour! (fennec invokes the annotation service on startup atm, to find its fake "bookmarks root")
Blocks: 527622
Attached patch patch v1.0 (obsolete) — Splinter Review
Cleaned up the idl (no changes, apart reporting setXXXAnnotation will throw for invalid pages/items)
Replaced most of the queries
Cleaned up and reduced code
Implemented copyXXXAnnotation (there was a stub implementation not working, but is exposed in the API :() fixed tests for it
All toolkit/Places unit tests pass with this version (will get results on tryserver)

Since i don't want to drive blindly, now i need to take some measure to at least confirm i've not regressed any performance (in some point increased robustness could have brought some unexpected hit, but i've tried to reduce query count and dependencies on other services as much as possible).

And to my future beloved reviewer: sorry, I changed a lot, but we should have done this before.
Since i'm unable to measure a performance improvement i'm changing bug title making it going toward robustness. I was still expecting some gain, but the numbers are so fluctuating that i can just say the average value is about the same.
Summary: Reduce annotations service overhead → Increase annotations service robustness
notice we can still gain on Ts through bug 500306, but i don't mean to address that bug here, i'll do that later.
Attached patch patch v1.1 (obsolete) — Splinter Review
passed tryserver without problems.

Even if performances won't change a lot (or won't change at all, but it's hard to tell, it would have to be verified on some really slow device), this makes the code more mintainable (it should also reduce it a bit even if i did not count rows), fixes 2 broken APIs, and introduces better error checking (actually used on SetXXXAnnotation, could be eventually enabled on other APIs, but i did not do to avoid breaking users, also avoids UNION ALL (i used some UNION that don't suffer from the threading issues) reducing random errors and has less dependancy on other services.

From this point should be pretty easy to extend the service for lazy statements (Ts) and binary annotations in a separate DB (DB bloat).

Sorry for the size, but at least is self contained in one service.
Attachment #414719 - Attachment is obsolete: true
Attachment #415503 - Flags: review?(dietrich)
Comment on attachment 415503 [details] [diff] [review]
patch v1.1


>diff --git a/browser/components/places/tests/unit/test_placesTxn.js b/browser/components/places/tests/unit/test_placesTxn.js
>--- a/browser/components/places/tests/unit/test_placesTxn.js
>+++ b/browser/components/places/tests/unit/test_placesTxn.js
>@@ -548,6 +548,10 @@ function run_test() {
>                       flags: 0,
>                       value: 123,
>                       expires: Ci.nsIAnnotationService.EXPIRE_NEVER };
>+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
>+           getService(Ci.nsINavHistoryService);
>+  hs.addVisit(uri("http://www.mozilla.org/"), Date.now() * 1000, null,
>+              hs.TRANSITION_TYPED, false, 0);

what's this change for?

also, indent

>+    /**
>+     * @throws NS_ERROR_ILLEGAL_VALUE if the page or the bookmark don't exist.
>+     */
>+    [noscript] void setPageAnnotationString(in nsIURI aURI,
>+                                            in AUTF8String aName,
>+                                            in AString aValue,
>+                                            in long aFlags,
>+                                            in unsigned short aExpiration);

s/don't/doesn't/

and all the other instances of this

>     /**
>      * Test for annotation existance.
>      */
>-    boolean pageHasAnnotation(in nsIURI aURI, in AUTF8String aName);
>-    boolean itemHasAnnotation(in long long aItemId, in AUTF8String aName);
>+    boolean pageHasAnnotation(in nsIURI aURI,
>+                              in AUTF8String aName);
>+    boolean itemHasAnnotation(in long long aItemId,
>+                              in AUTF8String aName);
> 

s/existance/existence/




>+#define ENUMERATE_ANNOS_OBSERVERS(_notification)                               \
>+  PR_BEGIN_MACRO                                                               \
>+  for (PRInt32 i = 0; i < mObservers.Count(); i ++)                            \
>+    mObservers[i]->_notification;                                              \
>+  PR_END_MACRO
>+

the macro name doesn't describe what it's actually doing, which is notification. please rename so that it's clear that it notifies.





> NS_IMETHODIMP
> nsAnnotationService::SetItemAnnotationString(PRInt64 aItemId,
>-                                             const nsACString& aName,
>-                                             const nsAString& aValue,
>+                                             const nsACString &aName,
>+                                             const nsAString &aValue,
>                                              PRInt32 aFlags,
>                                              PRUint16 aExpiration)
> {

why?








>+  if (!hasResult) {
>+    // We are trying to get an annotation on an invalid element.
>+    // Here we preserve the old behavior, return that we don't have an anno,
>+    // ignoring the fact itemId is invalid.
>+    // Otherwise we should return NS_ERROR_INVALID_ARG, but this will somehow
>+    // break the API.  In future we could want to be pickier.
>+    *_hasAnno = PR_FALSE;
>+  }
>+  else {
>+    PRInt64 annotationId = stmt->AsInt64(2);
>+    *_hasAnno = (annotationId > 0);
>+  }

s/element/item/


>+  // We have to check 2 things:
>+  // - if the annotation already exists we should update it.
>+  // - we should not allow setting annotations on invalid URIs or itemIds.
>+  // This query will tell us:
>+  // - whether the item or page exists.
>+  // - whether the annotation already exists.
>+  // - the nameID associated with the annotation name.
>+  // - the id and dateAdded of the old annotation, if it exists.
>+  mozIStorageStatement *stmt = isItemAnnotation ?
>+    mDBCheckItemAnnotation.get() : mDBCheckPageAnnotation.get();
>+  mozStorageStatementScoper checkAnnoScoper(stmt);
>+  rv = stmt->BindUTF8StringParameter(0, aName);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (isItemAnnotation)
>+    rv = stmt->BindInt64Parameter(1, aItemId);
>+  else
>+    rv = BindStatementURI(stmt, 1, aURI);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  PRBool hasResult;
>+  rv = stmt->ExecuteStep(&hasResult);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  if (!hasResult) {
>+    // We are trying to create an annotation on an invalid element.
>+    return NS_ERROR_INVALID_ARG;
>   }

s/element/item/

>+  rv = (*_statement)->BindInt64Parameter(kAnnoIndex_PageOrItem, fkId);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = (*aStatement)->BindInt32Parameter(kAnnoIndex_Flags, aFlags);
>+  rv = (*_statement)->BindInt64Parameter(kAnnoIndex_NameID, nameID);
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = (*aStatement)->BindInt32Parameter(kAnnoIndex_Expiration, aExpiration);
>+  // MimeType and Content will be binded by the caller.
>+  rv = (*_statement)->BindInt32Parameter(kAnnoIndex_Flags, aFlags);

s/binded/bound/

mostly just nits, r=me. i'd like to see all of this repeated per-type code compacted somehow, maybe in a followup, or just move the whole thing to JS eventually?
Attachment #415503 - Flags: review?(dietrich) → review+
(In reply to comment #6)
> (From update of attachment 415503 [details] [diff] [review])
> 
> >diff --git a/browser/components/places/tests/unit/test_placesTxn.js b/browser/components/places/tests/unit/test_placesTxn.js
> >--- a/browser/components/places/tests/unit/test_placesTxn.js
> >+++ b/browser/components/places/tests/unit/test_placesTxn.js
> >@@ -548,6 +548,10 @@ function run_test() {
> >                       flags: 0,
> >                       value: 123,
> >                       expires: Ci.nsIAnnotationService.EXPIRE_NEVER };
> >+  var hs = Cc["@mozilla.org/browser/nav-history-service;1"].
> >+           getService(Ci.nsINavHistoryService);
> >+  hs.addVisit(uri("http://www.mozilla.org/"), Date.now() * 1000, null,
> >+              hs.TRANSITION_TYPED, false, 0);
> 
> what's this change for?

SetPageAnno will throw if the page does not exist, so we have to add it
 
> also, indent

what's wrong with indent? 

> >+#define ENUMERATE_ANNOS_OBSERVERS(_notification)                               \
> >+  PR_BEGIN_MACRO                                                               \
> >+  for (PRInt32 i = 0; i < mObservers.Count(); i ++)                            \
> >+    mObservers[i]->_notification;                                              \
> >+  PR_END_MACRO
> >+
> 
> the macro name doesn't describe what it's actually doing, which is
> notification. please rename so that it's clear that it notifies.

well, it's consistent with all of our notification enumerators, it enumerates the observers calling the passed in method for each of them, btw i'll look for a better name.

> > NS_IMETHODIMP
> > nsAnnotationService::SetItemAnnotationString(PRInt64 aItemId,
> >-                                             const nsACString& aName,
> >-                                             const nsAString& aValue,
> >+                                             const nsACString &aName,
> >+                                             const nsAString &aValue,
> >                                              PRInt32 aFlags,
> >                                              PRUint16 aExpiration)
> > {
> 
> why?

i thought we decided to go for toward-varname decorations, can we decide for a final Places code style while in MV? :)

> >+  if (!hasResult) {
> >+    // We are trying to get an annotation on an invalid element.
> >+    // Here we preserve the old behavior, return that we don't have an anno,
> >+    // ignoring the fact itemId is invalid.
> >+    // Otherwise we should return NS_ERROR_INVALID_ARG, but this will somehow
> >+    // break the API.  In future we could want to be pickier.
> >+    *_hasAnno = PR_FALSE;
> >+  }
> >+  else {
> >+    PRInt64 annotationId = stmt->AsInt64(2);
> >+    *_hasAnno = (annotationId > 0);
> >+  }
> 
> s/element/item/

i wanted to avoid confusion with "item" inteded as bookmark (unfortunatly we use that association everywhere), any idea for a more generic name since the method applied to both bookmarks and pages?

> mostly just nits, r=me. i'd like to see all of this repeated per-type code
> compacted somehow, maybe in a followup, or just move the whole thing to JS
> eventually?

we can compact it with some macro, but i did not go further, it was large enough. If we are going to convert it to js there is much more stuff we could take in count for a redesign, but since both history and bookmarks are cpp and this is highly tied with them, maybe it's too early to move to js for binary compat. btw once this code is cleaned up converting to js should be barely a 1:1 conversion.
but mostly, we should first measure perf difference for a js impl, annos are one of the interesting features we provide to extensions, so we should be sure we don't kills perf. Since the code now will be pretty simple, js conversion benefits are a bit reduced, but if we see same perfs, then we can most likely do it to help new devs.
> > also, indent
> 
> what's wrong with indent? 

bah, nothing, sorry misread the patch

> well, it's consistent with all of our notification enumerators, it enumerates
> the observers calling the passed in method for each of them, btw i'll look for
> a better name.

yeah i'd prefer those other's be renamed eventually as well. this is optional nit, up to you.


> > s/element/item/
> 
> i wanted to avoid confusion with "item" inteded as bookmark (unfortunatly we
> use that association everywhere), any idea for a more generic name since the
> method applied to both bookmarks and pages?

"bookmark or history entry" for extreme explicitness?
Attached patch patch v1.2 (obsolete) — Splinter Review
comments addressed
Attachment #415503 - Attachment is obsolete: true
Attached patch patch v1.3Splinter Review
fix a couple warnings
Attachment #416983 - Attachment is obsolete: true
Does this patch affect bug 468754 at all - or is that a separate issue?
it's a different issue
http://hg.mozilla.org/mozilla-central/rev/9c732843f679
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Blocks: 510466
Blocks: 573679
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: