Bug 109993 - IndexedDB: Limit LevelDB's max open files
Summary: IndexedDB: Limit LevelDB's max open files
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: David Grogan
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-15 17:44 PST by David Grogan
Modified: 2013-02-20 15:02 PST (History)
5 users (show)

See Also:


Attachments
Patch (3.38 KB, patch)
2013-02-15 17:48 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (1.85 KB, patch)
2013-02-19 17:31 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (1.86 KB, patch)
2013-02-20 11:08 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (1.87 KB, patch)
2013-02-20 11:19 PST, David Grogan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2013-02-15 17:44:32 PST
IndexedDB: Limit LevelDB's max open files
Comment 1 David Grogan 2013-02-15 17:48:29 PST
Created attachment 188677 [details]
Patch
Comment 2 David Grogan 2013-02-16 17:00:44 PST
More details on the chromium bug:
http://code.google.com/p/chromium/issues/detail?id=176553
Comment 3 Joshua Bell 2013-02-19 12:28:27 PST
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?
Comment 4 David Grogan 2013-02-19 16:53:31 PST
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.
Comment 5 David Grogan 2013-02-19 17:31:53 PST
Created attachment 189212 [details]
Patch
Comment 6 David Grogan 2013-02-20 11:08:19 PST
Created attachment 189342 [details]
Patch
Comment 7 David Grogan 2013-02-20 11:09:08 PST
Josh, any more comments before I add Tony?
Comment 8 Joshua Bell 2013-02-20 11:16:59 PST
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.
Comment 9 David Grogan 2013-02-20 11:19:30 PST
Created attachment 189345 [details]
Patch
Comment 10 David Grogan 2013-02-20 11:20:04 PST
Tony, could you review this?
Comment 11 WebKit Review Bot 2013-02-20 15:02:27 PST
Comment on attachment 189345 [details]
Patch

Clearing flags on attachment: 189345

Committed r143512: <http://trac.webkit.org/changeset/143512>
Comment 12 WebKit Review Bot 2013-02-20 15:02:31 PST
All reviewed patches have been landed.  Closing bug.