IndexedDB: Limit LevelDB's max open files
Created attachment 188677 [details] Patch
More details on the chromium bug: http://code.google.com/p/chromium/issues/detail?id=176553
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 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.
Created attachment 189212 [details] Patch
Created attachment 189342 [details] Patch
Josh, any more comments before I add Tony?
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.
Created attachment 189345 [details] Patch
Tony, could you review this?
Comment on attachment 189345 [details] Patch Clearing flags on attachment: 189345 Committed r143512: <http://trac.webkit.org/changeset/143512>
All reviewed patches have been landed. Closing bug.