Bug 57694 – StorageAreaSync::sync needs a transaction for better performance.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Ryuan Choi]

Because sqlite operaions are basically atomic,
every insertion and deletion in sync will require block I/O.

[Ryuan Choi]

Created attachment 87966 [details]
Patch

[Eric Seidel (no email)]

I think beidson worked on database support?  I can't remember.

[Brady Eidson]

There's 3 different explanations for why this bug exists.

1 - The title - "::sync requires slow and heavy I/O"
2 - The first comment - "Because sqlite operations are basically atomic, writes will require block I/O"
3 - The ChangeLog for the patch - "Need a transaction in case there's a crash or a power off"

So, which one is it?

r- until we clear that up...

[Ryuan Choi]

(In reply to comment #3)
> There's 3 different explanations for why this bug exists.
> 
> 1 - The title - "::sync requires slow and heavy I/O"
> 2 - The first comment - "Because sqlite operations are basically atomic, writes will require block I/O"
> 3 - The ChangeLog for the patch - "Need a transaction in case there's a crash or a power off"
> 
> So, which one is it?
> 
> r- until we clear that up...

Sorry about confusion.
What I mean is "StorageAreaSync::sync needs a transaction for the performance."
I believe that batch operation with a transaction can save I/O (and computing time little bit).

[Gustavo Sverzut Barbieri]

informal r+ here.

Brady, actually it was an explanation problem, not patch problem. At least points you listed are connected:

"SQLite operations outside a transaction are all atomic and thus writes will block on I/O, which makes the process slow during heavy I/O"

The third point, explains why a test is not needed: this would only be exercised during premature process death, if it dies before transaction is committed to disk, nothing is actually saved. This is actually BETTER for consistency, as the database will only contain consistent data and not partially inserted values.

[Ryuan Choi]

Created attachment 110477 [details]
Patch

[Ryuan Choi]

(In reply to comment #3)
> There's 3 different explanations for why this bug exists.
> 
> 1 - The title - "::sync requires slow and heavy I/O"
> 2 - The first comment - "Because sqlite operations are basically atomic, writes will require block I/O"
> 3 - The ChangeLog for the patch - "Need a transaction in case there's a crash or a power off"
> 
> So, which one is it?
> 
> r- until we clear that up...

Sorry once more.
I changed title and comment.
If patch itself wasn't wrong, could you review this once more?

[Darin Adler]

Comment on attachment 110477 [details]
Patch

Seems like a good change. Is this the only case where we need a transaction and don’t have one?

[Ryuan Choi]

Created attachment 110623 [details]
Patch

[Ryuan Choi]

(In reply to comment #8)
> (From update of attachment 110477 [details])
> Seems like a good change. Is this the only case where we need a transaction and don’t have one?

Thanks for the comment.

I checked other areas using SQLiteStatement.
They looks fine for me.

Some modules like IconDatabase already are using transaction for write operations.

IMO, StorageAreaSync::sync also need a transaction.

[Darin Adler]

Comment on attachment 110623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=110623&action=review

> Source/WebCore/storage/StorageAreaSync.cpp:434
> +    SQLiteTransaction transaction(m_database, false);

There is no need to say “false” here; the default is false.

[Ryuan Choi]

Created attachment 110625 [details]
Patch

[WebKit Review Bot]

Comment on attachment 110625 [details]
Patch

Clearing flags on attachment: 110625

Committed r97223: <http://trac.webkit.org/changeset/97223>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.