Bug 100786 – Expose snapshots in platform/leveldb wrapper API

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Joshua Bell]

Expose snapshots in platform/leveldb wrapper API

[Joshua Bell]

Created attachment 171516 [details]
Patch

[Joshua Bell]

dgrogan@, alecflett@ - please take a look

Not used yet, but this could be the basis of parallel transactions. (Mostly an excuse to play with the leveldb snapshot API.)

[Alec Flett]

Comment on attachment 171516 [details]
Patch

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

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:96
> +class LevelDBSnapshotImpl : public LevelDBSnapshot {

My only thought here is that making this another separate heap-based object behind an interface seems unnecessary.. I don't know *exactly* what the ownership model is for LevelDB snapshots (I see DB::ReleaseSnapshot though) but does ownership of this thing change ever? (seems like no...its always bound to the transaction in question)

[Joshua Bell]

(In reply to comment #3)
> (From update of attachment 171516 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171516&action=review
> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:96
> > +class LevelDBSnapshotImpl : public LevelDBSnapshot {
> 
> My only thought here is that making this another separate heap-based object behind an interface seems unnecessary.. I don't know *exactly* what the ownership model is for LevelDB snapshots (I see DB::ReleaseSnapshot though) but does ownership of this thing change ever? (seems like no...its always bound to the transaction in question)

Since leveldb doesn't have transactions, in leveldb its separate; you either read from a specific snapshot or just the database.

The way we'd use it in IndexedDB we'd probably always have a IDBLevelDBBackingStoreTransaction own both a LevelDBSnapshot and a LevelDBTransaction. But I can imagine other clients of the WebCore::LevelDB* classes wanting to do things differently, so I stuck to the philosophy of making the WebCore::LevelDB* classes wrappers of leveldb (with value-adds). 

Making it "just" a property of the LevelDBTransaction (a flag passed in during construction) is doable.

[Alec Flett]

Comment on attachment 171516 [details]
Patch

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

>>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:96
>>> +class LevelDBSnapshotImpl : public LevelDBSnapshot {
>> 
>> My only thought here is that making this another separate heap-based object behind an interface seems unnecessary.. I don't know *exactly* what the ownership model is for LevelDB snapshots (I see DB::ReleaseSnapshot though) but does ownership of this thing change ever? (seems like no...its always bound to the transaction in question)
> 
> Since leveldb doesn't have transactions, in leveldb its separate; you either read from a specific snapshot or just the database.
> 
> The way we'd use it in IndexedDB we'd probably always have a IDBLevelDBBackingStoreTransaction own both a LevelDBSnapshot and a LevelDBTransaction. But I can imagine other clients of the WebCore::LevelDB* classes wanting to do things differently, so I stuck to the philosophy of making the WebCore::LevelDB* classes wrappers of leveldb (with value-adds). 
> 
> Making it "just" a property of the LevelDBTransaction (a flag passed in during construction) is doable.

yeah - this is more or less what I meant - I think snapshots is a necessary primitive which lets you create consistent read transactions, at least in the sense that IDB thinks of them. They really aren't peers, one is a building block of the other.

[Joshua Bell]

(In reply to comment #5)
> yeah - this is more or less what I meant - I think snapshots is a necessary primitive which lets you create consistent read transactions, at least in the sense that IDB thinks of them. They really aren't peers, one is a building block of the other.

Yeah, but should we expose that building block separately for non-IDB usage?

I'm still on the fence. Bundling it into LevelDBTransaction would definitely be less code.

One could argue that maybe *all* LevelDBTransactions should work off of snapshots, matching the IDB semantics. We'd want to perf test that, though - right now LevelDBTransactions are "cheap" and also have no lifetime requirements w/r/t other LevelDB objects.

...

Also, this patch is incomplete: it doesn't handle iterators - LevelDBTransaction::createIterator() should use the associated snapshot, and LevelDBDatabase::createIterator() should take an optional snapshot.

[Joshua Bell]

Created attachment 171693 [details]
Patch

[Joshua Bell]

Updated patch w/ support and tests for snapshots w/ iterators (creating iterators against a snapshot, and creating iterators against a transaction based on a snapshot)

[Joshua Bell]

Created attachment 171955 [details]
Patch

[Early Warning System Bot]

Comment on attachment 171955 [details]
Patch

Attachment 171955 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14696475

[Early Warning System Bot]

Comment on attachment 171955 [details]
Patch

Attachment 171955 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14692640

[Joshua Bell]

Created attachment 171957 [details]
Patch

[Joshua Bell]

Based on conversations with alecflett@, just made LevelDBTransactions take and use snapshots implicitly. This matches the IndexedDB needs.

alecflett@ - please take a look

[Alec Flett]

Comment on attachment 171957 [details]
Patch

lgtm
(makes me wonder if you can now have 'const LevelDBSnapshot m_snapshot;' - not a big deal though)

[Joshua Bell]

Created attachment 172387 [details]
Patch

[Joshua Bell]

(In reply to comment #14)
> (makes me wonder if you can now have 'const LevelDBSnapshot m_snapshot;' - not a big deal though)

Yes, thanks. Added const qualifier.

[Joshua Bell]

tony@ - r?

[Joshua Bell]

abarth@ - if you're bored (heh), could you take a look?

[Adam Barth]

Comment on attachment 172387 [details]
Patch

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

> Source/WebCore/platform/leveldb/LevelDBDatabase.h:56
> +    LevelDBSnapshot(LevelDBDatabase*);

explicit

[Adam Barth]

Looks like Tony has been reviewing most of the changes to this code.  I think he gets back from vacation tomorrow.  If you're in a rush, I can rubber-stamp this change, but I'm not super familiar with the code.

[Joshua Bell]

(In reply to comment #20)
> Looks like Tony has been reviewing most of the changes to this code.  I think he gets back from vacation tomorrow.  If you're in a rush, I can rubber-stamp this change, but I'm not super familiar with the code.

No rush on this one, thanks for looking.

[Joshua Bell]

Created attachment 172833 [details]
Patch

[Tony Chang]

Comment on attachment 172833 [details]
Patch

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

> Source/WebCore/platform/leveldb/LevelDBDatabase.h:46
> +class LevelDBDatabase;
>  class LevelDBComparator;

Nit: sort

[Joshua Bell]

Created attachment 173107 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173107 [details]
Patch for landing

Rejecting attachment 173107 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Core/platform/leveldb/LevelDBTransaction.h
patching file Source/WebKit/chromium/tests/LevelDBTest.cpp
patching file LayoutTests/ChangeLog
Hunk #1 succeeded at 94 (offset 57 lines).
patching file LayoutTests/platform/chromium/TestExpectations
Hunk #1 FAILED at 4190.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/14758883

[Joshua Bell]

> 1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/chromium/TestExpectations.rej

*sigh* Rebased onto a dirty tree and didn't notice it in the diff. Bleah.

[Joshua Bell]

Created attachment 173110 [details]
Patch for landing

[Joshua Bell]

Comment on attachment 173110 [details]
Patch for landing

Ugh, no, it wasn't in my local diff. One more time.

[Joshua Bell]

Created attachment 173112 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 173112 [details]
Patch for landing

Clearing flags on attachment: 173112

Committed r133963: <http://trac.webkit.org/changeset/133963>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.