WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
68903
IndexedDB: Use LevelDB also for in-memory databases
https://bugs.webkit.org/show_bug.cgi?id=68903
Summary
IndexedDB: Use LevelDB also for in-memory databases
Hans Wennborg
Reported
2011-09-27 07:46:58 PDT
IndexedDB: Use LevelDB also for in-memory databases
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Hans Wennborg
Comment 1
2011-09-27 08:06:43 PDT
Created
attachment 108850
[details]
Patch
David Grogan
Comment 2
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?
Hans Wennborg
Comment 3
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.
Steve Block
Comment 4
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.
Hans Wennborg
Comment 5
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.
Hans Wennborg
Comment 6
2011-09-28 06:35:30 PDT
Created
attachment 109018
[details]
Patch
Hans Wennborg
Comment 7
2011-09-29 02:48:34 PDT
Steve, would you like to take another look?
Steve Block
Comment 8
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.
WebKit Review Bot
Comment 9
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
>
WebKit Review Bot
Comment 10
2011-09-29 05:28:25 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug