IndexedDB: One transaction coordinator per database
Created attachment 165473 [details] Patch
dgrogan@, alecflett@ - please take a look This turned out to be pretty straightforward to implement. Not a huge win, though, since transactions within databases are still executed serially.
Comment on attachment 165473 [details] Patch Attachment 165473 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13990866
Created attachment 165475 [details] Patch
Comment on attachment 165475 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=165475&action=review LGTM This is great, seems like an easy win. readwrite transactions on separate databases will run in parallel too, right? > LayoutTests/storage/indexeddb/resources/transaction-coordination-across-databases.js:6 > +description("Check that transactions in different databases can run in parallel."); OT: I ran this test in content_shell and can get it to wedge by hitting refresh at various stages of completion. I thought your two-phase consolidation eliminated the last known wedge vector, but it looks like there's still at least one outstanding. > LayoutTests/storage/indexeddb/resources/transaction-coordination-across-databases.js:97 > + }()); It seems that this pattern is shorthand for function doTransaction1Get() { ... } doTransaction1Get(); I suppose it also avoids putting doTransaction1Get in startWork()'s namespace and serves as notice to anyone reading the code that doTransaction1Get() is not called anywhere else. Am I missing any other noteworthy properties of this pattern? Also, does it have a name? > LayoutTests/storage/indexeddb/resources/transaction-readwrite-exclusive.js:54 > + evalAndLog("transaction1 = db1.transaction('store', 'readwrite')"); Given the level of parallelization that this patch gives us, where we serialize per database, this test would still pass if these were readonly transactions, correct?
(In reply to comment #5) > > This is great, seems like an easy win. readwrite transactions on separate databases will run in parallel too, right? Correct. I was thinking of readonly as being the "base" case to test for, but ensuring readwrite are parallelized across databases is a good point; I'll update the test. > > LayoutTests/storage/indexeddb/resources/transaction-coordination-across-databases.js:97 > > + }()); > > It seems that this pattern is shorthand for function doTransaction1Get() { ... } doTransaction1Get(); I suppose it also avoids putting doTransaction1Get in startWork()'s namespace and serves as notice to anyone reading the code that doTransaction1Get() is not called anywhere else. Am I missing any other noteworthy properties of this pattern? Nope. I'll flatten it out a bit. > Also, does it have a name? It's an "Immediately Invoked Function Expression" (IIFE) with a named function so it can be called recursively. > > LayoutTests/storage/indexeddb/resources/transaction-readwrite-exclusive.js:54 > > + evalAndLog("transaction1 = db1.transaction('store', 'readwrite')"); > > Given the level of parallelization that this patch gives us, where we serialize per database, this test would still pass if these were readonly transactions, correct? Correct. I could add a test that validates the *current* limitation but thought this was enough for now.
Created attachment 165642 [details] Patch
tony@ - r?
Comment on attachment 165642 [details] Patch Clearing flags on attachment: 165642 Committed r129534: <http://trac.webkit.org/changeset/129534>
All reviewed patches have been landed. Closing bug.