Open Bug 854973 Opened 7 years ago Updated 10 months ago

Optimize away empty deferred transactions in Sqlite.jsm

Categories

(Toolkit :: Storage, defect, P3)

22 Branch
defect

Tracking

()

People

(Reporter: gps, Unassigned)

Details

Attachments

(1 file)

In bug 854606 we noticed that FHR was creating a lot of "empty" deferred transactions. Since deferred transactions don't have any side effects until the first statement is executed, we could defer the BEGIN TRANSACTION until the first statement is executed. If no statement is ever executed, we don't perform the BEGIN TRANSACTION and COMMIT.

This will result in cleaner code for consumers of Sqlite.jsm (they don't need to track if they perform statements and thus can blindly enter transaction contexts without worrying about overhead of empty transactions).
This should do it!
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #729713 - Flags: review?(mak77)
Comment on attachment 729713 [details] [diff] [review]
Optimize away empty deferred transactions, v1

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

fwiw, SQLite already executes a deferred transaction for each write statement, so you could optimize this even more by not creating a transaction if only a single statement is executed, rather than only empty transactions. Though the overhead is minor (it's just the BEGIN and COMMIT additional executions) so we may not care.

A couple questions:

::: toolkit/modules/Sqlite.jsm
@@ +232,5 @@
>    TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
>  
> +  _DEFERRED_NONE: 0,
> +  _DEFERRED_WAITING: 1,
> +  _DEFERRED_STATEMENT_PERFORMED: 2,

I though these were here to be used by the test, but it's not, so you may use the module scope to hide them in a DEFERRED_STATE = { NONE: 0, WAITING: 1, ACTIVE: 2} global object...

@@ +573,5 @@
> +
> +        // Our temporary prototype overwrites the execute APIs and
> +        // injects the BEGIN DEFERRED TRANSACTION when the first
> +        // statement executes. Then, it uninstalls itself.
> +        this.__proto__ = PendingDeferredTransactionConnectionProto;

I know this is all shiny and cool, but actually wouldn't be more readable and less error-prone a very simple
if (DEFERRED_WAITING) {
  BEGIN TRANSACTION
  DEFERRED_PERFORMED
}
at the beginning of execute and executeCached?
Btw, notice those are not the only calls that may write to the db, set schemaVersion should do the same and I can see a consumer opening a transaction for migrating the schema version...
Attachment #729713 - Flags: review?(mak77) → feedback+
(In reply to Marco Bonardo [:mak] from comment #2)
> fwiw, SQLite already executes a deferred transaction for each write
> statement, so you could optimize this even more by not creating a
> transaction if only a single statement is executed, rather than only empty
> transactions. Though the overhead is minor (it's just the BEGIN and COMMIT
> additional executions) so we may not care.

Except you don't know you are only going to execute a single statement until the statement has already executed. By that time, it's too late to go back and retroactively BEGIN TRANSACTION if you will be executing more than 1 statement.

> ::: toolkit/modules/Sqlite.jsm
> @@ +232,5 @@
> >    TRANSACTION_TYPES: ["DEFERRED", "IMMEDIATE", "EXCLUSIVE"],
> >  
> > +  _DEFERRED_NONE: 0,
> > +  _DEFERRED_WAITING: 1,
> > +  _DEFERRED_STATEMENT_PERFORMED: 2,
> 
> I though these were here to be used by the test, but it's not, so you may
> use the module scope to hide them in a DEFERRED_STATE = { NONE: 0, WAITING:
> 1, ACTIVE: 2} global object...

Good catch.

> @@ +573,5 @@
> > +
> > +        // Our temporary prototype overwrites the execute APIs and
> > +        // injects the BEGIN DEFERRED TRANSACTION when the first
> > +        // statement executes. Then, it uninstalls itself.
> > +        this.__proto__ = PendingDeferredTransactionConnectionProto;
> 
> I know this is all shiny and cool, but actually wouldn't be more readable
> and less error-prone a very simple
> if (DEFERRED_WAITING) {
>   BEGIN TRANSACTION
>   DEFERRED_PERFORMED
> }
> at the beginning of execute and executeCached?

The reason I didn't do this is because the logic for executing statements is already quite complicated and I didn't want to complicate it even more. When you consider that BEGIN TRANSACTION could error, things become quite complicated rather quickly if this logic were inlined.

> Btw, notice those are not the only calls that may write to the db, set
> schemaVersion should do the same and I can see a consumer opening a
> transaction for migrating the schema version...

Indeed. We should probably rewrite those functions to go through the regular statement execution flow rather than use the special functions available on the storage connection.
I'm not actively working on this.
Assignee: gps → nobody
Status: ASSIGNED → NEW
Component: General → Storage
Priority: -- → P3
You need to log in before you can comment on or make changes to this bug.