Bug 89239 – IndexedDB: Don't do full commit for empty transactions

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

For read-only IDB transactions we go through the full motions of an backing store commit, including creating/writing a (presumably empty) leveldb writebatch.

This can be short-circuited, which would eliminate unnecessary work and make the read-only transaction resilient to backing store faults (e.g. disk has gone away but data is still in memory)

[Joshua Bell]

Created attachment 153369 [details]
Patch

[Joshua Bell]

I don't know if we actually want this... the only practical upshot is that if leveldb has entered a bad state due to I/O error the read transaction won't trip it if the data is in an in-memory table.

[David Grogan]

Comment on attachment 153369 [details]
Patch

LGTM

Bike-shedding: Agreed that the desirability question doesn't have a clear answer. My vote would be to add this code, only because there's a conceivable scenario where a docs user wants to be able to read their docs offline even if they can't write due to some flaw in either our code or docs'. I suppose the downside is that this could make such errors less discoverable.  No strong opinion.

If you do add this code, could you add a comment indicating how it could matter?

[Alec Flett]

My vote is also to land this - I think this also helps in the case of a database going read-only, filling up, or overrunning quota, right? (Not all of that may kick this in today...) 

Seems like the downside is a tiny amount of code complexity, but it's pretty darn minor. Also agreed on the comment - the "if (m_tree.is_empty())" does not read "if this is a read-only transaction" to me - though maybe the naming of m_tree is the culprit there. not sure.

[Joshua Bell]

Comment on attachment 153369 [details]
Patch

What happens when we iterate a cursor during a read-only transaction - do we clean up stale entries? Should those be committed or should that wait for a follow-up read/write transaction?

If we commit, then this doesn't address the scenario where this is defensive against I/O errors.

If we don't commit, all subsequent transactions pay that penalty.

Also note that the patch as written checks at a lower level - if index cleanup was done, it would still commit, so it's actually moot for this patch. I'm tempted to land it as-is, but perhaps make the bug title less grandiose.

[Joshua Bell]

Changed title to "don't do full commit for empty transactions"

Read-only transactions can still be non-empty if they include index cleanup.

[Joshua Bell]

Created attachment 166971 [details]
Patch

[Joshua Bell]

michaeln@ came up with the same patch when looking into performance for read-only transactions:

https://groups.google.com/a/chromium.org/d/msg/chromium-html5/FRbwyUgspQ0/2xZkWZv1zQAJ

[Joshua Bell]

tony@ - r?

[WebKit Review Bot]

Comment on attachment 166971 [details]
Patch

Clearing flags on attachment: 166971

Committed r130338: <http://trac.webkit.org/changeset/130338>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

Woohoo... nice low hanging watermelon sized 10x improvement in small reads :)