Bug 68903 - IndexedDB: Use LevelDB also for in-memory databases
Summary: IndexedDB: Use LevelDB also for in-memory databases
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Hans Wennborg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-09-27 07:46 PDT by Hans Wennborg
Modified: 2011-09-29 05:28 PDT (History)
3 users (show)

See Also:


Attachments
Patch (14.54 KB, patch)
2011-09-27 08:06 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff
Patch (14.98 KB, patch)
2011-09-28 06:35 PDT, Hans Wennborg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-09-27 07:46:58 PDT
IndexedDB: Use LevelDB also for in-memory databases
Comment 1 Hans Wennborg 2011-09-27 08:06:43 PDT
Created attachment 108850 [details]
Patch
Comment 2 David Grogan 2011-09-27 23:29:57 PDT
Comment on attachment 108850 [details]
Patch

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

LGTM

I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation.  Though I suppose our browser tests would pick up some of those.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> +    const String name = path.isNull() ? "/in-mem-db" : path;

"in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?

> Source/WebCore/platform/leveldb/LevelDBDatabase.h:66
> +    OwnPtr<leveldb::Comparator> m_comparatorAdapter;

Did you move this to match the order that the destructor deletes these?
Comment 3 Hans Wennborg 2011-09-28 03:31:25 PDT
(In reply to comment #2)
> (From update of attachment 108850 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=108850&action=review
> 
> LGTM
> 
> I'm a little concerned that making our tests run in memory might mask problems that occur as a result of something in the default env implementation.  Though I suppose our browser tests would pick up some of those.

I'm not too concerned about that. Like you said, the browser tests should cover it.

We should look into running leveldb's own test suite as part of chromium's tests, but that's out-of-scope for this bug.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> > +    const String name = path.isNull() ? "/in-mem-db" : path;
> 
> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?

No magic, just a dummy name.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.h:66
> > +    OwnPtr<leveldb::Comparator> m_comparatorAdapter;
> 
> Did you move this to match the order that the destructor deletes these?

Yes, even though it doesn't matter because we clear them in the right order in the destructor, it bothered me to see them in the wrong order here.
Comment 4 Steve Block 2011-09-28 05:03:32 PDT
Comment on attachment 108850 [details]
Patch

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

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110
> +    if (env)

Do you need this check? What's the default value of Options::env ?

>>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
>>> +    const String name = path.isNull() ? "/in-mem-db" : path;
>> 
>> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?
> 
> No magic, just a dummy name.

This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory.

On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150
> +    OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase);

Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think.

> Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015
> +    String path = ":memory:";

Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment.
Comment 5 Hans Wennborg 2011-09-28 06:35:06 PDT
(In reply to comment #4)
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:110
> > +    if (env)
> 
> Do you need this check? What's the default value of Options::env ?

The default of Options::env is Env::Default(). I guess I could just pass that in explicitly.

> >>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:113
> >>> +    const String name = path.isNull() ? "/in-mem-db" : path;
> >> 
> >> "in-mem-db" isn't magic, right?  Nothing in leveldb looks for that string, it's just a dummy?
> > 
> > No magic, just a dummy name.
> 
> This seems a little odd. It looks like leveldb uses Options::env to determine whether the DB should be in-memory. But here we make a decision based on path. This means we're making an implicit assumption about the values of env and path passed to this method. Can't we test env for in-memory-ness, then ASSERT(!path.isNull()) if it's not? Also, add a comment that this method ignores path if the DB is in-memory.

You're right, that is weird. I think we can just scrap this line actually.

> 
> On a related note, why do you assign a value to path for an in memory DB? If leveldb::DB::Open() ignores it, why can't you pass NULL? At least add a comment.

Technically, it's not really ignored (it will be used as a path in the in-memory filesystem that the Env implements), but passing in an empty path works, so let's do that.

> 
> > Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:150
> > +    OwnPtr<LevelDBDatabase> result = adoptPtr(new LevelDBDatabase);
> 
> Is there any point in using an OwnPtr here? There's only one code path and we always release it. Same applies to existing code in open() I think.

After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and sticking things in OwnPtr was the way to do things?

> 
> > Source/WebCore/storage/IDBSQLiteBackingStore.cpp:1015
> > +    String path = ":memory:";
> 
> Is this a magic token for SQLite? If so, I'm surprised there isn't an SQLite constant for it. Probably warrants a comment.

Yes, that's a magic token for SQLite. I'll add a comment.
Comment 6 Hans Wennborg 2011-09-28 06:35:30 PDT
Created attachment 109018 [details]
Patch
Comment 7 Hans Wennborg 2011-09-29 02:48:34 PDT
Steve, would you like to take another look?
Comment 8 Steve Block 2011-09-29 03:14:08 PDT
Comment on attachment 109018 [details]
Patch

> After https://bugs.webkit.org/show_bug.cgi?id=59656, I got the impression that "naked new" was bad, and
> sticking things in OwnPtr was the way to do things?
Yes, you're right.
Comment 9 WebKit Review Bot 2011-09-29 05:28:20 PDT
Comment on attachment 109018 [details]
Patch

Clearing flags on attachment: 109018

Committed r96322: <http://trac.webkit.org/changeset/96322>
Comment 10 WebKit Review Bot 2011-09-29 05:28:25 PDT
All reviewed patches have been landed.  Closing bug.