501689 - Investigate making storage asynchronous

Status: RESOLVED FIXED

(Calendar :: Provider: Local Storage, defect)

Description

[Philipp Kewisch [:Fallen][📱📆]]

As noted in https://developer.mozilla.org/en/Storage#Executing_a_Statement it is now possible to make asynchronous storage requests.

We are assuming that storage is synchronous in quite some places, but given the horrible performance of the storage provider, I think its worth investigating this.

This may possibly be of interest for bug 498968 too.

Comment 1

[Philipp Kewisch [:Fallen][📱📆]]

Maybe we can simplify code using processNextEvent, i.e:

function processSynchronous(func) {
   let done = false;
   let cb = {
       onResult: function() { 
           done = true;
       }
    };

    func(cb);
    while(!done) {
        cal.processPendingEvent();
    }
}

Not sure if this prone to deadlocks though.

Comment 2

[Daniel Boelzle [:dbo]]

(In reply to comment #0)
> We are assuming that storage is synchronous in quite some places, but given the
I can only think of caching. We only require synchronous write calls there, because those are easier to handle when sync'ing the calendars. I think we're fine reading a storage calendar asynchronously (like any other calendar). +1, hope this helps.

Comment 3

[Shawn Wilsher :sdwilsh]

If there is a particular pain point in the current async API, please let me know and we can work out how to come up with a better API or figure out how to use the current API to do something.

Comment 4

[Philipp Kewisch [:Fallen][📱📆]]

Just to note, some simple testing with the async API gave quite a notable performance improvement. Given the amount of work it is (we will have to change a lot of code, which comes close to a rewrite), I think we should implement storage2 (i.e save whole ics data in a database BLOB field and just keep some columns for indexes) first and then just use the async api.

I noticed (this might not have to do with the async storage API though) is i.e when selecting events and tasks, we need to do two queries, but only send our onOperationComplete once, which means we will have to cancel the other query if an item is found. This is just a bit cludgy codewise, but its ok.

Shawn, one thing I did notice is that the new API (mozIStorageRow row in mozIStorageStatement) doesnt have the nice js accessors that mozIStorageStatementWrapper provided, so I need to do aResultSet.getNextRow().getResultByName("foo") instead of just aResultsSet.getNextRow().foo

Any chance this will come back?

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> Any chance this will come back?
Nobody has filed a bug on it, but it wouldn't be hard to do.

Comment 6

[Philipp Kewisch [:Fallen][📱📆]]

(In reply to comment #5)
> (In reply to comment #4)
> > Any chance this will come back?
> Nobody has filed a bug on it, but it wouldn't be hard to do.
Filed bug 512761.

Comment 7

[Philipp Kewisch [:Fallen][📱📆]]

While it is nice to make storage faster, I think the current state is sufficient for an 1.0 release. As mvl has noted often, the real performance issues are likely in the views instead.

For this bug to be a candidate for 1.0, retrieving the result from the database needs to take a while and as I've seen it thats not the case.

Comment 8

[Ben Lerner]

I'm not sure if this bug is still active or not, but to add one datapoint from my experience: with my calendars (GData provider, mostly), with caching enabled (which was suggested to fix other perf issues), when Lightning decides to refresh, I see hundreds of accesses to cache.sqlite-journal -- I'm guessing one per event, maybe, over however long a timespan Lightning is reloading.  (If there's an easy way for me to debug those accesses, let me know; I'd be happy to try.)  So while each access is pretty quick, the whole thing freezes TB completely, for minutes at a time.  Windows even includes a (Non-responsive) warning in the title bar and fades the window contents...  If there's any way to push the cache accesses off the main thread, that would be a huge win.

Comment 9

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: WiP - v1

I actually have a patch that is almost there, I just need to fix some callers in the caldav provider and others, that assume that the storage calendar is synchronous.

If you have other means of testing than a caldav calendar, you could give this test a spin (requires patch for bug 785733 applied, will push this soon) and see how the I/O is. I guess the I/O won't be much better, but at least it won't be blocking the main thread.

Downside to this patch is that its much more work to upgrade the timezones components. I haven't tested this on a large database, but this could really bring Thunderbird to a stall on startup if exactly the timezone the user is in has changed. We might need to make that process asynchronous too, although that means more complexity for processing items (main UI loads, user might change an event, then the timezone upgrade could be changing it back).

With this bug fixed, we should investigate places where lots of operations are being done and maybe wrap more of them into transactions.

There is also bug 412914 which suggests putting each calendar into a separate sqlite file, although I hope this is not needed as there is a higher regression risk on upgrade and we don't have unit tests for upgrades.

Comment 10

[Philipp Kewisch [:Fallen][📱📆]]

Oh and don't forget to backup your profile, this does a database upgrade.

Comment 11

[Ben Lerner]

@comment 9, the only calendars I have in active use right now are Google calendars, which IIRC are caldav (I'm fuzzy on the differences now between built-in caldav support and gdata provider suppoert; sorry).  If you have a patch for gdata calendars, or if my understanding is wrong, I'm happy to try to test the patch.

Comment 12

[Philipp Kewisch [:Fallen][📱📆]]

The actual fix here is made in the storage calendar, which is what is used as the "cache" for all other calendar types. You could try Provider for Google Calendar with the cache option, which is that thing where you select "Google Calendar" in the new calendar dialog, or the thing with "feeds" in the URL which you see in the calendar properties dialog.

Comment 13

[Philipp Kewisch [:Fallen][📱📆]]

Just a note, bug 411894 has just added executeSimpleSQLAsync, so simple statements can now also be done asynchronously.

Comment 14

[Philipp Kewisch [:Fallen][📱📆]]

Also, check out Sqlite.jsm. Not sure its worth porting, but something to keep in mind.

Comment 15

[Reid Anderson]

Attachment: WiP -v1 with promises

Comment 16

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8427112 [details] [diff] [review]
WiP -v1 with promises

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

Thanks for the WiP patch, here are a few comments. In general, I would organize the promises in a way that the helper functions (runCalendarStatement, runItemStatement) return a promise, which is resolved once the statement has run. This has a number of cool side effects, which I will go into detail below. Setting f- for now because we are not quite there yet - not to be taken as discouragement, I know this is WiP :-)

::: calendar/providers/storage/calStorageCalendar.js
@@ +95,5 @@
> +            handleCompletion: handleCompletion
> +        }, "Error deleting all events");
> +        self.runCalendarStatement(self.mDeleteAllMetaData, {
> +            handleCompletion: handleCompletion
> +        }, "Error deleting all metadata");

This is a nice example to show what you can do with promises. Check out this code (simplified, I've optimized away some stuff like null checks and try/catch)

let deleteItems = self.promiseCalendarStatement(self.mDeleteAllItems);
let deleteMeta = self.promiseCalendarStatement(self.mDeleteAllMetaData);

Promise.all([deleteItems, deleteMeta]).then(function success(values) {
  listener.onDeleteCalendar(aCalendar, Cr.NS_OK, null);
}, function failure(values) {
  // handle error somehow
});

This code makes the handleCompletion function obsolete, you don't have to count the calls anymore.

@@ +154,5 @@
> +        let newItem = aItem.clone();
> +        return this.adoptItem(newItem, aListener);
> +    },
> +
> +    adoptItem: function cSC_adoptItem(aItem, aListener) {

This is a function where you can nicely use Task.jsm:

Task.spawn(function*() {
  // do things that sanityCheck did until now
  if (self.readOnly) {
    // throwing causes the promise to reject
    // another possibility would be to return something special and 
    // modify the success function afterwards to handle it. This way 
    // you could split a code exception from an "expected" case like 
    // this one.
    throw new Components.Exception(...); 
  }
  ...

  let oldItem = yield self.promiseItemById(aItem.id);
  ...

  yield self.promiseWriteItem(parentItem);

  ...
  return parentItem; // or aItem? Not sure what we did before.
}).then(
  function success(aItem) notifyListener(aItem, Cr.NS_OK),
  function failure(aError) notifyListener(aError.message, aError.code) // not sure about message/code. Depends on exception type.
);

Note that the return statement above should not be mistaken with the return from the actual function (i.e the calIOperation, or null). It merely gives the subsequent promise a value that it is resolved with.

@@ +203,5 @@
> +
> +       sanityCheck();
> +    },
> +
> +    modifyItem: function cSC_modifyItem(aNewItem, aOldItem, aListener) {

Really any function that used to use inner functions to handle control flow is a candidate for Task.jsm. More generally, anything that has an asynchronous code flow.

@@ +300,5 @@
> +            return;
> +        }
> +        let self = this;
> +
> +        let promise = self.deleteItemById(aItem.id, function() {});

I'm sure this is just a WiP part, but make sure you remove the empty function at the end, it should be obsolete since its handled by promises now.

@@ +944,5 @@
> +    uncacheItem: function(aId) {
> +        delete this.mItemCache[aId];
> +        delete this.mFlagCache[aId];
> +        delete this.mRecEventCache[aId];
> +        delete this.mRecTodoCache[aId];

Just a minor comment, but we might want to convert these to a js Map instead to speed things up.

@@ +951,5 @@
> +    getCachedItem: function(aId) {
> +        return this.mItemCache[aId];
> +    },
> +
> +    runStatement: function runStatement(stmt, observer, errorMsg, synchronous) {

Here is a function you want to convert into a promise based function as far as possible. instead of using the observer, make it return the promise and resolve/reject as needed.

@@ +1116,5 @@
>  
>      //
>      // get item from db or from cache with given iid
>      //
> +    getItemById: function cSC_getItemById(aId, aCallback) {

You will want to use Task.jsm here too and return the promise it returns, i.e

getItemById: function(aId) {
  return Task.spawn(function*() {
    // magic
  });
};

Comment 17

[Philipp Kewisch [:Fallen][📱📆]]

> > +    runStatement: function runStatement(stmt, observer, errorMsg, synchronous) {
>
> Here is a function you want to convert into a promise based function as far as possible. instead of 
> using the observer, make it return the promise and resolve/reject as needed.

Too much context here, I obviously meant runStatement.

Comment 18

[Reid Anderson]

Attachment: Updated Promises patch

Comment 19

[Reid Anderson]

There are a couple functions like getItems, and the offline functions that still need callbacks to be elimintated, but I think those should just use functions that I've already written. There isn't any error handling as of yet, as I'm not entirely sure what the best way to go about it is. I'm also uncertain as to what the best way to go about setting the params of the statement calls is. Previously this was done in the handleInit callback. As it is now, I just loop through and set the needed params every time right before the statement call, but it repeats a lot of code.

Comment 20

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8431443 [details] [diff] [review]
Updated Promises patch

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

Setting f+ since I've commented on this patch per email. If I've missed anything please let me know!

Comment 21

[Reid Anderson]

Attachment: AsyncPatch.diff

Comment 22

[Reid Anderson]

Attachment: Patch

Comment 23

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8451446 [details] [diff] [review]
Patch

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

Very long patch usually means it takes a while for everything to be ready for checkin, so don't feel turned down by these comments. There are a few that can be taken care of in a followup patch, but then again others need taking care before checkin. r- for now to get a new version with comments considered. If you have questions feel free to ask any time: on the bug, via email, or IRC.

Thanks for the great work so far!

::: calendar/providers/caldav/calDavCalendar.js
@@ +1120,2 @@
>          this.mOfflineStorage.getItem(this.mHrefIndex[path],
>                                       getItemListener);

This can be done in a followup bug, but maybe this should be simplified in a helper like the calAsyncUtils.jsm I sent you.

let pcal = promisifyCalendar(this.mOfflineStorage);
pcal.getItem(this.mHrefIndex[path]).then((foundItem) => {

}, (aReason) = {

});

@@ +1141,1 @@
>          return isDeleted;

This won't work, isDeleted will always be false, because this return statement will execute before the async promise is resolved. You need to make all callers of deleteTargetCalendarItem asynchronous, creating an outer promise that resolves with the value of isDeleted. A Task might be best here:
function deleteTargetCalendarItem(...) {
   return Task.spawn(function*() {
  
    let foundItem = yield pcal.getItem(this.mHrefIndex[path]);
    ...
    if (wasInboxItem....) {
      ...
      isDeleted = true;
    }

    return isDeleted;
  }
}

Then in the calling functions call deleteTargetCalendarItem and wait for the promise to resolve.

::: calendar/providers/storage/calStorageCalendar.js
@@ +68,5 @@
>      // calICalendarProvider interface
>      //
> +    get prefChromeOverlay() null,
> +    get displayName() cal.calGetString("calendar", "storageName"),
> +    //createCalendar: throw NS_ERROR_NOT_IMPLEMENTED;,

Remove the commented out line.

@@ +92,2 @@
>  
> +            listener.onDeleteCalendar(aCalendar, Components.results.NS_ERROR_FAILURE,null);

What does aReason contain? Can you forward aReason to the listener?

@@ +150,5 @@
> +        return this.adoptItem(newItem, aListener);
> +    },
> +
> +    adoptItem: function cSC_adoptItem(aItem, aListener) {
> +        self = this;

let self = this;

@@ +158,5 @@
> +        }
> +
> +        Task.spawn(function*() {
> +            if (self.readOnly) {
> +                return -1;

Instead of using constants here, I would suggest throwing either Components.Exception or just plain Error and then using the rejection function to handle them. Error might even have better perf because Components.Exception is an XPCOM exception.

if (self.readOnly) {
  throw Components.Exception("Calendar is readonly", cIE.CAL_IS_READONLY);
}
...

then((aItem) => { 
  notifyListener(aItem, Components.results.NS_OK);
}, (ex) => {
  notifyListener(ex.message, ex.result);
});

Same goes for the other functions where you used constants.

@@ +484,5 @@
> +                    let cachedFlag = self.mFlagCache[itemId];
> +                    if (offline_flag_matches(cachedFlag)) {
> +                        let expandedItems = expandItems(item, checkTaskCompleted);
> +                        if (queue.enqueue(expandedItems)) {
> +                            return cal.forEach.BREAK;

Might have missed it, but I think you aren't using cal.forEach anymore? in that case a normal break works better.

@@ +532,5 @@
> +
> +        return Task.spawn(function*() {
> +            stmt = self.mEditItemOfflineFlag;
> +            stmt.params.offline_journal = flag || null;
> +            let editOfflineFlag = self.runItemStatement(stmt,aItem.id,"Error setting offline flag for " + aItem.id);

Hmm you just define the variable here? Should this yield or return something?

@@ +560,5 @@
> +                if (oldOfflineJournalFlag != cICL.OFFLINE_FLAG_CREATED_RECORD &&
> +                    oldOfflineJournalFlag != cICL.OFFLINE_FLAG_DELETED_RECORD) {
> +                    // Only set the modified flag if the item doesn't already
> +                    // have an offline flag
> +                    //self.setItemOfflineFlag(aItem, cICL.OFFLINE_FLAG_MODIFIED_RECORD, notifyListener);

Please remove the commented out function call.

@@ +562,5 @@
> +                    // Only set the modified flag if the item doesn't already
> +                    // have an offline flag
> +                    //self.setItemOfflineFlag(aItem, cICL.OFFLINE_FLAG_MODIFIED_RECORD, notifyListener);
> +                    let setItem = this.setItemOfflineFlag(aItem, cICL.OFFLINE_FLAG_MODIFIED_RECORD);
> +                    setItem.then(function onFulfill() {

Not mandatory, but if you prefer you can just do:

this.setItemOfflineFlag(aItem, ...).then(function onFulfill() { 
  ...
}, function onReject() {
  ...
});

@@ +942,5 @@
> +    getCachedItem: function(aId) {
> +        return this.mItemCache[aId];
> +    },
> +
> +    runStatement: function runStatement(stmt,errorMsg,synchronous) {

I think the synchronous parameter is no longer needed?

@@ +966,5 @@
> +            },
> +
> +            isComplete: function isComplete() {
> +                return this.complete;
> +            },

you might want to make this a getter, i.e

    ...
    get completionPromise() this.completion.promise,
    get isComplete() this.complete,

@@ +1078,5 @@
> +                let stmtObserver = {
> +                    handleResult: function(aResultSet) {
> +                        if ("handleResult" in observer) {
> +                            observer.handleResult.call(self, aResultSet);
> +                        } else if ("handleRow" in observer) {

Are the handleResult/handleRow callbacks still used? I have the feeling this was replaced by the promise runner and that this code either needs updating or removal.

NOTE: I saw later on they are still used, but please see my later comment about all the different run*Statement functions.

@@ +1304,3 @@
>  
> +        let insertItem = this.runItemStatement(statements, item.id,"Error inserting item");
> +        insertItem.completionPromise().then(function onFulfill() {

You know what might be pretty nice? Make the runner provide the same interface as the promise does, so you can do the following. I'm fine with a followup bug if you prefer:


this.runItemStatement(statements, ...).then(function onFulfill() { 
  ...
}, function onReject() {
  ...
});

@@ +1304,5 @@
>  
> +        let insertItem = this.runItemStatement(statements, item.id,"Error inserting item");
> +        insertItem.completionPromise().then(function onFulfill() {
> +            self.cacheItem(item);
> +            completionPromise.resolve();

If you don't go with the above suggestion, I would suggest to at least name the local variable completionPromise differently (deferred?). On first sight it confused me that there was a both a function and a variable named completionPromise.

@@ +1332,5 @@
> +        }, function onReject(aReason) {
> +            deferred.reject(aReason);
> +            cal.ERROR("Failed to deleteItemById: " + aReason);
> +        });
> +        return deferred.promise;

I think instead of creating an extra promise that is resolved/rejected here, you can just return the promise returned by promise.completionPromise.then(). Example (also shows the coolness of arrow functions):


return promise.completionPromise().then(() => {
  this.uncacheItem(aId);
}, () => {
  cal.ERROR(...);
});

@@ +1369,2 @@
>  
>          return value;

I think the last parameter determines if the call is synchronous? Its a little confusing that the function is called runAsyncItemStatement. Maybe it should just be called runItemStatement and let the parameter decide if its synchronous or not, or add another helper function runSyncItemStatement that is always synchronous.

I'm also confused there are so many different functions with run, async, item and statement in the name. Could you up those functions so that only those still used are still in the code and redundant functions are consolidated?

@@ +1418,5 @@
>          }
>  
>          if (this.mLastStatement) {
>              logMessage += "\nLast DB Statement: " + this.mLastStatement;
> +            const mISS = Components.interfaces.mozIStorageStatement;

if mISS is used more often, consider putting it at the top of the file.

::: calendar/providers/storage/calStorageUpgrade.jsm
@@ +488,4 @@
>          try {
> +            beginTransaction(db);
> +                
> +            if (updateZones) {

Some whitespaces snuck in here

@@ +578,5 @@
> +    }
> +}
> +
> +// TODO Afterwards, we have to migrate the moz-profile-calendars to the
> +// new moz-storage-calendar schema. This is needed due to 

Hmm looks like there is still a TODO here. Does anything have to be done with the new async code to handle this?

::: calendar/test/unit/test_timezone.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function run_test() {
> +    var f = cal.createEvent();
> +     

Some whitespaces in this file.

Comment 24

[Reid Anderson]

Attachment: AsyncPatch

This patch (hopefully) addresses the review comments. The things that you mentioned could be split into seperate bugs I have left out of this patch and will file new bugs for later.

Comment 25

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8456637 [details] [diff] [review]
AsyncPatch

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

r- for now, a few more comments. To push this, we need to somehow solve the getItem stuff in the first comment below, either by reverting that change fully or fixing it in another bug before we land this one.

::: calendar/providers/caldav/calDavCalendar.js
@@ +1116,5 @@
> +                    }
> +                },
> +                onOperationComplete: function deleteLocalItem_getItem_onOperationComplete() {}
> +            };
> +            yield this.mOfflineStorage.getItem(this.mHrefIndex[path],getItemListener);

Are you going to be fixing this part in the other bugs you wanted to file? If so, I think we should land that first then rebase this patch. If you have questions about this part I'm happy to schedule another call.

::: calendar/providers/storage/calStorageCalendar.js
@@ +1229,3 @@
>  
> +        let insertItem = this.runItemStatement(statements, item.id,"Error inserting item");
> +        insertItem.completionPromise.then(function onFulfill() {

Just like you did in other places, return insertItem.completionPromise.then(...); and remove all traces of deferred.

@@ +1245,5 @@
>       * @param aID           The id of the item to delete.
>       * @param aIsModify     If true, then leave in metadata for the item
>       */
> +    deleteItemById: function cSC_deleteItemById(aId) {
> +        //let deferred = Promise.defer();

Remove the commented line

::: calendar/test/unit/test_providers.js
@@ +4,5 @@
>  
>  var icalStringArray = [
>                  // Comments refer to the range defined in testGetItems().
>                  // 1: one-hour event
> +                "BEGIN:VEVENT\n" +

Just changing a few whitespaces in this file? I think we should leave that to a bug that actually makes non-whitespace changes.

::: calendar/test/unit/test_timezone.js
@@ +40,5 @@
> +          "LOCATION:1CP Conference Room 4350",
> +          "END:VEVENT",
> +          "END:VCALENDAR",].join("\r\n");
> +          
> +    var strTz =

A few whitespaces at end of line left in this file.

Comment 26

[Reid Anderson]

Attachment: Patchv3

Preparing the CalDAV provider for the switch to an asynchronous storage calendar was done in Bug 1042125, which hopefully clears the way for this. Also referenced in the comments on that bug, Task.async() has been added where appropriate here.

Comment 27

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8461436 [details] [diff] [review]
Patchv3

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

This got a little longer than I anticipated, sorry for all the comments. Most of them are just code style things, but I noticed a few lines that are bound to produce an error. I'd appreciate if you could fix these comments and upload a new version before I go into testing the code again.

There are a few general things you should watch out for to adhere to the calendar code style. I know this is pedantic and annoying, but if we don't change it now it will stay that way until those lines are changed again, no one will file a bug just to fix a few style nits. Since this is a big patch we can let them slip, but if you'd like to change it the main issues I've found violate these principles: https://wiki.mozilla.org/Calendar:Style_Guide#Keywords_and_Operators

::: calendar/providers/storage/calStorageCalendar.js
@@ +275,5 @@
> +
> +    getItem: function cSC_getItem(aId, aListener) {
> +        let self = this;
> +        let promise = self.getItemById(aId);
> +        promise.then(function onFulfill(item) {

We can get rid of self here by using arrow functions.

@@ +369,5 @@
> +                // This will be changed with bug 416975.
> +                expandedItems = [ item ];
> +            }
> +
> +            function filterItems(item) {

Minor nit: could you put this function at the top of the expandItems function? In some cases not doing this emits a warning. Also, please rename the parameter to something other than item, so it doesn't shadow the parameter of expandItems.

@@ +478,5 @@
> +    getItemOfflineFlag: function cSC_getOfflineJournalFlag(aItem, aListener) {
> +        let flag = null;
> +        let self = this;
> +        if (aItem) {
> +            Task.spawn(function() {

Missing the * to make this a generator function.

Also, to make this code a little more readable, I suggest pulling in the if(aItem) part into the task. I have another comment related to this function further below, so be sure to read that before you make changes.

@@ +488,5 @@
> +                    }
> +                }
> +            }).then(function onFulfill() {
> +                    aListener.onOperationComplete(self, Components.results.NS_OK,
> +                                                  cIOL.GET, aItem.id, flag);

Any reason you are calling aListener directly here while using notifyOperationComplete in other places?

@@ +497,5 @@
> +        } else {
> +            // It is possible that aItem can be null, flag provided should be
> +            // null in this case.
> +            aListener.onOperationComplete(this, Components.results.NS_OK,
> +                                          cIOL.GET, aItem.id, flag);

If aItem is null, then getting the id will fail.

@@ +510,5 @@
> +
> +    // calIOfflineStorage interface
> +    addOfflineItem: function(aItem, aListener) {
> +        this.setItemOfflineFlag(aItem, cICL.OFFLINE_FLAG_CREATED_RECORD).then(function onFulfill() {
> +            self.notifyOperationComplete(aListener, Components.results.NS_OK,

self will be undefined here, use arrow functions and |this| instead.

@@ +525,5 @@
> +            self.notifyOperationComplete(aListener, Components.results.NS_OK,
> +                                         cIOL.MODIFY, aItem.id, aItem);
> +        }
> +        this.getItemOfflineFlag(aItem, {
> +            onOperationComplete: function(c, s, o, i, oldOfflineJournalFlag) {

This is the comment I was referencing above. As this.getItemOfflineFlag is called internally, I'd suggest splitting that into two functions: one that adheres to the listener, the other a function that returns a promise. Example:


getItemOfflineFlag: function(aItem, aListener) {
  this.promiseItemOfflineFlag(aItem).then(function(aFlag) {
    aListener.onOperationComplete(..., aItem && aItem.id, aFlag);
  }, function(aReason) {
    aListener.onOperationComplete(..., aItem && aItem.id, aReason);
  });
},
promiseItemOfflineFlag: Task.async(function*(aItem) {
    let flag = null;
    if (aItem) {
       ...
    }
    return flag;
}),

Then you can make modifyOfflineItem and deleteOfflineItem a lot more readable with Task.async().

@@ +571,5 @@
> +
> +    resetItemOfflineFlag: function(aItem, aListener) {
> +        let self = this;
> +        this.setItemOfflineFlag(aItem, null, function() {
> +            self.notifyOperationComplete(aListener, Components.results.NS_OK,

Using an arrow function will allow you to use |this| instead. Also, doesn't setItemOfflineFlag return a promise? You want:

this.setItemOfflineFlag(aItem, null).then(() => {
  this.notifyOperationComplete(...);
}, (aError) => {
  this.notifyOperationComplete(..., aError);
});

@@ +1028,3 @@
>          }
>  
> +        for each (let stmt in statements) {

for (let stmt of statements), here and in some other places around here.

@@ +1033,5 @@
> +        return this.runStatement(stmt, errorMsg, synchronous);
> +    },
> +
> +    runItemStatement: function(stmt, itemId, errorMsg, synchronous) {
> +        //return this.runStatement(stmt, errorMsg, synchronous);

Remove commented out code.

@@ +1046,5 @@
> +            stmt.params.cal_id = this.id;
> +            stmt.params.item_id = itemId;
> +        }
> +        let runner = this.runStatement(statements, errorMsg, synchronous);
> +        return runner;

Go ahead and return this.runStatement() directly.

@@ +1130,5 @@
> +    getItemById: Task.async(function*(aId) {
> +        let item;
> +        let queryCalls = 1;
> +            let promise = yield this.assureRecurringItemCaches();
> +            item = this.getCachedItem(aId);

Check indentation here. Also, queryCalls is no longer used and since we are not using |promise| anywhere, you don't have to assign the yield result to anything.

@@ +1135,4 @@
>              if (item) {
>                  return item;
> +            } else {
> +                let runner = yield this.runItemStatement(this.mSelectEvent,aId,"Error selecting item by id " + aId);

Shouldn't this be failing? runItemStatement doesn't return a promise, it returns our runner directly.

@@ +1141,5 @@
> +                    let aResultSet;
> +                    while(aResultSet = yield runner.promiseResult()) {
> +                        for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
> +                            item = cal.createEvent(row.getResultByName("icalString"));
> +                            item.calendar = this.superCalendar;

Hmm I've seen this a few times, so maybe it makes sense to create a helper. The patch has code that wants a single item from the select statement. To do so we create the runner, loop until its complete, loop for each resultset, loop for reach result, just to get one item.

I would imagine a helper function so we can do this:

let runner = this.runItemStatement(this.mSelectEvent, aId, ...);
let row = yield runner.promiseOneRow();
let item = cal.createEvent(row.getResultByName("icalString"));
item.calendar = this.superCalendar;

The helper returns a promise that runs all of the code we have here, resolving with the first found row.

@@ +1214,4 @@
>  
> +        let insertItem = this.runItemStatement(statements, item.id,"Error inserting item");
> +        return insertItem.completionPromise.then(function onFulfill() {
> +            self.cacheItem(item);

Arrow function and self vs this please.

Comment 28

[Reid Anderson]

Attachment: Patchv4

Comment 29

[Reid Anderson]

Attachment: Patchv4.1

Fixed a self vs this

Comment 30

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8467893 [details] [diff] [review]
Patchv4.1

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

I just ran the tests with this patch applied and came across a few errors in test_providers.js. First of all, one of the addItem() calls is missing a do_test_pending and do_test_finished, then there is an error with onGetItem being called twice with the same item. My first thought was an error in itemQueue, but it seems fine. I'd appreciate if you could look into this.

Unfortunately there are still a few comments that would cause a hard error, so r- for now.

::: calendar/providers/storage/calStorageCalendar.js
@@ +389,5 @@
> +                let aResultSet;
> +                while (aResultSet = yield query.promiseResult()) {
> +                    for (let row = aResultSet.getNextRow(); row; row = aResultSet.getNextRow()) {
> +                        let rowItem = action(row.getResultByName("icalString"));
> +                        rowItem.calendar = self.superCalendar;

If you bind to this a few lines further, then you shouldn't need to use self. Or did you experience issues with using this?

@@ +494,5 @@
> +
> +    setItemOfflineFlag: Task.async(function*(aItem, flag) {
> +        stmt = this.mEditItemOfflineFlag;
> +        stmt.params.offline_journal = flag || null;
> +        this.runItemStatement(stmt,aItem.id,"Error setting offline flag for " + aItem.id);

I believe you will need to return the completion promise here, or maybe the runner. Depends on whats better for the callers.

@@ +510,5 @@
> +    },
> +
> +    modifyOfflineItem: Task.async(function*(aItem,aListener) {
> +        function notifyListener() {
> +            this.notifyOperationComplete(aListener, Components.results.NS_OK,

You can't use this inside notifyListener because it has its own this contex, i.e. is not bound or uses an arrow function. In this case, since notifyListener is only used once in the else block, you should just call this.notifyOperationComplete directly in the else block.

@@ +545,5 @@
> +            // If the item was created offline, we can just delete it again
> +            this.deleteItemById(aItem.id).then((item) => {
> +                notifyListener();
> +            },(aReason) => {
> +                cal.ERROR("Failed to get an items offline flag: " + aReason);

You also need to notify the listener with an error in case this fails

@@ +549,5 @@
> +                cal.ERROR("Failed to get an items offline flag: " + aReason);
> +            });
> +        } else {
> +            // Otherwise we need to mark it deleted
> +            this.setItemOfflineFlag(aItem, cICL.OFFLINE_FLAG_DELETED_RECORD, notifyListener);

setItemOfflineFlag no longer takes a third parameter, you probably need to yield its promise instead.

@@ +1128,5 @@
> +            return item;
> +        } else {
> +            let row;
> +            let runner = this.runItemStatement(this.mSelectEvent,aId,"Error selecting item by id " + aId);
> +            if(row = yield runner.promiseOneRow()) {

This will cause a strict warning about a possible mistaken assignment. Go ahead and use:

let runner = this.runItemStatement();
let row = yield runner.promiseOneRow();
if (row) {
   ...
}

Also, no need to select the todo if an event was already retrieved. Saves a few database calls.

Comment 31

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8467893 [details] [diff] [review]
Patchv4.1

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

Two minor things I found during testing:

::: calendar/providers/storage/calStorageCalendar.js
@@ +366,5 @@
> +                expandedItems = [ item ];
> +            }
> +
> +            function filterItems(item) {
> +                let att = superCal.getInvitedAttendee(item);

superCal is not defined in case its not a scheduling calendar, so use:

let att = superCal && superCal.getInvitedAttendee(...)

@@ +484,5 @@
> +    promiseItemOfflineFlag: Task.async(function*(aItem) {
> +        let flag = null;
> +        if (aItem) {
> +            let selectItemBase = this.runItemStatement(this.mSelectItemBase,aItem.id,"Error getting offline flag");
> +            if(row = yield selectItemBase.promiseOneRow()) {

row is not defined with let. Another place where there may be a strict warning due to possible mistaken assignment instead of comparison. Better yield outside of the if statement, go ahead and check all changes for this type of problem.

Comment 32

[Reid Anderson]

Attachment: Patchv5

This fixes the recurrence issues that were calling the unit test to fail, and should also address the other review comments.

Comment 33

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8470780 [details] [diff] [review]
Patchv5

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

I will upload a new patch shortly with a few nits fixed to speed things up. Here is a list of things I will change:

* Spaces after comma for arguments and function calls
* Use default args instead of setting them manually
* Used arrow functions to get rid of self
* Replaced use of then() inside a Task.async with a call to yield.
* Added an extra set of () when making assignments in while() to silence strict warnings.
* Switched a Task to a simple promise where it was overkill (promiseOneRow)

I think we finally have a patch ready to be pushed (!)

::: calendar/providers/caldav/calDavRequestHandlers.js
@@ +121,5 @@
>                      if ((wasInboxItem && this.calendar.isInbox(this.baseUri.spec)) ||
>                          (wasInboxItem === false && !this.calendar.isInbox(this.baseUri.spec))) {
>                          cal.LOG("Deleting local href: " + path)
>                          delete this.calendar.mHrefIndex[path];
> +                        yield pcal.deleteItem(foundItem,null);

No need for the null arguments with pcal. Also, do you think you could push the caldav changes into a different bug? It may be easier to find regressions this way. I'm going to take these bits out of the patch I am uploading so don't be surprised if you can't find them.

::: calendar/test/unit/test_providers.js
@@ +272,5 @@
>                       Ci.calICalendar.ITEM_FILTER_COMPLETED_ALL;
>  
>          // implement listener
> +        let count = 0;
> +        let tempCount = 0;

I think there are no changes aside from a few unneeded variables, whitespace changes and var/let in this file? I think we should postpone these changes until we make more substantial changes to this file. I've reverted this in the patch I am uploading, if I missed something let me know.

Comment 34

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v6

Comment 35

[Philipp Kewisch [:Fallen][📱📆]]

v6 is ready for checkin and should be pushed after or together with bug 1054679.

Comment 36

[Philipp Kewisch [:Fallen][📱📆]]

Also, please make sure there is at least one green nightly for all platforms before pushing this patch.

Comment 37

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v7

Sorry, missing qref.

Comment 38

[Philipp Kewisch [:Fallen][📱📆]]

Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/538a83acca21>

Comment 39

[Philipp Kewisch [:Fallen][📱📆]]

The push caused some test failures, I suspect the same issues I had when testing the metadata patch. I've pushed the fix from that bug as a bustage fix:

https://hg.mozilla.org/comm-central/rev/bf09861f8002

Comment 40

[Philipp Kewisch [:Fallen][📱📆]]

Unfortunately I have to back this out, there are still remaining test failures. We should run this through the tryserver a few times and fix them.

https://hg.mozilla.org/comm-central/rev/b3e999a72882

Comment 41

[Martin Schröder [:mschroeder]]

Philipp, the patch got backed out but the bug never re-opened. Was this on purpose?

Comment 42

[Philipp Kewisch [:Fallen][📱📆]]

No, an oversight. Thanks for catching this. I still owe Reid an answer on emails. Finding the test failure isn't that easy as it can't be reliably reproduced.

Comment 43

[Wayne Mery (:wsmwk)]

Phillipp, Does this really still depend on bug 512761?

Comment 44

[Philipp Kewisch [:Fallen][📱📆]]

Not a strong dependency, but it would make the code slightly nicer.

Comment 45

[Philipp Kewisch [:Fallen][📱📆]]

I'm sending another try-run to get the exact error message. I've also made some slight adjustments, mostly to fix core changes since the patch (e.g. we now need to use PromiseUtils.defer()).

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=eaa6fb7a294e

I also went through the code again to see if there were any situations that might cause timing issues, but I couldn't find anything obvious.

Comment 46

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: Fix - v7 - debitrotted

Comment 47

[Philipp Kewisch [:Fallen][📱📆]]

Attachment: WiP - test fixes

I've managed to figure out some of the problems. Note that the try run contains a few bogus failures that happen because I had applied the wrong patches. I've emailed Reid some info on why the tests are failing, luckily they now fail with more of a bang than last time and we can figure it out.

This patch still needs some work, specifically figuring out why deleteCalendar doesn't work, making test_calmgr, test_alarmservice and test_deleted_items async safe, and ensuring that deleteCalendar is called with a listener everywhere now. Possibly the same with other methods that are now async.

Comment 48

[Reid Anderson]

Attachment: AsyncStorage.diff

Comment 49

[Reid Anderson]

Attachment: AsyncStorageExtras.diff

The storage system is now passing all tests on my machine.

Comment 50

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8577696 [details] [diff] [review]
AsyncStorageExtras.diff

I've folded the two patches and did a try run:

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e8c060c5ae85

Looks like AsyncStorage.diff is still the same as v7 debitrotted, which is great because now I can just review the extras.

Comment 51

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8577695 [details] [diff] [review]
AsyncStorage.diff

...and we can use the folded version at the end for upload.

Comment 52

[alexander.haensch]

Great news. We have slowdowns with the calendars many times.
Is a patched version already in a downloadable version available. I would be happy to test.

Comment 53

[alexander.haensch]

Ok found it here: 
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/[e-mail]-e8c060c5ae85/

Comment 54

[Philipp Kewisch [:Fallen][📱📆]]

Reid, I'm still seeing a failure all debug that is not expected:

TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:calendar/test/unit/test_bug494140.js | xpcshell return code: -11

Could you have a look?


Alexander, please let us know how the test builds work for you an if you encounter any issues.

Comment 55

[alexander.haensch]

I tested the try build. Unfortunatly it changed nothing to the slowdown.
Is it possible to change the Pragma synchronous in the sqlite files? I have the feeling that the slowdowns are related to a wait on the storage side.

Comment 56

[Philipp Kewisch [:Fallen][📱📆]]

We are already using WAL mode, so I don't think fiddling with PRAGMA synchronous will fix anything. While setting PRAGMA synchronous off might speed up a few operations, its also more likely that powerloss will corrupt the database.

Comment 57

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 8577696 [details] [diff] [review]
AsyncStorageExtras.diff

r- because we need to fix the debug builds bustage. Reid, any updates?

Comment 58

[Philipp Kewisch [:Fallen][📱📆]]

This won't make it for 4.0, but I do hope we can get this in soon!

Comment 59

[Wayne Mery (:wsmwk)]

FYI Reid wrote me a few weeks ago "I hope to get back and finish it this summer, but can't promise that."

Comment 60

[[e-mail]]

Attachment: Quick and dirty test

This is just some proof of concept code I whipped up. In particular, there are almost no comments, and it only does the bare minimum necessary to execute the delete single item and insert single item as transactions on the background thread (as opposed to executing each statement individually on the background thread, which may not be what you want).

Rather annoyingly, insert transactions are difficult to execute on the background thread. First, you need to create a binding params array for each of the statements that you want to execute and create binding params for each row you want to insert. You need to add the params to the array after binding the values (which apparently requires the explicit binding API, but maybe I missed something), and then only add the array to the statement after all of the rows have been bound. This is tricky when exceptions get involved as they're recursive calls! Once you've added each array to its statement you can then execute the whole transaction asynchronously.

I compared this locally against main thread inserts and not only of course does this mostly free up the main thread but it was also about twice as fast (I didn't make accurate timings).

Comment 61

[[e-mail]]

Attachment: Make item writes async

I've been testing on my PC against a calendar of 4800 items. In Thunderbird 60, this takes approximately 192 seconds to complete its initial sync. During this time, the Thunderbird UI is usually unresponsive; there's a 4-second cycle whereby it flips between completely unresponsive and merely sluggish.

In Thunderbird 60, this has at least improved and now only takes approximately 144 seconds, which is a useful speed-up. However, the UI is still unresponsive during this time.

With this patch, the wall clock time to complete the sync stays about the same, but now the UI is pretty responsive all of the time.

Changes since my draft patch:

• Switched to using async statements where relevant (the statement to delete metadata can't be made async as still needs to be usable on the main thread)
• Although I had removed the try/catch blocks relating to transactions that were no longer necessary now that executeAsync automagically executes in a transaction the helper methods were also no longer necessary
• Removed one other unnecessary line of code that I had previously overlooked

Tests performed:

• Importing 4800 items
• Manually creating a recurring appointment with an attachment
• Updating a recurring master
• Updating a recurrence
• Deleting a recurrence
• Deleting a recurring master

Comment 62

[[e-mail]]

Oh, and I've also pushed this patch to Try.

Comment 63

[[e-mail]]

Ah, I've discovered that setDateParamHelper is used in more places. Of course once Geoff finishes async reads then that won't be a problem, but guess I need to come up with interim workaround...

Comment 64

[[e-mail]]

Attachment: Make item writes async

Fix turned out to be easy enough, since the interface setDateParamHelper was looking for is already implemented on the statement itself anyway.

Comment 65

[[e-mail]]

Comment on attachment 9082252 [details] [diff] [review]
Make item writes async

Actually something's not quite right with exceptions for some reason...

Comment 66

[[e-mail]]

Attachment: Make item writes async

Now with workaround for Toolkit bug 1570672 - only the first set of parameters on an async statement defaults to null, so we have to explicitly set all "unused" parameters to null.

Comment 67

[Geoff Lankow (:darktrojan)]

Attachment: 501689-async-storage-calendar-1.diff

Neil, did you not get my email saying that I was 99% finished? ;-)

Comment 68

[Geoff Lankow (:darktrojan)]

I do like your approach though to writing items though.

Comment 69

[[e-mail]]

Comment on attachment 9082463 [details] [diff] [review]
501689-async-storage-calendar-1.diff

>-    deleteItemById: function(aID, aIsModify) {
>+    deleteItemById: async function(aID, aIsModify) {
>         this.acquireTransaction();
>         try {
>+            await this.executeItemStatement(this.mDeleteAttendees, "item_id", aID);
Does that transaction actually affect those async statements? It's unclear to me from the documentation, which is why I went with the approach of executing all of the statements in one call to `executeAsync`.

Comment 70

[[e-mail]]

When I say unclear, I'm looking at these passages from MDN:

The database engine does not support nested transactions, so attempting to start a transaction when one is already active will throw an exception.

... later regarding executeAsync...

The statements are run wrapped in a transaction.

I'm taking this as meaning that each invocation of executeAsync executes in an independent transaction, which means the only way to execute multiple statements on the background thread in a single transaction is to pass them all at once to executeAsync.

(In reply to Geoff Lankow from comment #67)

Neil, did you not get my email saying that I was 99% finished? ;-)

I didn't even get your bugmail saying you had started!

Comment 71

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9082277 [details] [diff] [review]
Make item writes async

Given Geoff has also spent considerable effort in this area I think it is best he reviews the patch. I've skimmed through the patch and it seems reasonable, some testing would be great and if possible some tests.

You might also want to check the interface, I believe addItem/adoptItem returns a calIOperation, if you make the function async it is returning a Promise instead.

Mid/Long term we need to get rid of idl usage in JS anyway, so I'm not set on keeping calICalendar. We'll need an intermediate solution though, which could be returning a calIOperation that signals completion when the promise resolves.

Also keep in mind that the cached calendar had in the past made bad assumptions on how storage works, it would assume the operations complete synchronously. I'm not sure this is still the case, and it may not apply to this patch.

Comment 72

[Geoff Lankow (:darktrojan)]

(In reply to [e-mail] from comment #69)

Does that transaction actually affect those async statements? It's unclear
to me from the documentation, which is why I went with the approach of
executing all of the statements in one call to executeAsync.

I'm glad you asked, I wasn't sure myself, but yes, transactions appear to work as expected for both stmt.executeAsync and db.executeAsync. It probably isn't very useful in the latter.

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #71)

You might also want to check the interface, I believe addItem/adoptItem
returns a calIOperation, if you make the function async it is returning a
Promise instead.

Indeed, many of these functions should be returning calIOperation, but they're not. We should fix that.

I think the best way forward here is for me to get Neil's patch then adapt mine to go on top of it. Trying that now.

Comment 73

[Geoff Lankow (:darktrojan)]

Comment on attachment 9082277 [details] [diff] [review]
Make item writes async

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

Okay, this looks good.

::: calendar/providers/storage/calStorageCalendar.js
@@ +419,5 @@
> +    },
> +
> +    prepareItemStatement: function(aStmts, aStmt, aIdParam, aId) {
> +        aStmt.params.cal_id = this.id;
> +        aStmt.params[aIdParam] = aId;

Surely this should be .bindByName, .params isn't going to work on these statements.

@@ +425,5 @@
> +    },
> +
> +    executeAsync: function(aStmts) {
> +        return new Promise((resolve, reject) => {
> +            this.mDB.executeAsync(aStmts, aStmts.length, {

The length argument has now been removed.

@@ +2021,4 @@
>          cal.ASSERT(!item.recurrenceId, "no parent item passed!", true);
>  
> +        await this.deleteItemById(olditem ? olditem.id : item.id, true);
> +        /// Map {mozIStorageStatement -> mozIStorageBindingParamsArray}

Linting error: two slashes, not three.

@@ +2039,1 @@
>      // to writeEvent/writeTodo to actually do the writing.

This comment isn't strictly accurate any more.

Comment 74

[[e-mail]]

(In reply to Geoff Lankow from comment #73)

::: calendar/providers/storage/calStorageCalendar.js
@@ +419,5 @@
> +    },
> +
> +    prepareItemStatement: function(aStmts, aStmt, aIdParam, aId) {
> +        aStmt.params.cal_id = this.id;
> +        aStmt.params[aIdParam] = aId;

Surely this should be .bindByName, .params isn't going to work on these statements.

It's fine if you know you're only going to execute the statement with one set of parameters.

Comment 75

[[e-mail]]

Attachment: Make item writes async

I updated it as you asked but I guess the Try build doesn't include calendar or something, so I haven't actually tested it on trunk.

Comment 76

[Geoff Lankow (:darktrojan)]

(In reply to [e-mail] from comment #74)

(In reply to Geoff Lankow from comment #73)

::: calendar/providers/storage/calStorageCalendar.js
@@ +419,5 @@
> +    },
> +
> +    prepareItemStatement: function(aStmts, aStmt, aIdParam, aId) {
> +        aStmt.params.cal_id = this.id;
> +        aStmt.params[aIdParam] = aId;

Surely this should be .bindByName, .params isn't going to work on these statements.

It's fine if you know you're only going to execute the statement with one set of parameters.

You've made the statements into mozIStorageAsyncStatement, which doesn't have the clever javascript params helper that can handle binding values like this.

Comment 77

[[e-mail]]

(In reply to Geoff Lankow from comment #76)

You've made the statements into mozIStorageAsyncStatement, which doesn't have the clever javascript params helper that can handle binding values like this.

https://searchfox.org/mozilla-esr60/search?q=AsyncStatementParams%3A%3ANamedSetter

Comment 78

[Geoff Lankow (:darktrojan)]

Ohhh I see why it didn't work when I tested it. I tested it wrong. :-/

Comment 79

[Geoff Lankow (:darktrojan)]

Attachment: 501689-item-writes-1.diff

Here's Neil's patch again with the indentation and use of Components.Exception fixed.

Comment 80

[Geoff Lankow (:darktrojan)]

Attachment: 501689-async-storage-calendar-2.diff

… and here's mine again to apply on top of Neil's.

Comment 81

[[e-mail]]

(In reply to Geoff Lankow from comment #79)

use of Components.Exception fixed.

Thanks for finding that.

Comment 82

[[e-mail]]

Comment on attachment 9085001 [details] [diff] [review]
501689-async-storage-calendar-2.diff

>-            destCal.addItem(item, listener);
>+            await new Promise(resolve => {
>+                resolvePromise = resolve;
>+                destCal.addItem(item, listener);
>+            });
IMHO this looks ugly but I can't think of anything better right now.

>-            let storedOldItem = (aOldItem ? this.getItemById(aOldItem.id) : null);
>+            let storedOldItem = null;
>+            if (aOldItem) {
>+                storedOldItem = await this.getItemById(aOldItem.id);
>+            }
This is just to avoid `await` in the middle of an expression? (Happens in other places too.)

>-                this.prepareStatement(this.mSelectNonRecurringEventsByRange);
>-                params = this.mSelectNonRecurringEventsByRange.params;
>-                params.range_start = startTime;
>-                params.range_end = endTime;
>-                params.start_offset = aRangeStart ? aRangeStart.timezoneOffset * USECS_PER_SECOND : 0;
>-                params.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * USECS_PER_SECOND : 0;
>-                params.offline_journal = requestedOfflineJournal;
>+                let stmt = this.mSelectNonRecurringEventsByRange;
>+                this.prepareStatement(stmt);
>+                stmt.params.range_start = startTime;
>+                stmt.params.range_end = endTime;
>+                stmt.params.start_offset = aRangeStart ? aRangeStart.timezoneOffset * USECS_PER_SECOND : 0;
>+                stmt.params.end_offset = aRangeEnd ? aRangeEnd.timezoneOffset * USECS_PER_SECOND : 0;
>+                stmt.params.offline_journal = requestedOfflineJournal;
Caused by your temporary switch away from `params`? Might be neater to keep `params` instead of `stmt`. (Would apply to other places also.)

>     assureRecurringItemCaches: function() {
>+        if (this.mRecItemCachePromise) {
>+            return this.mRecItemCachePromise;
>         }
>         // build up recurring event and todo cache with its offline flags,
>         // because we need that on every query: for recurring items, we need to
>         // query database-wide.. yuck
>+        this.mRecItemCachePromise = new Promise(async (resolve) => {
Maybe a separate async function would make this clearer i.e.

    assureRecurringItemCaches: function() {
        if (!this.mRecItemCachePromise) {
            this.mRecItemCachePromise = this.populateRecurringItemCaches();
        }
        return this.mRecItemCachePromise;
    },
    populateRecurringItemCaches: async function() {

>+            this.prepareStatement(this.mSelectAllAttendees);
(I don't know what all of this new code is doing, but I'm guessing that you're fetching all of the tables separately and then combining the results at the end, whereas the old code would have simply fetched each item separately from its constituent tables. In particular I'm not sure how this handles additional data in exceptions.)

Comment 83

[Geoff Lankow (:darktrojan)]

Attachment: 501689-async-storage-calendar-3.diff

Thanks Neil, I've made the changes you suggested.

Comment 84

[Geoff Lankow (:darktrojan)]

(In reply to [e-mail] from comment #82)

This is just to avoid await in the middle of an expression? (Happens in
other places too.)

More or less. I'm not the biggest fan of unary expressions anyway and the await just makes it more confusing. That said, I'm not really bothered either way, it's just what I did at the time.

(I don't know what all of this new code is doing, but I'm guessing that
you're fetching all of the tables separately and then combining the results
at the end, whereas the old code would have simply fetched each item
separately from its constituent tables. In particular I'm not sure how this
handles additional data in exceptions.)

Yes, that's what it does. It should be more efficient to do one query for each table that one for each event for each table. Not only that but because the queries are now async I found all of the sub-queries were starting immediately as the main query looped and that's terrible performance-wise.

Comment 85

[[e-mail]]

Comment on attachment 9086877 [details] [diff] [review]
501689-async-storage-calendar-3.diff

>     assureRecurringItemCaches: function() {
>+        if (this.mRecItemCachePromise) {
>+            this.mRecItemCachePromise = this._assureRecurringItemCaches();
>         }
>+        return this.mRecItemCachePromise;
>+    },
Needs to be `if (!this.mRecItemCachePromise)`.

Comment 86

[Geoff Lankow (:darktrojan)]

Oops! Yes it does. I've fixed it locally, not going to upload again just for one character. Let's see if Philipp reads the comments before reviewing, or doesn't notice the mistake. ;-)

Comment 87

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9086877 [details] [diff] [review]
501689-async-storage-calendar-3.diff

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

Thanks for the extra eyes and work on this, I'm really excited about it!

::: calendar/base/content/import-export.js
@@ +185,5 @@
>          try {
> +            await new Promise(resolve => {
> +                resolvePromise = resolve;
> +                destCal.addItem(item, listener);
> +            });

Here is your way to get rid of resolvePromise :)

```
let pcal = cal.async.promisifyCalendar(destCal);

try {
  await pcal.addItem(item);
  // do other stuff from listener and catch block as necessary
} catch (e) {
  lastError = e;
  ...
}
```

::: calendar/providers/storage/calStorageCalendar.js
@@ +85,5 @@
>              }
>          }
>  
> +        this.prepareStatement(this.mDeleteAllEvents);
> +        stmts.push(this.mDeleteAllEvents);

You could just have `prepareStatement` return the statement and put thes into one line.

@@ +273,5 @@
> +                },
> +                async handleResultInner(aResultSet) {
> +                    let row = aResultSet.getNextRow();
> +                    while (row) {
> +                        await aCallback(row);

Do we need to wait for the callback to complete to pump out the next row or can we Promise.all() this?

Also, what happens if the callback rejects, are we prepared to handle that?

@@ +1459,3 @@
>      assureRecurringItemCaches: function() {
> +        if (this.mRecItemCachePromise) {
> +            this.mRecItemCachePromise = this._assureRecurringItemCaches();

Nope, I wouldn't have noticed during the review. Good catch Neil!

@@ +1469,5 @@
> +        let events = [];
> +        let itemsMap = new Map();
> +        this.prepareStatement(this.mSelectEventsWithRecurrence);
> +        await this.executeAsync(this.mSelectEventsWithRecurrence, async (row) => {
> +            events.push(row);

Would it make sense to have a different mode where all events are just returned as an array? This would avoid callbacks that just add to an array. Might not be a great perf issue though.

@@ +1493,5 @@
>  
> +        this.prepareStatement(this.mSelectAllAttendees);
> +        await this.executeAsync(this.mSelectAllAttendees, (row) => {
> +            let item = itemsMap.get(row.getResultByName("item_id"));
> +            if (!item || !item.isMutable) {

In which cases would we have immutable items here?

@@ +1506,5 @@
> +                    item.addAttendee(attendee);
> +                }
> +            } else {
> +                cal.WARN("[calStorageCalendar] Skipping invalid attendee for item '" +
> +                         item.title + "' (" + item.id + ").");

If you want to change this to a template string I wouldn't mind :)

::: calendar/test/unit/test_bug494140.js
@@ +51,5 @@
> +            onOperationComplete: resolve,
> +        });
> +    });
> +    let retrievedItem = await new Promise(resolve => {
> +        storageCal.getItem("c1a6cfe7-7fbb-4bfb-a00d-861e07c649a5", {

This could probably be simplified with cal.async.promisifyCalendar

::: calendar/test/unit/test_providers.js
@@ +275,5 @@
>          // implement listener
>          let count = 0;
> +        await new Promise(resolve => {
> +            let listener = {
> +                onOperationComplete: function(aCalendar,

Same here. Likely a few other places that could make use of it.

@@ +343,5 @@
>                      } else if (aOperationType == Ci.calIOperationListener.GET) {
>                          equal(count, 1);
>                          compareItemsSpecific(returnedItem, aItem);
>                      }
> +                    this.promises.pop()();

I wouldn't mind putting this in two lines if you still need it after using cal.async.promisifyCalendar.

Comment 88

[Geoff Lankow (:darktrojan)]

Attachment: 501689-async-storage-calendar-4.diff

This addresses the review comments and a rather fundamental error in my earlier patch. For ease of review, this patch is only the changes on top of that patch – I'll fold them together once ready.

The error was in listening for a Promise before continuing with handleCompletion. The way it was written, only the first set of results from the query would be sure to be consumed before completion. I've rewritten the function to stop relying on Promises and have handleResultInner call handleCompletionInner when it's ready.

Comment 89

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9089790 [details] [diff] [review]
501689-async-storage-calendar-4.diff

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

r- for the pcal.addItem() thing. Would be great if we could cover this by a test because I'm thinking it should have failed the way it is now.

::: calendar/base/content/import-export.js
@@ +186,4 @@
>    for (let item of aItems) {
>      // XXX prompt when finding a duplicate.
>      try {
> +      await pcal.addItem(item, listener);

promisifyCalendar makes it so that the listener argument isn't necessary. In fact, it won't actually be called. Instead, the promise wait until completion.

So you need to move what was originally in the listener in case of success to after the addItem call, and anything with `Components.isSuccessCode(aStatus)` being false into the catch block.

::: calendar/providers/storage/calStorageCalendar.js
@@ +268,1 @@
>          async handleResult(aResultSet) {

This function doesn't seem to need async any more?

@@ +284,3 @@
>            while (row) {
> +            try {
> +              await aCallback(row);

Given you didn't change this I guess we need to push out the items in order? I see how Promise.all() might not work, but we might not need to wait until callback completion to run the next one.

Comment 90

[Geoff Lankow (:darktrojan)]

You're right, I've had in mind that I wanted one batch to complete before starting the next, because at one point each row returned started a bunch of new queries, and that was terrible for performance. That doesn't happen any more but I've not stopped thinking I need to prevent it.

Comment 91

[Geoff Lankow (:darktrojan)]

Attachment: 501689-async-storage-calendar-5.diff

I'll make a test for importing in a new bug, because it's really getting out of scope for this bug.

Comment 92

[Philipp Kewisch [:Fallen][📱📆]]

Comment on attachment 9090903 [details] [diff] [review]
501689-async-storage-calendar-5.diff

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

(In reply to Philipp Kewisch [:Fallen] [:📆] from comment #89)
> Given you didn't change this I guess we need to push out the items in order?
> I see how Promise.all() might not work, but we might not need to wait until
> callback completion to run the next one.
I never got a straight answer, but given I asked twice I'm assuming you got this covered :)

r+, thank you for pushing this forward, I'm really looking forward to the perf gains!

Comment 93

[Geoff Lankow (:darktrojan)]

Okay, here's your straight answer, since I wasn't clear: no, order is not important, or there'd be ORDER BY in the SQL. Doing more than one thing at a time was the problem I was trying to solve.

Comment 94

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/be068ad28ef8
Make storage calendar item writes async. r=darktrojan
https://hg.mozilla.org/comm-central/rev/56cc567da57e
Convert calStorageCalendar to use asynchronous SQL where possible; r=Fallen

Comment 95

[Richard Leger]

Would this patch be backported into the 68.x ESR branch to bring performance to those version as well?

Comment 96

[Wayne Mery (:wsmwk)]

(In reply to Richard Leger from comment #95)

Would this patch be backported into the 68.x ESR branch to bring performance to those version as well?

We need to see first that it proves stable on nightly. And then, purely from a QA point of view, only after considerable baking on beta (a solid 3-4 weeks) and if comment 61 holds true "With this patch, the wall clock time to complete the sync stays about the same, but now the UI is pretty responsive all of the time.".

Geoff and Philipp could speak from a code POV, but I'd think it is a promising idea given that this is backend code limited to the storage area.

Comment 97

[Geoff Lankow (:darktrojan)]

I'd seriously consider it, but like Wayne says, only after a full round of testing in Daily and Beta.