Bug 109993 – IndexedDB: Limit LevelDB's max open files

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

[David Grogan]

IndexedDB: Limit LevelDB's max open files

[David Grogan]

Created attachment 188677 [details]
Patch

[David Grogan]

More details on the chromium bug:
http://code.google.com/p/chromium/issues/detail?id=176553

[Joshua Bell]

Comment on attachment 188677 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        IndexedDB: Limit LevelDB's max open files

Probably want a "why" explanation in the ChangeLog.

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:136
> +static int calculateMaxOpenFiles()

Each LevelDB instance (i.e. each origin) will be competing for the same pool of file descriptors, won't it? This approach (tell leveldb it can use RLIMIT_NOFILE - 250) doesn't seem like it'll work if there are multiple origins competing for a small number of descriptors.

Maybe we want to pin this as low as possible (i.e. 20) until we learn otherwise?

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:152
> +#if OS(UNIX)

|| OS(DARWIN) ?

It also seems like this "roughly how many file descriptors are available?" query should be factored out and a chromium-specific implementation could do the "limit - 250" part. (But see above re: multiple origins.)

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:169
> +        HistogramSupport::histogramEnumeration(histogramName, Error, NumEntries);

Do we want a histogram for other OSs where the default is used?

[David Grogan]

Comment on attachment 188677 [details]
Patch

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

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:136
>> +static int calculateMaxOpenFiles()
> 
> Each LevelDB instance (i.e. each origin) will be competing for the same pool of file descriptors, won't it? This approach (tell leveldb it can use RLIMIT_NOFILE - 250) doesn't seem like it'll work if there are multiple origins competing for a small number of descriptors.
> 
> Maybe we want to pin this as low as possible (i.e. 20) until we learn otherwise?

You're right that each origin will be competing for the same pool, but this method is still an improvement in the common problematic case of one origin having a huge IDB. Given that we only have one confirmed report of IDB using all the FDs, and that was from one origin, I feel like the chance of two origins with huge IDBs is pretty small. At least until there is wider IDB adoption.

But the simplicity of always using 20 is alluring. So my plan is to hardcode 20 in this patch, commit to trunk and merge to m26. Then I'm going to do some local perf testing with a database large enough that max_open_files could make a difference. If there is a difference we can then figure out what to do about the perf hit.

Hardcoding 20 makes the rest of the comments academic, at least for now.

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:152
>> +#if OS(UNIX)
> 
> || OS(DARWIN) ?
> 
> It also seems like this "roughly how many file descriptors are available?" query should be factored out and a chromium-specific implementation could do the "limit - 250" part. (But see above re: multiple origins.)

Darwin is covered by UNIX: https://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WTF/wtf/Platform.h&exact_package=chromium&q=define.*UNIX%20file:third%20file:webkit&type=cs&l=415

>> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:169
>> +        HistogramSupport::histogramEnumeration(histogramName, Error, NumEntries);
> 
> Do we want a histogram for other OSs where the default is used?

So, other OSs means windows. And you're right, it couldn't hurt.

[David Grogan]

Created attachment 189212 [details]
Patch

[David Grogan]

Created attachment 189342 [details]
Patch

[David Grogan]

Josh, any more comments before I add Tony?

[Joshua Bell]

Comment on attachment 189342 [details]
Patch

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

lgtm

> Source/WebCore/platform/leveldb/LevelDBDatabase.cpp:136
> +    // LevelDB clamps max_open_files to 20.

Maybe indicate that this is the *minimum* enforced by leveldb.

[David Grogan]

Created attachment 189345 [details]
Patch

[David Grogan]

Tony, could you review this?

[WebKit Review Bot]

Comment on attachment 189345 [details]
Patch

Clearing flags on attachment: 189345

Committed r143512: <http://trac.webkit.org/changeset/143512>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.