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).

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

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

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...

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

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.

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

I do like your approach though to writing items though.

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!

(In reply to neil@parkwaycc.co.uk 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.

(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.

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.

(In reply to neil@parkwaycc.co.uk 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.

(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

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

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

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

(In reply to Geoff Lankow from comment #79)

use of Components.Exception fixed.

Thanks for finding that.

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

(In reply to neil@parkwaycc.co.uk 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.

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. ;-)

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.

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.

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

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.

Pushed by geoff@darktrojan.net:
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

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

(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.

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