Because sqlite operaions are basically atomic, every insertion and deletion in sync will require block I/O.
Created attachment 87966 [details] Patch
I think beidson worked on database support? I can't remember.
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...
(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).
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.
Created attachment 110477 [details] Patch
(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 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?
Created attachment 110623 [details] Patch
(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 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.
Created attachment 110625 [details] Patch
Comment on attachment 110625 [details] Patch Clearing flags on attachment: 110625 Committed r97223: <http://trac.webkit.org/changeset/97223>
All reviewed patches have been landed. Closing bug.