Bug 164932 - IndexedDB 2.0: The client's transaction operation queue should flush as much to the server as possible
Summary: IndexedDB 2.0: The client's transaction operation queue should flush as much ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords:
Depends on: 165000
Blocks: 160306
  Show dependency treegraph
 
Reported: 2016-11-18 10:01 PST by Brady Eidson
Modified: 2016-11-29 13:08 PST (History)
6 users (show)

See Also:


Attachments
Patch (10.77 KB, patch)
2016-11-18 15:52 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2016-11-18 16:31 PST, Brady Eidson
beidson: review-
Details | Formatted Diff | Diff
Patch (10.77 KB, patch)
2016-11-18 16:36 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews126 for ios-simulator-wk2 (18.50 MB, application/zip)
2016-11-18 17:48 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-yosemite (1.84 MB, application/zip)
2016-11-18 18:10 PST, Build Bot
no flags Details
Patch (12.06 KB, patch)
2016-11-18 21:09 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-yosemite-wk2 (1.05 MB, application/zip)
2016-11-18 21:52 PST, Build Bot
no flags Details
Patch (10.12 KB, patch)
2016-11-29 10:21 PST, Brady Eidson
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-yosemite (917.44 KB, application/zip)
2016-11-29 11:21 PST, Build Bot
no flags Details
Patch (11.42 KB, patch)
2016-11-29 11:33 PST, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2016-11-18 10:01:13 PST
IndexedDB 2.0: The client's transaction operation queue should always flush to the server

Right now, if the web page does 1000 insert's in rapid succession, the client builds up 1000 operations in the transaction queue.

We send the first insert over to the server, then wait for the response.

Then we send the second insert over.

etc etc.

Turns out - unsurprisingly - this leads to massive amounts of idle time on both the client and server side while each waits for feedback from the other.

Flushing the transaction operation queue can speed up certain usage patterns *significantly*
Comment 1 Brady Eidson 2016-11-18 15:42:42 PST
In testing and fixing up this patch, it became apparently you can't *ALWAYS* flush *EVERYTHING* to the server.

Retitling:
IndexedDB 2.0: The client's transaction operation queue should flush as much to the server as possible
Comment 2 Brady Eidson 2016-11-18 15:52:59 PST
Created attachment 295206 [details]
Patch
Comment 3 Brady Eidson 2016-11-18 16:31:01 PST
Created attachment 295214 [details]
Patch
Comment 4 Brady Eidson 2016-11-18 16:31:49 PST
Comment on attachment 295214 [details]
Patch

Still working on last minute cleanups
Comment 5 Brady Eidson 2016-11-18 16:36:13 PST
Created attachment 295218 [details]
Patch
Comment 6 Build Bot 2016-11-18 17:48:21 PST
Comment on attachment 295218 [details]
Patch

Attachment 295218 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/2539727

New failing tests:
imported/blink/storage/indexeddb/blob-valid-before-commit.html
Comment 7 Build Bot 2016-11-18 17:48:24 PST
Created attachment 295238 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6
Comment 8 Build Bot 2016-11-18 18:10:47 PST
Comment on attachment 295218 [details]
Patch

Attachment 295218 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2539925

New failing tests:
imported/blink/storage/indexeddb/blob-valid-before-commit.html
Comment 9 Build Bot 2016-11-18 18:10:49 PST
Created attachment 295239 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 10 Brady Eidson 2016-11-18 20:54:59 PST
Nice catch with that test - a very special kind of operation wasn't accounted for which is a put/add operation *with* blobs.

*sigh*
Comment 11 Brady Eidson 2016-11-18 21:09:20 PST
Created attachment 295248 [details]
Patch
Comment 12 Brady Eidson 2016-11-18 21:12:07 PST
This is for landing. I will cq+ once the previous failing tests are clear.
Comment 13 Build Bot 2016-11-18 21:52:10 PST
Comment on attachment 295248 [details]
Patch

Attachment 295248 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2540876

New failing tests:
imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html
Comment 14 Build Bot 2016-11-18 21:52:13 PST
Created attachment 295251 [details]
Archive of layout-test-results from ews104 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5
Comment 15 Brady Eidson 2016-11-19 14:03:20 PST
Interested set of failings.

Digging in on imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html first: Can confirm it's a flaky failure locally but, bizarrely, only in WK2

(Private browsing works in-memory, in-process on both WK1 and WK2)
Comment 16 Brady Eidson 2016-11-19 14:04:22 PST
(In reply to comment #15)
> Interested set of failings.
> 
> Digging in on
> imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-
> event_order.html first: Can confirm it's a flaky failure locally but,
> bizarrely, only in WK2
> 
> (Private browsing works in-memory, in-process on both WK1 and WK2)

Never mind.
`run-webkit-tests -1 imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html --repeat-each=10` got it to fail ~5 times.
Comment 17 Brady Eidson 2016-11-19 14:07:06 PST
So far, absolutely cannot repro in a debug build, which severely limits my logging. *Sigh*
Comment 18 Brady Eidson 2016-11-19 14:14:39 PST
rq_add2 causes a constraint error, so the rq_add3 should not succeed, but sometimes it does.

Two possible solutions:
1 - rq_add3 is already in flight in the server when the client receives the constraint error. If the server tracked "this is aborted due to the constraint error" then it could also abort rq_add3

2 - Alternately, the client remembers all in-flight requests and *Should* abort then when it aborts the transaction for a constraint error. 

#2 is preferred. And my guess why it's failing is because it schedules the abort with a spin of the runloop but, before that happens, the success for rq_add3 comes in from the server.

Aborts due to contraints/failed requests need to be synchronous, maybe.
Comment 19 Brady Eidson 2016-11-19 14:37:28 PST
(In reply to comment #18)
> rq_add2 causes a constraint error, so the rq_add3 should not succeed, but
> sometimes it does.
> 
> Two possible solutions:
> 1 - rq_add3 is already in flight in the server when the client receives the
> constraint error. If the server tracked "this is aborted due to the
> constraint error" then it could also abort rq_add3
> 
> 2 - Alternately, the client remembers all in-flight requests and *Should*
> abort then when it aborts the transaction for a constraint error. 
> 
> #2 is preferred. And my guess why it's failing is because it schedules the
> abort with a spin of the runloop but, before that happens, the success for
> rq_add3 comes in from the server.
> 
> Aborts due to contraints/failed requests need to be synchronous, maybe.

It has to be #2, and it's semi-nefarious.

The 2nd put comes back with a failure, and the event dispatch is scheduled.
Then the 3rd comes back with a success, and the event dispatch is scheduled.
Then the event for the 2nd fires, and since it's not handled we schedule an abort transaction.
Then the success event for the 3rd fires.
Then the transaction is aborted.

Gotta break the event for the 3rd firing.
Comment 20 Brady Eidson 2016-11-19 16:15:14 PST
(In reply to comment #19)
> (In reply to comment #18)
> > rq_add2 causes a constraint error, so the rq_add3 should not succeed, but
> > sometimes it does.
> > 
> > Two possible solutions:
> > 1 - rq_add3 is already in flight in the server when the client receives the
> > constraint error. If the server tracked "this is aborted due to the
> > constraint error" then it could also abort rq_add3
> > 
> > 2 - Alternately, the client remembers all in-flight requests and *Should*
> > abort then when it aborts the transaction for a constraint error. 
> > 
> > #2 is preferred. And my guess why it's failing is because it schedules the
> > abort with a spin of the runloop but, before that happens, the success for
> > rq_add3 comes in from the server.
> > 
> > Aborts due to contraints/failed requests need to be synchronous, maybe.
> 
> It has to be #2, and it's semi-nefarious.
> 
> The 2nd put comes back with a failure, and the event dispatch is scheduled.
> Then the 3rd comes back with a success, and the event dispatch is scheduled.
> Then the event for the 2nd fires, and since it's not handled we schedule an
> abort transaction.
> Then the success event for the 3rd fires.
> Then the transaction is aborted.
> 
> Gotta break the event for the 3rd firing.

While working on a patch where IDBRequests remember the events they've queued up in the ScriptExecutionContext and know how to cancel them later, I realize there's still edge cases that will come up.

New strategy I'm considering: Operations that come back complete go into a "CompleteOperationQueue", and we call the complete handler on each one-by-one.

The idea being that if an operation completing results in event dispatch, we'd first wait on the event to dispatch.
Comment 21 Brady Eidson 2016-11-20 14:19:02 PST
Resolving this will require a refactor which I'll do first over in https://bugs.webkit.org/show_bug.cgi?id=165000
Comment 22 Darin Adler 2016-11-20 16:17:57 PST
Comment on attachment 295248 [details]
Patch

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

imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-event_order.html failure?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:256
> +    Vector<RefPtr<IDBClient::TransactionOperation>> inProgressAbortVector;

Ref instead of RefPtr?

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:261
> +    while (!m_transactionOperationsInProgressQueue.isEmpty()) {
> +        auto operation = m_transactionOperationsInProgressQueue.takeFirst();
> +        inProgressAbortVector.uncheckedAppend(operation);
> +    }

Would read better without the local variable, I think.
Comment 23 Brady Eidson 2016-11-22 20:15:44 PST
(In reply to comment #22)
> Comment on attachment 295248 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=295248&action=review
> 
> imported/w3c/IndexedDB-private-browsing/idbobjectstore_createIndex7-
> event_order.html failure?
> 
> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:256
> > +    Vector<RefPtr<IDBClient::TransactionOperation>> inProgressAbortVector;
> 
> Ref instead of RefPtr?
> 
> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:261
> > +    while (!m_transactionOperationsInProgressQueue.isEmpty()) {
> > +        auto operation = m_transactionOperationsInProgressQueue.takeFirst();
> > +        inProgressAbortVector.uncheckedAppend(operation);
> > +    }
> 
> Would read better without the local variable, I think.

Will make both of those changes before landing this (but still working on a pre-requisite patch over in https://webkit.org/b/165000)
Comment 24 Brady Eidson 2016-11-29 10:21:49 PST
Created attachment 295603 [details]
Patch
Comment 25 Brady Eidson 2016-11-29 10:22:37 PST
Looks solid locally. Giving EWS a run before landing.
Comment 26 Brady Eidson 2016-11-29 11:12:47 PST
Bots are seeing a crash in imported/blink/storage/indexeddb/blob-valid-before-commit.html and I can repro locally.

Exploring.
Comment 27 Brady Eidson 2016-11-29 11:13:20 PST
(Debug bots assert, that's why release bots went green)
Comment 28 Build Bot 2016-11-29 11:21:44 PST
Comment on attachment 295603 [details]
Patch

Attachment 295603 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2589791

New failing tests:
imported/blink/storage/indexeddb/blob-valid-before-commit.html
Comment 29 Build Bot 2016-11-29 11:21:48 PST
Created attachment 295610 [details]
Archive of layout-test-results from ews101 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-yosemite  Platform: Mac OS X 10.10.5
Comment 30 Brady Eidson 2016-11-29 11:33:58 PST
Created attachment 295612 [details]
Patch
Comment 31 WebKit Commit Bot 2016-11-29 13:08:31 PST
Comment on attachment 295612 [details]
Patch

Clearing flags on attachment: 295612

Committed r209086: <http://trac.webkit.org/changeset/209086>
Comment 32 WebKit Commit Bot 2016-11-29 13:08:37 PST
All reviewed patches have been landed.  Closing bug.