584927 - Use an async annotation query to avoid main thread lock contention

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Ed Lee :Mardak]

From bug 563538 comment 55:
> The common case may be fast, but the failure case makes us fall on our faces,
> skid about 100 feet, and then fall off a cliff without a parachute.  We need an
> async annotation API for sync at the very least stat, I think (maybe just an
> async guid API for now?).
Sync could roll its own async annotation query or places could provide a new api. Probably be better as an api in the long run, but we can try doing some initial testing to see if it helps.

Comment 1

[Brendan Eich [:brendan]]

Are we sure the conversion to async has left us with sync uses that lock us up, or is something else going wrong?

/be

Comment 2

[Shawn Wilsher :sdwilsh]

(In reply to comment #1)
> Are we sure the conversion to async has left us with sync uses that lock us up,
> or is something else going wrong?
Given every profile I've seen so far, we are certainly seeing lock contention with the async thread and the main thread.

Comment 3

[Ed Lee :Mardak]

Attachment: v1

Here's a first pass at rolling our own query to avoid the sync query implementation of Svc.Anno.getPageAnnotation.

Comment 4

[Shawn Wilsher :sdwilsh]

You really should be using createAsyncStatement if it is only going to be executed asynchronously.  And since you are already going to have for fork code cor fx4 and not fx4, you should use that in the fx4 code.

Comment 5

[Philipp von Weitershausen [:philikon]]

Comment on attachment 463668 [details] [diff] [review]
v1

> // Create some helper functions to handle GUIDs
> function setGUID(uri, guid) {
>   if (arguments.length == 1)
>     guid = Utils.makeGUID();
>   Utils.anno(uri, GUID_ANNO, guid, "WITH_HISTORY");
>   return guid;
> }

So I'm guessing the lock-ups have only been observed on annotation reads, not writes? Or why aren't changing this write to be async as well?

> function GUIDForUri(uri, create) {
>-  try {
>-    // Use the existing GUID if it exists
>-    return Utils.anno(uri, GUID_ANNO);
>+  let stm = GUIDForUri.guidStm;
>+  if (stm == null) {
>+    stm = GUIDForUri.guidStm = Svc.History.DBConnection.createStatement(

Like Shawn said, we should probably use createAsyncStatement when available (Gecko 2.0) -- see bug 583847. Also, if we're going to hang on to the statement like that, we'll have to finalize() it on shutdown (e.g. listen to 'places-shutdown') to avoid leakage. See how the HistoryStore does it.

>+      "SELECT a.content " +
>+      "FROM moz_annos a " +
>+      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id " +
>+      "JOIN moz_places_view h ON h.id = a.place_id " +
>+      "WHERE n.name = :anno_name AND h.url = :page_url");

moz_places_view is going away on trunk soon. In that case the query will just work with s/moz_places_view/moz_places/. On previous versions where the _temp tables are present, the JOIN should probably look like this:

    "JOIN ( " +
      "SELECT id FROM moz_places_temp WHERE url = :page_url " +
      "UNION " +
      "SELECT id FROM moz_places WHERE url = :page_url " +
    ") AS h ON h.id = a.place_id " +
    "WHERE n.name = :anno_name");

Also see bug 583852.

Comment 6

[Shawn Wilsher :sdwilsh]

So, given the later discussion in bug 563538, this is blocks the release.  On every page load Sync is going to be asking about a guid for each entry added, and on pages with iframes, that's going to be more than once.

Assigning to Mardak since he's already got a patch up for this.

Comment 7

[Ed Lee :Mardak]

philiKON, you did the rewrite from _view to SELECT UNION with backwards compatibility? Also, as you pointed out we might need an async query for writing as well?

Perhaps these changes should be rolled into a custom implementation of Utils.anno so all call sites don't need to be changed. If we make it sync/async, probably best to still check each call site to make sure it's setTimeout, e.g., from history observer.

Comment 8

[Mike Shaver (:shaver emeritus)]

(In reply to comment #6)
> On
> every page load Sync is going to be asking about a guid for each entry added,
> and on pages with iframes, that's going to be more than once.

It's not going to be doing that in the page-load path, though, right?  It's off on some other thread or otherwise async to the important page-load completion?

Comment 9

[Shawn Wilsher :sdwilsh]

(In reply to comment #8)
> It's not going to be doing that in the page-load path, though, right?  It's off
> on some other thread or otherwise async to the important page-load completion?
It's not going to block page loading, unless you happen to load another page when it's running.

Comment 10

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> Perhaps these changes should be rolled into a custom implementation of
> Utils.anno so all call sites don't need to be changed.

There are two uses of Utils.anno. The history engine uses page annotations whereas the bookmarks uses item annotations. Utils.anno wraps around the two separate annotation APIs, but the queries are different so rewriting Utils.anno as it is wouldn't make much sense. We want to do it separately for bookmarks and history. Since history is tracked on page load, making page annotations async is a priority (bug 606062).

> If we make it
> sync/async, probably best to still check each call site to make sure it's
> setTimeout, e.g., from history observer.

Hmm, so history observers aren't called async already?

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): Factor GUIDForUri and setGUID into HistoryStore

Factor GUIDForUri and setGUID into HistoryStore

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Async query to get page annotations

Use an async query to get page annotations in the history engine

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Async queries to set page annotations

Use async queries to set page annotations in the history engine

Comment 14

[Mike Connor [:mconnor]]

Comment on attachment 485536 [details] [diff] [review]
Part 2 (v1): Async query to get page annotations

Looks good, but should get sdwilsh to take a quick look at the queries.

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 485539 [details] [diff] [review]
Part 3 (v1): Async queries to set page annotations

looks good to me, but want places team review.

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 485536 [details] [diff] [review]
Part 2 (v1): Async query to get page annotations

>+  get _guidStm() {
>+    let base =
>+      "SELECT a.content " +
probably makes sense to do |a.content AS guid| to make your code clearer

>+      "FROM moz_annos a " +
>+      "JOIN moz_anno_attributes n ON n.id = a.anno_attribute_id ";
>+    // Gecko <2.0
>+    if (this._haveTempTables)
>+      return this._getStmt(base +
>+        "JOIN ( " +
>+          "SELECT id FROM moz_places_temp WHERE url = :page_url " +
>+          "UNION " +
>+          "SELECT id FROM moz_places WHERE url = :page_url " +
>+        ") AS h ON h.id = a.place_id " +
>+        "WHERE n.name = :anno_name");
>+    // Gecko 2.0
>+    return this._getStmt(base +
>+      "JOIN moz_places h ON h.id = a.place_id " +
>+      "WHERE n.name = :anno_name AND h.url = :page_url");
>+  },
I would expect this to bind :anno_name before returning since it's _guidStm and not a generic annotation getter...

>   GUIDForUri: function GUIDForUri(uri, create) {
>+    let stm = this._guidStm;
>+    stm.params.anno_name = GUID_ANNO;
>+    stm.params.page_url = uri.spec ? uri.spec : uri;
...and then only bind the url here.

r=sdwilsh

Comment 17

[Shawn Wilsher :sdwilsh]

Comment on attachment 485539 [details] [diff] [review]
Part 3 (v1): Async queries to set page annotations

>+  get _checkPageAnnotationStm() {
>+    let base =
>+      "SELECT h.id AS place_id, " +
>+        "(SELECT id FROM moz_anno_attributes WHERE name = :anno_name) AS name_id, " +
>+        "a.id AS anno_id, a.dateAdded AS anno_date ";
>+    // Gecko <2.0
>+    if (this._haveTempTables)
>+      return this._getStmt(base +
>+        "FROM (SELECT id FROM moz_places_temp WHERE url = :page_url " +
>+              "UNION " +
>+              "SELECT id FROM moz_places WHERE url = :page_url) AS h " +
>+        "LEFT JOIN moz_annos a ON a.place_id = h.id " +
>+                             "AND a.anno_attribute_id = name_id");
Oh gosh you didn't brace this one line if?  Please do this if it ends up being more than one line of code in an editor always (I think this applies to the last patch too.

>+    // Ensure annotation name exists
>+    let stmt = this._addAnnotationNameStm;
>+    stmt.params.anno_name = GUID_ANNO;
I guess the question I have here is "do you set other annotations besides guids here?  If not, you should make the statement getter be called something with guid in it, and bind GUID_ANNO before returning it like the get guid stuff.

On a somewhat related note, does getStmt return a mozIStorageAsyncStatement or a mozIStorageStatement?

r=sdwilsh

Comment 18

[Philipp von Weitershausen [:philikon]]

(In reply to comment #17)
> On a somewhat related note, does getStmt return a mozIStorageAsyncStatement or
> a mozIStorageStatement?

If available (Gecko 2.0), it returns the async variant.

So, I guess the whole "have GUIDs provided by places" effort (bug 607107) will obsolete this work. I'd rather avoid landing a bunch of code that would probably get ripped out again anyway (not to mention that it's largely a copy'n'paste job from places anyway). Are there any reasons for landing this nonetheless?

Comment 19

[Mike Connor [:mconnor]]

I would rather have this in as a hedge in the meantime.  GUID stuff may not make Fennec b3, f.e.

Comment 20

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Async query to get page annotations

Address sdwilsh's review comments.

Comment 21

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v2): Async queries to set page annotations

Address sdwilsh's review comments

Comment 22

[Philipp von Weitershausen [:philikon]]

Landed:

part 1: http://hg.mozilla.org/services/fx-sync/rev/44707aad2320
part 2: http://hg.mozilla.org/services/fx-sync/rev/98fa78802def
part 3: http://hg.mozilla.org/services/fx-sync/rev/cc2db84bddbc

Should we resolve this? The original bug would also have us do the same thing for item annotations (bookmark engine) and potentially have the history tracker do its tracking async (a la setTimeout).

Item annotations would be covered by places gaining GUID support proper. Shawn, are history observers notified synchronously? If so, we should probably do the async tracking. If not we probably don't have to bother...

Comment 23

[Shawn Wilsher :sdwilsh]

(In reply to comment #22)
> Item annotations would be covered by places gaining GUID support proper. Shawn,
> are history observers notified synchronously? If so, we should probably do the
> async tracking. If not we probably don't have to bother...
Yes and no.  It depends on the situation.

Comment 24

[Philipp von Weitershausen [:philikon]]

(In reply to comment #23)
> (In reply to comment #22)
> > Item annotations would be covered by places gaining GUID support proper. Shawn,
> > are history observers notified synchronously? If so, we should probably do the
> > async tracking. If not we probably don't have to bother...
> Yes and no.  It depends on the situation.

Mostly interested in onVisit here.

Comment 25

[Shawn Wilsher :sdwilsh]

(In reply to comment #24)
> Mostly interested in onVisit here.
I guess it also depends on what you consider "fired synchronously" or "fired asynchronously" means here.  On trunk, and in the places branch, it's dispatched after the main thread is notified that our async write was completed (that is, unless somebody called nsINavHistorService::addVisit which will do it before that method returns).

Comment 26

[Philipp von Weitershausen [:philikon]]

Resolving this as we fixed this for history. Filed bug 618965 for bookmarks.