I noticed in my Nightly's about:telemetry that FHR executed a "COMMIT TRANSACTION" statement on the main thread causing a 190ms jank. After some testing, I found that this commit is executed on the main thread 10 seconds after every startup but the jank is severe only when the health report DB is first created. I think one possible explanation is that you might calling "BEGIN DEFERRED" on the main thread, then executing all the CREATE TABLE/VIEW/INDEX calls on a helper thread and finally calling "COMMIT TRANSACTION" on the main thread, causing all the DB changes to be committed to disk on the main thread. I attached the sequence of SQL statements executed for FHR DB creation.

I actually think we mishandled transactions in SQLite.jsm::executeTransaction indeed the only way to properly do transaction off mainthread is connection.executeStatements(multiple statements)

that method is indeed dangerous and should be removed or fixed to just accept list of queries and forward to executeStatements. Unfortunately this means there's no way to do anything else than queries in a transaction

s/executeStatements/executeAsync/

This is a Sqlite.jsm issue. If we fix it for Sqlite.jsm, we'll fix it for FHR. Changing component.

This patch implements manual transaction management in Sqlite.jsm by issuing the "BEGIN TRANSACTION" etc statements through the execute() API. I haven't looked at what Storage does under the hood. So, I'm not sure how this varies, if at all. Public API has not changed with this patch!

Comment on attachment 703133 [details] [diff] [review] Manual transaction management, v1 Review of attachment 703133 [details] [diff] [review]: ----------------------------------------------------------------- This is a great idea, my fault for not having thought about it before! ::: toolkit/modules/Sqlite.jsm @@ +254,5 @@ > + let deferred = Promise.defer(); > + > + // If we have a transaction, we need to wait for it to be rolled back. > + // Otherwise, we proceed immediately to finalizing the connection. > + if (!this._inProgressTransaction) { the comment punches with the if condition. I'd invert the comment as "If we don't have a transaction in progress, proceed immediately to finalizing the connection. Otherwise, we need to wait..." @@ +265,5 @@ > + let onRollback = this._finalize.bind(this, deferred); > + > + this.execute("ROLLBACK TRANSACTION").then(onRollback, onRollback); > + this._inProgressTransaction.reject(new Error("Connection being closed.")); > + this._inProgressTransaction = null; I have just one doubt, here you call rollback, reject the transaction and set it to null, but if this actually happens we are in the middle of the transaction Task... won't that again try to ROLLBACK/COMMIT? Maybe before trying to commit/rollback in executeTransaction we should check if _inProgressTransaction is still valid? @@ +454,5 @@ > * @param type optional > * One of the TRANSACTION_* constants attached to this type. > */ > executeTransaction: function (func, type=this.TRANSACTION_DEFERRED) { > this._ensureOpen(); I think it's worth to check type is one of the supported values, since now you are directly injecting it into a query... any case we take input and inject it into a query we must be sure to validate it. @@ +466,4 @@ > let deferred = Promise.defer(); > this._inProgressTransaction = deferred; > + Task.spawn(function doTransaction() { > + yield this.execute("BEGIN " + type + " TRANSACTION"); I don't think you need to yield here, since everything is serialized on the thread, any query executed in the function will be naturally enqueued to this one... if you don't yield we may start executing the function contents earlier. not a big deal btw.

(In reply to Marco Bonardo [:mak] from comment #6) > @@ +265,5 @@ > > + let onRollback = this._finalize.bind(this, deferred); > > + > > + this.execute("ROLLBACK TRANSACTION").then(onRollback, onRollback); > > + this._inProgressTransaction.reject(new Error("Connection being closed.")); > > + this._inProgressTransaction = null; > > I have just one doubt, here you call rollback, reject the transaction and > set it to null, but if this actually happens we are in the middle of the > transaction Task... won't that again try to ROLLBACK/COMMIT? > Maybe before trying to commit/rollback in executeTransaction we should check > if _inProgressTransaction is still valid? This is a valid point! I noticed this while testing last night. However, it appeared to be harmless because the statement is issued on a connection that is closed or it errors immediately because the transaction was already finished. Between a night to sleep on it and your comment, I agree we should add extra detection here. Patch forthcoming. > @@ +454,5 @@ > > * @param type optional > > * One of the TRANSACTION_* constants attached to this type. > > */ > > executeTransaction: function (func, type=this.TRANSACTION_DEFERRED) { > > this._ensureOpen(); > > I think it's worth to check type is one of the supported values, since now > you are directly injecting it into a query... any case we take input and > inject it into a query we must be sure to validate it. Makes sense. > @@ +466,4 @@ > > let deferred = Promise.defer(); > > this._inProgressTransaction = deferred; > > + Task.spawn(function doTransaction() { > > + yield this.execute("BEGIN " + type + " TRANSACTION"); > > I don't think you need to yield here, since everything is serialized on the > thread, any query executed in the function will be naturally enqueued to > this one... if you don't yield we may start executing the function contents > earlier. not a big deal btw. The benefit of yielding is that any error will abort the task. If we failed to yield, the error may or may not occur during the next statement that is part of the transaction. This could possibly lead to a statement being performed outside a transaction or in another active transaction (hopefully our code prevents the latter).

I could carry r+ forward. But, the amount of changed code is just beyond my threshold for good faith carry forwards :) Delta hits all points from previous review.

Comment on attachment 703449 [details] [diff] [review] Manual transaction management, v2 Review of attachment 703449 [details] [diff] [review]: ----------------------------------------------------------------- The interdiff looks good ::: toolkit/modules/tests/xpcshell/test_sqlite.js @@ +223,5 @@ > + errored = true; > + do_check_true(ex.message.startsWith("Unknown transaction type")); > + } finally { > + do_check_true(errored); > + } I don't remember why you don't like try { something(); do_throw("should have thrown"); } catch (ex) { ... }

Well, actually you may move TRANSACTION_TYPES to a const out of the module, we don't want to export it, otherwise it could be modified and the argument check is useless.

s/out of the module/out of the object/

We use Object.freeze() on the prototype so the only way to "mutate" TRANSACTION_TYPES is to Object.defineProperty(connection, "TRANSACTION_TYPES", {...}) and that will only have an impact on the specified object instance. If someone is that stupid, then they deserve to eat the consequences, IMO.

whatever, I think constants in the module scope are cleaner, but it's not that important, I'll let you take what you think makes sense.

Comment on attachment 703449 [details] [diff] [review] Manual transaction management, v2 [Approval Request Comment] Firefox Health Report perf-related uplift request. Backend code only.

(In reply to Gregory Szorc [:gps] from comment #16) > Comment on attachment 703449 [details] [diff] [review] > Manual transaction management, v2 > > [Approval Request Comment] > > Firefox Health Report perf-related uplift request. Backend code only. Could you help understand the risk involved on this uplift ? Has there been specific testing keeping in mind the chances of regression's this huge patch may incur ? Were there any significant obvious perf improvements observed since the patch landed on m-c ? Thanks in advance for all the answers :)

(In reply to bhavana bajaj [:bajaj] from comment #17) > Has there been specific testing keeping in mind the chances of regression's this huge patch may incur ? If this patch broke things, FHR wouldn't work. Furthermore, FHR is the only consumer of this file. Uplifting only impacts FHR. > Were there any significant obvious perf improvements observed > since the patch landed on m-c ? We know we were performing SQL transactions on the main thread before this patch and we no longer do this with this patch applied. While Vladan has not explicitly reported a perf win, I suspect we have attained one. Furthermore, I'd like the code for FHR to be as similar between 20 and 21 as possible. This will make future uplifts and performance regression tracking much easier to manager. The exception to this will be patches that are not FHR specific. For these, I will give a more detailed uplift request. I was under the impression that FHR-specific patches are essentially getting an automatic stamp of approval, which is why I've skimped on uplift details for many patches. If you want me to change behavior, just ask.

(In reply to Gregory Szorc [:gps] from comment #18) > We know we were performing SQL transactions on the main thread before this > patch and we no longer do this with this patch applied. While Vladan has not > explicitly reported a perf win, I suspect we have attained one. doing mixed-threads storage access like this can cause contention, we had many cases in the past. contention means the browser stays beach-balling for seconds. This is far worse than simple perf issues.

(just to clarify, my comments refers to the situation before the patch fixed it, the patch avoids the contention problem)

(In reply to Gregory Szorc [:gps] from comment #18) > (In reply to bhavana bajaj [:bajaj] from comment #17) > > Has there been specific testing keeping in mind the chances of regression's this huge patch may incur ? > > If this patch broke things, FHR wouldn't work. Furthermore, FHR is the only > consumer of this file. Uplifting only impacts FHR. > > > Were there any significant obvious perf improvements observed > > since the patch landed on m-c ? > > We know we were performing SQL transactions on the main thread before this > patch and we no longer do this with this patch applied. While Vladan has not > explicitly reported a perf win, I suspect we have attained one. > > Furthermore, I'd like the code for FHR to be as similar between 20 and 21 as > possible. This will make future uplifts and performance regression tracking > much easier to manager. The exception to this will be patches that are not > FHR specific. For these, I will give a more detailed uplift request. I was > under the impression that FHR-specific patches are essentially getting an > automatic stamp of approval, which is why I've skimped on uplift details for > many patches. If you want me to change behavior, just ask. Thanks for all the answers.The size of the patch compelled me to raise questions here :) Will keep in mind your point on skipping the uplift details & I am ok with this as long as the impact is only FHR & it also depends on where we are in the cycle.If we have any specific questions we will shoot as usual :)