Bug 164932 – IndexedDB 2.0: The client's transaction operation queue should flush as much to the server as possible

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

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*

[Brady Eidson]

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

[Brady Eidson]

Created attachment 295206 [details]
Patch

[Brady Eidson]

Created attachment 295214 [details]
Patch

[Brady Eidson]

Comment on attachment 295214 [details]
Patch

Still working on last minute cleanups

[Brady Eidson]

Created attachment 295218 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

Nice catch with that test - a very special kind of operation wasn't accounted for which is a put/add operation *with* blobs.

*sigh*

[Brady Eidson]

Created attachment 295248 [details]
Patch

[Brady Eidson]

This is for landing. I will cq+ once the previous failing tests are clear.

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

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)

[Brady Eidson]

(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.

[Brady Eidson]

So far, absolutely cannot repro in a debug build, which severely limits my logging. *Sigh*

[Brady Eidson]

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.

[Brady Eidson]

(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.

[Brady Eidson]

(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.

[Brady Eidson]

Resolving this will require a refactor which I'll do first over in https://bugs.webkit.org/show_bug.cgi?id=165000

[Darin Adler]

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.

[Brady Eidson]

(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)

[Brady Eidson]

Created attachment 295603 [details]
Patch

[Brady Eidson]

Looks solid locally. Giving EWS a run before landing.

[Brady Eidson]

Bots are seeing a crash in imported/blink/storage/indexeddb/blob-valid-before-commit.html and I can repro locally.

Exploring.

[Brady Eidson]

(Debug bots assert, that's why release bots went green)

[Build Bot]

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

[Build Bot]

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

[Brady Eidson]

Created attachment 295612 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 295612 [details]
Patch

Clearing flags on attachment: 295612

Committed r209086: <http://trac.webkit.org/changeset/209086>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.