Bug 57694 - StorageAreaSync::sync needs a transaction for better performance.
Summary: StorageAreaSync::sync needs a transaction for better performance.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Ryuan Choi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-04-01 21:59 PDT by Ryuan Choi
Modified: 2011-10-11 20:28 PDT (History)
7 users (show)

See Also:


Attachments
Patch (1.60 KB, patch)
2011-04-01 22:16 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.20 KB, patch)
2011-10-10 22:57 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.25 KB, patch)
2011-10-11 18:16 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff
Patch (2.24 KB, patch)
2011-10-11 18:28 PDT, Ryuan Choi
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryuan Choi 2011-04-01 21:59:07 PDT
Because sqlite operaions are basically atomic,
every insertion and deletion in sync will require block I/O.
Comment 1 Ryuan Choi 2011-04-01 22:16:03 PDT
Created attachment 87966 [details]
Patch
Comment 2 Eric Seidel (no email) 2011-04-03 22:54:54 PDT
I think beidson worked on database support?  I can't remember.
Comment 3 Brady Eidson 2011-04-04 08:35:04 PDT
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...
Comment 4 Ryuan Choi 2011-04-04 16:54:15 PDT
(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).
Comment 5 Gustavo Sverzut Barbieri 2011-09-14 10:45:54 PDT
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.
Comment 6 Ryuan Choi 2011-10-10 22:57:35 PDT
Created attachment 110477 [details]
Patch
Comment 7 Ryuan Choi 2011-10-10 22:59:28 PDT
(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?
Comment 8 Darin Adler 2011-10-11 17:36:31 PDT
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?
Comment 9 Ryuan Choi 2011-10-11 18:16:55 PDT
Created attachment 110623 [details]
Patch
Comment 10 Ryuan Choi 2011-10-11 18:25:01 PDT
(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.
Comment 11 Darin Adler 2011-10-11 18:26:34 PDT
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.
Comment 12 Ryuan Choi 2011-10-11 18:28:56 PDT
Created attachment 110625 [details]
Patch
Comment 13 WebKit Review Bot 2011-10-11 20:28:24 PDT
Comment on attachment 110625 [details]
Patch

Clearing flags on attachment: 110625

Committed r97223: <http://trac.webkit.org/changeset/97223>
Comment 14 WebKit Review Bot 2011-10-11 20:28:30 PDT
All reviewed patches have been landed.  Closing bug.