The default bug view has changed. See this FAQ.

Investigate making storage asynchronous

REOPENED
Assigned to

Status

Calendar
Provider: Local Storage
--
major
REOPENED
8 years ago
a year ago

People

(Reporter: Fallen, Assigned: Reid Anderson, NeedInfo)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {perf})

unspecified
Dependency tree / graph
Bug Flags:
wanted-calendar1.0 +

Details

(Whiteboard: [no l10n impact], URL)

Attachments

(2 attachments, 14 obsolete attachments)

170.57 KB, patch
Details | Diff | Splinter Review
64.76 KB, patch
Fallen
: review-
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
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.
Flags: wanted-calendar1.0+
(Reporter)

Comment 1

8 years ago
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.
(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.
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.
(Reporter)

Comment 4

8 years ago
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?
(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.
(Reporter)

Comment 6

8 years ago
(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.
Depends on: 512761
(Reporter)

Updated

7 years ago
Blocks: 462277
(Reporter)

Updated

7 years ago
Flags: wanted-calendar1.0+ → blocking-calendar1.0+
Whiteboard: [not needed beta][no l10n impact]
Blocks: 576017
Keywords: perf
(Reporter)

Comment 7

6 years ago
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.
Flags: blocking-calendar1.0+ → wanted-calendar1.0+
Whiteboard: [not needed beta][no l10n impact] → [no l10n impact]

Comment 8

5 years ago
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.
(Reporter)

Comment 9

5 years ago
Created attachment 661751 [details] [diff] [review]
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.
Assignee: nobody → philipp
Status: NEW → ASSIGNED
(Reporter)

Comment 10

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

Comment 11

5 years ago
@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.
(Reporter)

Comment 12

5 years ago
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.
No longer blocks: 576017
(Reporter)

Updated

3 years ago
Priority: -- → P2
(Reporter)

Updated

3 years ago
Priority: P2 → --
(Reporter)

Comment 13

3 years ago
Just a note, bug 411894 has just added executeSimpleSQLAsync, so simple statements can now also be done asynchronously.
(Reporter)

Comment 14

3 years ago
Also, check out Sqlite.jsm. Not sure its worth porting, but something to keep in mind.
(Assignee)

Comment 15

3 years ago
Created attachment 8427112 [details] [diff] [review]
WiP -v1 with promises
Attachment #8427112 - Flags: review?(philipp)
(Reporter)

Comment 16

3 years ago
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
  });
};
Attachment #8427112 - Flags: review?(philipp) → feedback-
(Reporter)

Comment 17

3 years ago
> > +    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.
(Assignee)

Comment 18

3 years ago
Created attachment 8431443 [details] [diff] [review]
Updated Promises patch
Attachment #8427112 - Attachment is obsolete: true
Attachment #8431443 - Flags: feedback?(philipp)
(Assignee)

Comment 19

3 years ago
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.
(Reporter)

Comment 20

3 years ago
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!
Attachment #8431443 - Flags: feedback?(philipp) → feedback+
(Assignee)

Comment 21

3 years ago
Created attachment 8446618 [details] [diff] [review]
AsyncPatch.diff
Attachment #8431443 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8446618 - Flags: review?(philipp)
(Assignee)

Comment 22

3 years ago
Created attachment 8451446 [details] [diff] [review]
Patch
Attachment #8446618 - Attachment is obsolete: true
Attachment #8446618 - Flags: review?(philipp)
Attachment #8451446 - Flags: review?(philipp)
(Reporter)

Comment 23

3 years ago
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.
Attachment #8451446 - Flags: review?(philipp) → review-
(Assignee)

Comment 24

3 years ago
Created attachment 8456637 [details] [diff] [review]
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.
Assignee: philipp → anderson
Attachment #8451446 - Attachment is obsolete: true
Attachment #8456637 - Flags: review?(philipp)
(Reporter)

Comment 25

3 years ago
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.
Attachment #8456637 - Flags: review?(philipp) → review-
(Assignee)

Comment 26

3 years ago
Created attachment 8461436 [details] [diff] [review]
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.
Attachment #8456637 - Attachment is obsolete: true
Attachment #8461436 - Flags: review?(philipp)
(Reporter)

Comment 27

3 years ago
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.
Attachment #8461436 - Flags: review?(philipp) → review-
(Assignee)

Comment 28

3 years ago
Created attachment 8466078 [details] [diff] [review]
Patchv4
Attachment #8461436 - Attachment is obsolete: true
Attachment #8466078 - Flags: review?(philipp)
(Assignee)

Updated

3 years ago
Attachment #8466078 - Flags: review?(philipp)
(Assignee)

Comment 29

3 years ago
Created attachment 8467893 [details] [diff] [review]
Patchv4.1

Fixed a self vs this
Attachment #8466078 - Attachment is obsolete: true
Attachment #8467893 - Flags: review?(philipp)
(Reporter)

Comment 30

3 years ago
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.
Attachment #8467893 - Flags: review?(philipp) → review-
(Reporter)

Comment 31

3 years ago
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.
(Assignee)

Comment 32

3 years ago
Created attachment 8470780 [details] [diff] [review]
Patchv5

This fixes the recurrence issues that were calling the unit test to fail, and should also address the other review comments.
Attachment #8467893 - Attachment is obsolete: true
Attachment #8470780 - Flags: review?(philipp)
(Reporter)

Comment 33

3 years ago
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.
Attachment #8470780 - Flags: review?(philipp) → review-
(Reporter)

Comment 34

3 years ago
Created attachment 8474143 [details] [diff] [review]
Fix - v6
Attachment #661751 - Attachment is obsolete: true
Attachment #8470780 - Attachment is obsolete: true
Attachment #8474143 - Flags: review+
(Reporter)

Comment 35

3 years ago
v6 is ready for checkin and should be pushed after or together with bug 1054679.
Keywords: checkin-needed
(Reporter)

Comment 36

3 years ago
Also, please make sure there is at least one green nightly for all platforms before pushing this patch.
(Reporter)

Comment 37

3 years ago
Created attachment 8474146 [details] [diff] [review]
Fix - v7

Sorry, missing qref.
Attachment #8474143 - Attachment is obsolete: true
Attachment #8474146 - Flags: review+
(Reporter)

Comment 38

3 years ago
Pushed to comm-central <http://hg.mozilla.org/comm-central/rev/538a83acca21>
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 3.7
(Reporter)

Updated

3 years ago
Keywords: checkin-needed
(Reporter)

Comment 39

3 years ago
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
(Reporter)

Comment 40

3 years ago
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
Philipp, the patch got backed out but the bug never re-opened. Was this on purpose?
Flags: needinfo?(philipp)
(Reporter)

Comment 42

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Phillipp, Does this really still depend on bug 512761?
Severity: normal → major
(Reporter)

Comment 44

2 years ago
Not a strong dependency, but it would make the code slightly nicer.
Flags: needinfo?(philipp)
(Reporter)

Comment 45

2 years ago
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.
(Reporter)

Comment 46

2 years ago
Created attachment 8555175 [details] [diff] [review]
Fix - v7 - debitrotted
Attachment #8474146 - Attachment is obsolete: true
(Reporter)

Comment 47

2 years ago
Created attachment 8555177 [details] [diff] [review]
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.
(Reporter)

Updated

2 years ago
Target Milestone: 3.7 → 4.0
(Assignee)

Comment 48

2 years ago
Created attachment 8577695 [details] [diff] [review]
AsyncStorage.diff
Attachment #8555175 - Attachment is obsolete: true
Attachment #8555177 - Attachment is obsolete: true
Attachment #8577695 - Flags: review?(philipp)
(Assignee)

Comment 49

2 years ago
Created attachment 8577696 [details] [diff] [review]
AsyncStorageExtras.diff

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

Comment 50

2 years ago
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.
Attachment #8577696 - Flags: review?(philipp)
(Reporter)

Comment 51

2 years ago
Comment on attachment 8577695 [details] [diff] [review]
AsyncStorage.diff

...and we can use the folded version at the end for upload.
Attachment #8577695 - Flags: review?(philipp)

Comment 52

2 years ago
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

2 years ago
Ok found it here: 
http://ftp.mozilla.org/pub/mozilla.org/calendar/lightning/try-builds/mozilla@kewis.ch-e8c060c5ae85/
(Reporter)

Comment 54

2 years ago
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.
Flags: needinfo?(anderson)

Comment 55

2 years ago
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.
(Reporter)

Comment 56

2 years ago
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.
(Reporter)

Comment 57

2 years ago
Comment on attachment 8577696 [details] [diff] [review]
AsyncStorageExtras.diff

r- because we need to fix the debug builds bustage. Reid, any updates?
Attachment #8577696 - Flags: review?(philipp) → review-
(Reporter)

Comment 58

2 years ago
This won't make it for 4.0, but I do hope we can get this in soon!
Target Milestone: 4.0 → ---
Blocks: 780992
FYI Reid wrote me a few weeks ago "I hope to get back and finish it this summer, but can't promise that."
You need to log in before you can comment on or make changes to this bug.