Bug 97501 – IndexedDB: One transaction coordinator per database

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: One transaction coordinator per database

[Joshua Bell]

Created attachment 165473 [details]
Patch

[Joshua Bell]

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.

[WebKit Review Bot]

Comment on attachment 165473 [details]
Patch

Attachment 165473 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13990866

[Joshua Bell]

Created attachment 165475 [details]
Patch

[David Grogan]

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?

[Joshua Bell]

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

[Joshua Bell]

Created attachment 165642 [details]
Patch

[Joshua Bell]

tony@ - r?

[WebKit Review Bot]

Comment on attachment 165642 [details]
Patch

Clearing flags on attachment: 165642

Committed r129534: <http://trac.webkit.org/changeset/129534>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.