Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
27242002:
Use file pool to handle running out of file descriptors. (Closed)
|
Created:
7 years, 2 months ago by Bob Nystrom Modified:
7 years, 2 months ago CC:
reviews_dartlang.org, Siggi Cherem (dart-lang), Jennifer Messerly Visibility:
Public. |
DescriptionUse file pool to handle running out of file descriptors.
BUG=https://code.google.com/p/dart/issues/detail?id=13752
Committed: https://code.google.com/p/dart/source/detail?r=28702
Patch Set 1 #Patch Set 2 : Rename test. #
Total comments: 8
Patch Set 3 : Rebase. #
Total comments: 4
Patch Set 4 : Lower number of files a bit. #Patch Set 5 : Revise to work with readAsString() too. (Thanks Kevin!) #
Total comments: 30
Patch Set 6 : Revise. #
Messages
Total messages: 13 (0 generated)
I think this alleviates the issue with running out of file descriptors when you're reading file assets from barback. On my mac, this test fails with the previous barback code for any i > 256. With the FilePool used, the test passes all the way up to 5,000+. (Too much higher than that, and the VM runs out of memory and dies.) Friends on the CC line, can you try this patch out on your use cases and see if it helps there? Thanks!
drive by comments... https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; barback.src.file_pool? Or did you mean to make this library public (not in "src")? I think we may need to use this from Polymer too. Siggi would know for sure. In that case, we'd ideally be using the same singleton instance as barback.
https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; On 2013/10/14 21:00:08, John Messerly wrote: > barback.src.file_pool? > > Or did you mean to make this library public (not in "src")? I think we may need > to use this from Polymer too. Siggi would know for sure. In that case, we'd > ideally be using the same singleton instance as barback. Good point. Making it visible would simplify what we do. Alternatively, when creating copies of files I could write: new Asset.fromPath(path).read().pipe(new File(output).openWrite()) https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:27: Stream<List<int>> openRead(File file) => new _FileReader(this, file).stream; if we make it public, can we also add openWrite?
https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; On 2013/10/14 21:00:08, John Messerly wrote: > barback.src.file_pool? We don't use ".src" in other library names in Barback. I'm not sure if we should or not. <shrug> > Or did you mean to make this library public (not in "src")? I think we may need > to use this from Polymer too. Siggi would know for sure. In that case, we'd > ideally be using the same singleton instance as barback. No, I didn't intend it to be public. Barback doesn't expose any file-system-level APIs. My intent here is that if you're getting the file descriptor error because you're reading from barback file assets, this should just invisibly solve the problem for you. https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; On 2013/10/14 21:14:28, Siggi Cherem (dart-lang) wrote: > On 2013/10/14 21:00:08, John Messerly wrote: > > barback.src.file_pool? > > > > Or did you mean to make this library public (not in "src")? I think we may > need > > to use this from Polymer too. Siggi would know for sure. In that case, we'd > > ideally be using the same singleton instance as barback. I'm trying to avoid exposing this functionality as a public API in barback. Barback's job definitely isn't just to be an IO wrapper. :) > Good point. Making it visible would simplify what we do. Alternatively, when > creating copies of files I could write: > > new Asset.fromPath(path).read().pipe(new File(output).openWrite()) This patch may help here even without you touching the API directly. Eventually those reads will fail because you have too many files open (for read and write). When some of the previous ones finish, both readers and writers will close. Barback will see the reads close and try again. If you'd like to give this a shot and see if it helps, I'd definitely appreciate it. At least in the short term, I'm trying to keep this solution an implementation detail of barback. If we have to make an actual public API so that other code can reuse this solution, we can investigate that, but that's a lot more work. https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:27: Stream<List<int>> openRead(File file) => new _FileReader(this, file).stream; On 2013/10/14 21:14:28, Siggi Cherem (dart-lang) wrote: > if we make it public, can we also add openWrite? See above. Trying to not make it public. :)
https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; trivial detail but: regarding library name -- shouldn't it be consistent with our hoped-for-import name? e.g. "import barback.src.file_pool;" maybe someday?
https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/3001/pkg/barback/lib/src/file_p... pkg/barback/lib/src/file_pool.dart:5: library barback.file_pool; On 2013/10/14 21:30:42, John Messerly wrote: > trivial detail but: regarding library name -- shouldn't it be consistent with > our hoped-for-import name? e.g. "import barback.src.file_pool;" maybe someday? Hmm, good point. I'm up for changing all of these library names at some point, but I'd rather keep them consistent and I don't want to change them all in this patch.
DBC https://codereview.chromium.org/27242002/diff/10001/pkg/barback/lib/src/file_... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/10001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:110: if (exception is! FileException || exception.osError.errorCode != 24) { Using up all file-descriptors would also block e.g. incoming connections. Is that acceptable in barback? Would it be better to not retry on fail, but keep the maximum # of open file-streams to e.g. 24?. https://codereview.chromium.org/27242002/diff/10001/pkg/barback/test/too_many... File pkg/barback/test/too_many_open_files_test.dart (right): https://codereview.chromium.org/27242002/diff/10001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:31: for (var i = 0; i < 5000; i++) { Currently we don't tract external memory, but once we start doing so, this could lead to OOM: 5000 * 64k = 327MB Alive memory, at the bare minimum. Can this be done using maybe 100b files?
https://codereview.chromium.org/27242002/diff/10001/pkg/barback/lib/src/file_... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/10001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:110: if (exception is! FileException || exception.osError.errorCode != 24) { On 2013/10/15 10:48:26, Anders Johnsen wrote: > Using up all file-descriptors would also block e.g. incoming connections. Is > that acceptable in barback? Would it be better to not retry on fail, but keep > the maximum # of open file-streams to e.g. 24?. I'm hesitant to add a more or less arbitrary limit here. Relying on the error lets this adapt to whatever the user's system supports. In practice, I think this issue mainly occurs when the system is reading and bouncing a large pile of assets to disk in one lump. It's a transient burst problem. If this code copes with that, I think it may be good enough. However, your point is valid. When this kicks in, you can still run out of other resources. It may be smarter to have a lower cap. I'll talk it over with Nathan and see what he thinks. https://codereview.chromium.org/27242002/diff/10001/pkg/barback/test/too_many... File pkg/barback/test/too_many_open_files_test.dart (right): https://codereview.chromium.org/27242002/diff/10001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:31: for (var i = 0; i < 5000; i++) { On 2013/10/15 10:48:26, Anders Johnsen wrote: > Currently we don't tract external memory, but once we start doing so, this could > lead to OOM: > > 5000 * 64k = 327MB > > Alive memory, at the bare minimum. > > Can this be done using maybe 100b files? Yeah, 5k is a bit gratuitous. I needed that to hit the problem on my machine because, somehow, my ulimit got bumped up for this. But it seems to be back to the normal (but sad) 256. Lowered this some.
Almost. For my original issue w/ Polymer, we also need to intercept calls to readAsString Here's my impl -> just piggybacking on your CL with the addition of readAsString copied mostly from file_impl https://codereview.chromium.org/26959010
Good call! I've updated the patch with your change (more or less) and added a test.
https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/asset... File pkg/barback/lib/src/asset.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/asset... pkg/barback/lib/src/asset.dart:107: final File _file; I don't understand why we're storing a File object here. I feel like barback should adopt the pub philosophy of only dealing in paths internally, and only converting to File at the last minute. FilePool should also follow this pattern. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:20: // TODO(rnystrom): Should we cap this to some maximum size? I don't think so. Let the "too many files" error be the limit. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:21: final _pendingListens = new Queue<_FileReader>(); Document this. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:26: /// open, this will wait for a previously opened to file to be closed and "opened to file" -> "opened file" https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:51: /// Tries to reopen the next pending open if there are any. "reopen" -> "restart" Also a little confusing that you call it an "open" here and a "listen" in the code. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:72: /// The controller for the wrapped stream. "wrapped" here isn't accurate; the wrapped stream is [_fileStream]. Maybe "wrapping stream"? https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:79: Timer _timer; Document this. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:93: _controller = new StreamController<List<int>>(onListen: _listen, Why isn't this being initialized in the constructor? [stream] is always called immediately after the FileReader is created anyway. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:127: assert(_exception == null); This doesn't seem right. It's definitely possible for the same FileReader to get multiple "too many files" errors. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:133: if (exception is! FileException || exception.osError.errorCode != 24) { We should figure out what error code the exception will produce on Windows and check for that here as well. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:144: // TODO(rnystrom): How long should this delay be? The chance of a deadlock here is extremely small, so I'm happy with making the timeout very long. I don't think this timeout strategy is quite correct, though. Suppose there are many readers queued up and being slowly dequeued. We're now under the file limit, so each one that's dequeued is working correctly. But the dequeuing is slow enough that one of the last readers to be added can still time out, even though there's no deadlock. This could be solved by putting the timer on the FilePool, and resetting it whenever a file is successfully opened. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:172: _stackTrace = null; I don't know how I feel about nulling out all the mutable instance variables whenever we know a class's lifecycle is done. On one hand, it's safe because the lifecycle is done, and there's some chance it catches a bug. On the other hand, it's probably unnecessary, since this will get garbage collected anyway. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:173: Nit: extra newline. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... File pkg/barback/test/too_many_open_files_test.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:19: runOnManyFiles(Future assetHandler(Asset asset)) { It feels like this should be defined at the top level or in utils. I find it confusing to read in main(), where I just expect test cases. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:59: }); It would be cleaner just to return asset.read().toList().
Thanks! https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/asset... File pkg/barback/lib/src/asset.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/asset... pkg/barback/lib/src/asset.dart:107: final File _file; On 2013/10/16 00:16:01, nweiz wrote: > I don't understand why we're storing a File object here. I feel like barback > should adopt the pub philosophy of only dealing in paths internally, and only > converting to File at the last minute. > > FilePool should also follow this pattern. FileAsset already has the File object, so this was more consistent. I'm up for changing this, but not in this patch. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... File pkg/barback/lib/src/file_pool.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:20: // TODO(rnystrom): Should we cap this to some maximum size? On 2013/10/16 00:16:01, nweiz wrote: > I don't think so. Let the "too many files" error be the limit. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:21: final _pendingListens = new Queue<_FileReader>(); On 2013/10/16 00:16:01, nweiz wrote: > Document this. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:26: /// open, this will wait for a previously opened to file to be closed and On 2013/10/16 00:16:01, nweiz wrote: > "opened to file" -> "opened file" Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:51: /// Tries to reopen the next pending open if there are any. On 2013/10/16 00:16:01, nweiz wrote: > "reopen" -> "restart" > > Also a little confusing that you call it an "open" here and a "listen" in the > code. Fixed. The original implementation retried on open(), not listen(). https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:72: /// The controller for the wrapped stream. On 2013/10/16 00:16:01, nweiz wrote: > "wrapped" here isn't accurate; the wrapped stream is [_fileStream]. Maybe > "wrapping stream"? "stream wrapper". https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:79: Timer _timer; On 2013/10/16 00:16:01, nweiz wrote: > Document this. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:93: _controller = new StreamController<List<int>>(onListen: _listen, On 2013/10/16 00:16:01, nweiz wrote: > Why isn't this being initialized in the constructor? [stream] is always called > immediately after the FileReader is created anyway. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:127: assert(_exception == null); On 2013/10/16 00:16:01, nweiz wrote: > This doesn't seem right. It's definitely possible for the same FileReader to get > multiple "too many files" errors. Changed this to clear _exception in _listen(). Since the subscription is cancelled on an error, that should ensure that _exception will be null when we get here. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:133: if (exception is! FileException || exception.osError.errorCode != 24) { On 2013/10/16 00:16:01, nweiz wrote: > We should figure out what error code the exception will produce on Windows and > check for that here as well. I did a little checking and I couldn't find much info on this. Once it lands, we can see how it goes on the bots. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:144: // TODO(rnystrom): How long should this delay be? On 2013/10/16 00:16:01, nweiz wrote: > The chance of a deadlock here is extremely small, so I'm happy with making the > timeout very long. I don't think this timeout strategy is quite correct, though. > Suppose there are many readers queued up and being slowly dequeued. We're now > under the file limit, so each one that's dequeued is working correctly. But the > dequeuing is slow enough that one of the last readers to be added can still time > out, even though there's no deadlock. This could be solved by putting the timer > on the FilePool, and resetting it whenever a file is successfully opened. Good call. Increased the timeout and added a long TODO about this since I don't have time to get to it today. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:172: _stackTrace = null; On 2013/10/16 00:16:01, nweiz wrote: > I don't know how I feel about nulling out all the mutable instance variables > whenever we know a class's lifecycle is done. On one hand, it's safe because the > lifecycle is done, and there's some chance it catches a bug. On the other hand, > it's probably unnecessary, since this will get garbage collected anyway. Yeah, I'm mainly doing it as an ad-hoc state machine so I can assert on them in the various methods and sanity check what's going on. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/lib/src/file_... pkg/barback/lib/src/file_pool.dart:173: On 2013/10/16 00:16:01, nweiz wrote: > Nit: extra newline. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... File pkg/barback/test/too_many_open_files_test.dart (right): https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:19: runOnManyFiles(Future assetHandler(Asset asset)) { On 2013/10/16 00:16:01, nweiz wrote: > It feels like this should be defined at the top level or in utils. I find it > confusing to read in main(), where I just expect test cases. Done. https://codereview.chromium.org/27242002/diff/22001/pkg/barback/test/too_many... pkg/barback/test/too_many_open_files_test.dart:59: }); On 2013/10/16 00:16:01, nweiz wrote: > It would be cleaner just to return asset.read().toList(). Indeed! Done.
Message was sent while issue was closed.
Committed patchset #6 manually as r28702 (presubmit successful). |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
