Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
318563002:
[fsp] Introduce BufferingFileStreamReader to read files in bigger chunks. (Closed)
|
Created:
6 years, 6 months ago by mtomasz Modified:
6 years, 5 months ago CC:
chromium-reviews, nkostylev+watch_chromium.org, tzik, nhiroki, oshima+watch_chromium.org, stevenjb+watch_chromium.org, kinuko+fileapi, davemoore+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[fsp] Introduce BufferingFileStreamReader to read files in bigger chunks.
Default chunk size is 32KB and it is too small for provided file systems, which
require IPC calls for each chunk. Such small chunks cause the IPC overhead to
slow down the reading process drastically.
Moreover, since there is no streaming support in XMLHttpRequest, returning
data in chunks in providing extensions, requires making separate HTTP requests
with appriopriate Range header. That introduces another overhead.
This patch introduces a wrapper on a FileStreamReader, which converts 32KB
chunk requests to 512KB chunk requests to the underlying file stream reader.
As a result, the overhead of IPC and HTTP is decreased significantly.
TEST=unit_tests: *FileSystemProvider*BufferingFileStreamReader*
BUG=248427
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282290
Patch Set 1 #Patch Set 2 : Fixed a test. #
Total comments: 46
Patch Set 3 : Fixed. #
Total comments: 6
Patch Set 4 : Fixed. #
Total comments: 13
Patch Set 5 : Fixed. #Patch Set 6 : Removed support for sync results + added support for larger buffers. #Patch Set 7 : #
Total comments: 2
Patch Set 8 : Fixed naming. #
Total comments: 10
Patch Set 9 : Addressed comments. #Messages
Total messages: 39 (0 generated)
@hashimoto: PTAL. Thanks.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc:24: const int kBufferSize = 512 * 1024; // 512KB. nit: kBufferSize sounds too general. How about something like kReaderBufferSize? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:55: buffer_length <= buffered_bytes_ ? buffer_length : buffered_bytes_; nit: std::min(buffer_length, buffered_bytes_)? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:77: callback.Run(result); Return the result as a return value, or post a task to run the callback. Returning ERR_IO_PENDING after running the callback with the result may cause problems in user code. (e.g. BlobURLRequestJob::ReadFileItem() may call SetStatus() with net::URLRequestStatus::IO_PENDING after the result was returning through the callback https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/blo...) https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:12: #include "net/base/io_buffer.h" nit: forward declaration is enough. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:16: class AsyncFileUtil; nit: unused forward declaration. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:28: explicit BufferingFileStreamReader( nit: "explicit" unneeded. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; How about using DrainableIOBuffer? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:49: // Registers eresults returned asynchronously by the buffering strem reader, nit: s/eresults/results/? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } No need to have this method? I don't see any danger in using base::Unretained as Logger always lives longer than the reader. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); How about using TestCompletionCallback? The same goes for other places passing Logger methods as callbacks. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:12: #include "net/base/io_buffer.h" nit: forward declaration is enough.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/backend_delegate.cc:24: const int kBufferSize = 512 * 1024; // 512KB. On 2014/06/05 03:18:36, hashimoto wrote: > nit: kBufferSize sounds too general. How about something like kReaderBufferSize? Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:55: buffer_length <= buffered_bytes_ ? buffer_length : buffered_bytes_; On 2014/06/05 03:18:36, hashimoto wrote: > nit: std::min(buffer_length, buffered_bytes_)? Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:77: callback.Run(result); On 2014/06/05 03:18:36, hashimoto wrote: > Return the result as a return value, or post a task to run the callback. > Returning ERR_IO_PENDING after running the callback with the result may cause > problems in user code. > (e.g. BlobURLRequestJob::ReadFileItem() may call SetStatus() with > net::URLRequestStatus::IO_PENDING after the result was returning through the > callback > https://code.google.com/p/chromium/codesearch#chromium/src/webkit/browser/blo...) Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:12: #include "net/base/io_buffer.h" On 2014/06/05 03:18:36, hashimoto wrote: > nit: forward declaration is enough. Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:16: class AsyncFileUtil; On 2014/06/05 03:18:36, hashimoto wrote: > nit: unused forward declaration. Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:28: explicit BufferingFileStreamReader( On 2014/06/05 03:18:36, hashimoto wrote: > nit: "explicit" unneeded. Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; On 2014/06/05 03:18:36, hashimoto wrote: > How about using DrainableIOBuffer? Net::DrainableIOBuffer works well if the buffer is always fully filled out. In our case, the preloading_buffer_ is often not. So, we would have to recreate the net::DrainableIOBuffer every time it is filled out. Otherwise, DrainableIOBuffer::BytesRemaining() would return an incorrect value for not fully-filled out buffers. I'm not very sure if it would be better here, since we would need two members: scoped_refptr<net::IOBuffer> internal_buffer; // internal buffer for current_buffer. scoped_refptr<net::DrainableIOBuffer> current_buffer; // recreated every Preload() call. WDYT? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:49: // Registers eresults returned asynchronously by the buffering strem reader, On 2014/06/05 03:18:36, hashimoto wrote: > nit: s/eresults/results/? Done. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/05 03:18:36, hashimoto wrote: > No need to have this method? > I don't see any danger in using base::Unretained as Logger always lives longer > than the reader. We are passing it to the BufferingFileStreamReader, which can call it back with a PostTask. Using Unretained wouldn't crash, only because we (1) call RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates fake threads backed up with the same message loop. If we had real threads, then this could crash (IIUC). I think having a weak ptr is just a safe choice, since its very simple and will always work fine. It doesn't add much overhead either. WDYT? https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/05 03:18:36, hashimoto wrote: > How about using TestCompletionCallback? > The same goes for other places passing Logger methods as callbacks. With TestCompletionCallback we can't control how many times the callback has been called. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:12: #include "net/base/io_buffer.h" On 2014/06/05 03:18:36, hashimoto wrote: > nit: forward declaration is enough. Done.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; On 2014/06/05 09:22:27, mtomasz wrote: > On 2014/06/05 03:18:36, hashimoto wrote: > > How about using DrainableIOBuffer? > > Net::DrainableIOBuffer works well if the buffer is always fully filled out. In > our case, the preloading_buffer_ is often not. So, we would have to recreate the > net::DrainableIOBuffer every time it is filled out. Otherwise, > DrainableIOBuffer::BytesRemaining() would return an incorrect value for not > fully-filled out buffers. I'm not very sure if it would be better here, since we > would need two members: > > scoped_refptr<net::IOBuffer> internal_buffer; // internal buffer for > current_buffer. > scoped_refptr<net::DrainableIOBuffer> current_buffer; // recreated every > Preload() call. > > WDYT? I meant by using DrailableIOBuffer there is no need to have |preloading_buffer_offset_| and manually offset the pointer. You can use SetOffset() and DidConsume() to do the same thing as you do by changing the value of preloading_buffer_offset_. You can reuse the same DrainableIOBuffer by resetting the offset with SetOffset(0). https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/05 09:22:27, mtomasz wrote: > On 2014/06/05 03:18:36, hashimoto wrote: > > No need to have this method? > > I don't see any danger in using base::Unretained as Logger always lives longer > > than the reader. > > We are passing it to the BufferingFileStreamReader, which can call it back with > a PostTask. Using Unretained wouldn't crash, only because we (1) call > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates fake > threads backed up with the same message loop. If we had real threads, then this > could crash (IIUC). I think having a weak ptr is just a safe choice, since its > very simple and will always work fine. It doesn't add much overhead either. > WDYT? I think it's OK to take advantage of the single threaded test and using WeakPtr looks like a kind of overkill. Also, we can even stop Logger for recording results like below. (the only exception is recording the method calls for the internal reader) void PushResultToVector(std::vector<int>* out, int result) { out->push_back(result); } TEST(...) { std::vector<int> results; reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, &results)); } Anyways, it's up to you. https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:100: FROM_HERE, base::Bind(callback, copy_result)); No need to post task here as OnPreloadCompleted() is never called synchronously. (and callback is run directly when result < 0) https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:27: explicit BufferingFileStreamReader( Still seeing unneeded "explicit" https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:16: class AsyncFileUtil; nit: Unused forward declaration?
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; On 2014/06/05 10:07:10, hashimoto wrote: > On 2014/06/05 09:22:27, mtomasz wrote: > > On 2014/06/05 03:18:36, hashimoto wrote: > > > How about using DrainableIOBuffer? > > > > Net::DrainableIOBuffer works well if the buffer is always fully filled out. In > > our case, the preloading_buffer_ is often not. So, we would have to recreate > the > > net::DrainableIOBuffer every time it is filled out. Otherwise, > > DrainableIOBuffer::BytesRemaining() would return an incorrect value for not > > fully-filled out buffers. I'm not very sure if it would be better here, since > we > > would need two members: > > > > scoped_refptr<net::IOBuffer> internal_buffer; // internal buffer for > > current_buffer. > > scoped_refptr<net::DrainableIOBuffer> current_buffer; // recreated every > > Preload() call. > > > > WDYT? > > I meant by using DrailableIOBuffer there is no need to have > |preloading_buffer_offset_| and manually offset the pointer. > You can use SetOffset() and DidConsume() to do the same thing as you do by > changing the value of preloading_buffer_offset_. > > You can reuse the same DrainableIOBuffer by resetting the offset with > SetOffset(0). This won't work since the buffer is not always full. Eg. if there is only 10 bytes in the buffer of size 50, then BytesRemaining() will return 50, once it should return 10.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; On 2014/06/05 11:27:43, mtomasz wrote: > On 2014/06/05 10:07:10, hashimoto wrote: > > On 2014/06/05 09:22:27, mtomasz wrote: > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > How about using DrainableIOBuffer? > > > > > > Net::DrainableIOBuffer works well if the buffer is always fully filled out. > In > > > our case, the preloading_buffer_ is often not. So, we would have to recreate > > the > > > net::DrainableIOBuffer every time it is filled out. Otherwise, > > > DrainableIOBuffer::BytesRemaining() would return an incorrect value for not > > > fully-filled out buffers. I'm not very sure if it would be better here, > since > > we > > > would need two members: > > > > > > scoped_refptr<net::IOBuffer> internal_buffer; // internal buffer for > > > current_buffer. > > > scoped_refptr<net::DrainableIOBuffer> current_buffer; // recreated every > > > Preload() call. > > > > > > WDYT? > > > > I meant by using DrailableIOBuffer there is no need to have > > |preloading_buffer_offset_| and manually offset the pointer. > > You can use SetOffset() and DidConsume() to do the same thing as you do by > > changing the value of preloading_buffer_offset_. > > > > You can reuse the same DrainableIOBuffer by resetting the offset with > > SetOffset(0). > > This won't work since the buffer is not always full. > Eg. if there is only 10 bytes in the buffer of size 50, then BytesRemaining() > will return 50, once it should return 10. You don't need BytesRemaining(). What I suggested was replacing: - "preloading_buffer_offset_ += read_bytes;" with DidConsume(read_bytes) - "preloading_buffer_offset_ = 0;" with SetOffset(0) so that you can manage the offset with the buffer itself. This is just a suggestion. You can use whatever you think reasonable.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:58: scoped_refptr<net::IOBuffer> preloading_buffer_; On 2014/06/05 12:31:09, hashimoto wrote: > On 2014/06/05 11:27:43, mtomasz wrote: > > On 2014/06/05 10:07:10, hashimoto wrote: > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > How about using DrainableIOBuffer? > > > > > > > > Net::DrainableIOBuffer works well if the buffer is always fully filled > out. > > In > > > > our case, the preloading_buffer_ is often not. So, we would have to > recreate > > > the > > > > net::DrainableIOBuffer every time it is filled out. Otherwise, > > > > DrainableIOBuffer::BytesRemaining() would return an incorrect value for > not > > > > fully-filled out buffers. I'm not very sure if it would be better here, > > since > > > we > > > > would need two members: > > > > > > > > scoped_refptr<net::IOBuffer> internal_buffer; // internal buffer for > > > > current_buffer. > > > > scoped_refptr<net::DrainableIOBuffer> current_buffer; // recreated every > > > > Preload() call. > > > > > > > > WDYT? > > > > > > I meant by using DrailableIOBuffer there is no need to have > > > |preloading_buffer_offset_| and manually offset the pointer. > > > You can use SetOffset() and DidConsume() to do the same thing as you do by > > > changing the value of preloading_buffer_offset_. > > > > > > You can reuse the same DrainableIOBuffer by resetting the offset with > > > SetOffset(0). > > > > This won't work since the buffer is not always full. > > Eg. if there is only 10 bytes in the buffer of size 50, then BytesRemaining() > > will return 50, once it should return 10. > > You don't need BytesRemaining(). > What I suggested was replacing: > - "preloading_buffer_offset_ += read_bytes;" with DidConsume(read_bytes) > - "preloading_buffer_offset_ = 0;" with SetOffset(0) > so that you can manage the offset with the buffer itself. > This is just a suggestion. You can use whatever you think reasonable. DrailableIOBuffer is designed for fully filled out buffers. I think it is confusing to use it here, since some of methods would return misleading values (even if we don't use them). Let me go with the simpler solution, using preloading_buffer_offset_. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/05 10:07:10, hashimoto wrote: > On 2014/06/05 09:22:27, mtomasz wrote: > > On 2014/06/05 03:18:36, hashimoto wrote: > > > No need to have this method? > > > I don't see any danger in using base::Unretained as Logger always lives > longer > > > than the reader. > > > > We are passing it to the BufferingFileStreamReader, which can call it back > with > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates fake > > threads backed up with the same message loop. If we had real threads, then > this > > could crash (IIUC). I think having a weak ptr is just a safe choice, since its > > very simple and will always work fine. It doesn't add much overhead either. > > WDYT? > > I think it's OK to take advantage of the single threaded test and using WeakPtr > looks like a kind of overkill. > Also, we can even stop Logger for recording results like below. > (the only exception is recording the method calls for the internal reader) > > void PushResultToVector(std::vector<int>* out, int result) { > out->push_back(result); > } > > TEST(...) { > std::vector<int> results; > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, &results)); > } > > Anyways, it's up to you. Even if all threads are backed by the same message loop, I think it is not safe to use base::Unretained. It may happen that tested classes (1) store the callback and run it via PostTask in their destructor, *or* (2) we forget to run RunLoopUntilIdle(). As a result, a task will be added to the message loop, and executed *after* the test case is finished. When TestBrowserThreadBundle is destroyed, all message loops are drained, and then we would get a crash: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... This is not likely to happen in this case, but in general I think we can't safely assume that we can use base::Unretained in unit tests in a single threaded environment. Saying that, always using a weak pointer gives us a guarantee that it will always work, and never crash, whatever is the implementation of tested classes, and whether we forget to call RunLoop() or not. And it costs us only 1 extra line of code. https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:100: FROM_HERE, base::Bind(callback, copy_result)); On 2014/06/05 10:07:10, hashimoto wrote: > No need to post task here as OnPreloadCompleted() is never called synchronously. > (and callback is run directly when result < 0) Done. https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h:27: explicit BufferingFileStreamReader( On 2014/06/05 10:07:10, hashimoto wrote: > Still seeing unneeded "explicit" Done. https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h (right): https://codereview.chromium.org/318563002/diff/60001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.h:16: class AsyncFileUtil; On 2014/06/05 10:07:10, hashimoto wrote: > nit: Unused forward declaration? Done.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/06 01:31:03, mtomasz wrote: > On 2014/06/05 10:07:10, hashimoto wrote: > > On 2014/06/05 09:22:27, mtomasz wrote: > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > No need to have this method? > > > > I don't see any danger in using base::Unretained as Logger always lives > > longer > > > > than the reader. > > > > > > We are passing it to the BufferingFileStreamReader, which can call it back > > with > > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates > fake > > > threads backed up with the same message loop. If we had real threads, then > > this > > > could crash (IIUC). I think having a weak ptr is just a safe choice, since > its > > > very simple and will always work fine. It doesn't add much overhead either. > > > WDYT? > > > > I think it's OK to take advantage of the single threaded test and using > WeakPtr > > looks like a kind of overkill. > > Also, we can even stop Logger for recording results like below. > > (the only exception is recording the method calls for the internal reader) > > > > void PushResultToVector(std::vector<int>* out, int result) { > > out->push_back(result); > > } > > > > TEST(...) { > > std::vector<int> results; > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, &results)); > > } > > > > Anyways, it's up to you. > > Even if all threads are backed by the same message loop, I think it is not safe > to use base::Unretained. It may happen that tested classes (1) store the > callback and run it via PostTask in their destructor, *or* (2) we forget to run > RunLoopUntilIdle(). As a result, a task will be added to the message loop, and > executed *after* the test case is finished. When TestBrowserThreadBundle is > destroyed, all message loops are drained, and then we would get a crash: > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... (1) Here we know that doesn't happen with the tested class. (2) To report what happened, tests should behave nicely even when the tested class behaves badly, but forgetting to call Run() is a bug of the test itself. There is no point to add another complexity to be robust against badly implemented tests. > > This is not likely to happen in this case, but in general I think we can't > safely assume that we can use base::Unretained in unit tests in a single > threaded environment. Saying that, always using a weak pointer gives us a > guarantee that it will always work, and never crash, whatever is the > implementation of tested classes, and whether we forget to call RunLoop() or > not. And it costs us only 1 extra line of code. Not only one line, but also the vectors and base::Bind. Basically, you are reinventing TestCopmletionCallback. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/05 09:22:27, mtomasz wrote: > On 2014/06/05 03:18:36, hashimoto wrote: > > How about using TestCompletionCallback? > > The same goes for other places passing Logger methods as callbacks. > > With TestCompletionCallback we can't control how many times the callback has > been called. What do you mean? TestCompletionCallback::WaitForResult() quits the message loop once the callback gets called. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; How about adding here something like: if (buffer_length >= buffer_size_) return file_stream_reader_->Read(buffer, buffer_length, callback); I'm worrying, in future, the user code may pass a big value for |buffer_length| and this place may be a hidden bottleneck.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/06 03:36:52, hashimoto wrote: > On 2014/06/06 01:31:03, mtomasz wrote: > > On 2014/06/05 10:07:10, hashimoto wrote: > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > No need to have this method? > > > > > I don't see any danger in using base::Unretained as Logger always lives > > > longer > > > > > than the reader. > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call it back > > > with > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle creates > > fake > > > > threads backed up with the same message loop. If we had real threads, then > > > this > > > > could crash (IIUC). I think having a weak ptr is just a safe choice, since > > its > > > > very simple and will always work fine. It doesn't add much overhead > either. > > > > WDYT? > > > > > > I think it's OK to take advantage of the single threaded test and using > > WeakPtr > > > looks like a kind of overkill. > > > Also, we can even stop Logger for recording results like below. > > > (the only exception is recording the method calls for the internal reader) > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > out->push_back(result); > > > } > > > > > > TEST(...) { > > > std::vector<int> results; > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > &results)); > > > } > > > > > > Anyways, it's up to you. > > > > Even if all threads are backed by the same message loop, I think it is not > safe > > to use base::Unretained. It may happen that tested classes (1) store the > > callback and run it via PostTask in their destructor, *or* (2) we forget to > run > > RunLoopUntilIdle(). As a result, a task will be added to the message loop, and > > executed *after* the test case is finished. When TestBrowserThreadBundle is > > destroyed, all message loops are drained, and then we would get a crash: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > (1) Here we know that doesn't happen with the tested class. > (2) To report what happened, tests should behave nicely even when the tested > class behaves badly, but forgetting to call Run() is a bug of the test itself. > There is no point to add another complexity to be robust against badly > implemented tests. > > > > This is not likely to happen in this case, but in general I think we can't > > safely assume that we can use base::Unretained in unit tests in a single > > threaded environment. Saying that, always using a weak pointer gives us a > > guarantee that it will always work, and never crash, whatever is the > > implementation of tested classes, and whether we forget to call RunLoop() or > > not. And it costs us only 1 extra line of code. > Not only one line, but also the vectors and base::Bind. > Basically, you are reinventing TestCopmletionCallback. Your suggestion also uses vectors and base::Bind, but without the class wrapper. I can't use TestCompletionCallback, since it doesn't register multiple calls. Saying that, I don't think having a logger class is such a big overhead. It is self documenting and wrapping all of the logging callbacks and vectors in one place. If you don't have a strong feeling about this, I'd like to go with the current solution, as all of the tests in chrome/browser/file_system_provider use the same pattern. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/06 03:36:52, hashimoto wrote: > On 2014/06/05 09:22:27, mtomasz wrote: > > On 2014/06/05 03:18:36, hashimoto wrote: > > > How about using TestCompletionCallback? > > > The same goes for other places passing Logger methods as callbacks. > > > > With TestCompletionCallback we can't control how many times the callback has > > been called. > > What do you mean? > TestCompletionCallback::WaitForResult() quits the message loop once the callback > gets called. If the tested class by accident calls the callback multiple times instead of one, then we won't catch this error with TestCompletionCallback. Such error would happen, if we had an error in: https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... GOOD: 79 if (result != net::ERR_IO_PENDING) { BAD: 79 if (result == net::ERR_IO_PENDING) { Using TestCompletionCallback would not let us catch this serious (and easy to make) mistake. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/06 03:36:52, hashimoto wrote: > How about adding here something like: > > if (buffer_length >= buffer_size_) > return file_stream_reader_->Read(buffer, buffer_length, callback); > > I'm worrying, in future, the user code may pass a big value for |buffer_length| > and this place may be a hidden bottleneck. The buffer length is not user controlled, but it is hard-coded currently to 32KB. However, if it happened that it is larger than |buffer_size| (512KB) and we would have this extra-if, then we could crash the renderer process, since the maximum IPC message is 1MB. https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... Hence, the current code has two purposes - to increase chunk size from 32KB to 512KB, and to cap it to 512KB, so we won't crash the renderer.
On 2014/06/06 04:22:56, mtomasz wrote: > https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc > (right): > > https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: > base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } > On 2014/06/06 03:36:52, hashimoto wrote: > > On 2014/06/06 01:31:03, mtomasz wrote: > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > No need to have this method? > > > > > > I don't see any danger in using base::Unretained as Logger always > lives > > > > longer > > > > > > than the reader. > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call it > back > > > > with > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle > creates > > > fake > > > > > threads backed up with the same message loop. If we had real threads, > then > > > > this > > > > > could crash (IIUC). I think having a weak ptr is just a safe choice, > since > > > its > > > > > very simple and will always work fine. It doesn't add much overhead > > either. > > > > > WDYT? > > > > > > > > I think it's OK to take advantage of the single threaded test and using > > > WeakPtr > > > > looks like a kind of overkill. > > > > Also, we can even stop Logger for recording results like below. > > > > (the only exception is recording the method calls for the internal reader) > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > out->push_back(result); > > > > } > > > > > > > > TEST(...) { > > > > std::vector<int> results; > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > &results)); > > > > } > > > > > > > > Anyways, it's up to you. > > > > > > Even if all threads are backed by the same message loop, I think it is not > > safe > > > to use base::Unretained. It may happen that tested classes (1) store the > > > callback and run it via PostTask in their destructor, *or* (2) we forget to > > run > > > RunLoopUntilIdle(). As a result, a task will be added to the message loop, > and > > > executed *after* the test case is finished. When TestBrowserThreadBundle is > > > destroyed, all message loops are drained, and then we would get a crash: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > (1) Here we know that doesn't happen with the tested class. > > (2) To report what happened, tests should behave nicely even when the tested > > class behaves badly, but forgetting to call Run() is a bug of the test itself. > > > There is no point to add another complexity to be robust against badly > > implemented tests. > > > > > > This is not likely to happen in this case, but in general I think we can't > > > safely assume that we can use base::Unretained in unit tests in a single > > > threaded environment. Saying that, always using a weak pointer gives us a > > > guarantee that it will always work, and never crash, whatever is the > > > implementation of tested classes, and whether we forget to call RunLoop() or > > > not. And it costs us only 1 extra line of code. > > Not only one line, but also the vectors and base::Bind. > > Basically, you are reinventing TestCopmletionCallback. > > Your suggestion also uses vectors and base::Bind, but without the class wrapper. > I can't use TestCompletionCallback, since it doesn't register multiple calls. > Saying that, I don't think having a logger class is such a big overhead. It is > self documenting and wrapping all of the logging callbacks and vectors in one > place. > > If you don't have a strong feeling about this, I'd like to go with the current > solution, as all of the tests in chrome/browser/file_system_provider use the > same pattern. > > https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: > base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); > On 2014/06/06 03:36:52, hashimoto wrote: > > On 2014/06/05 09:22:27, mtomasz wrote: > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > How about using TestCompletionCallback? > > > > The same goes for other places passing Logger methods as callbacks. > > > > > > With TestCompletionCallback we can't control how many times the callback has > > > been called. > > > > What do you mean? > > TestCompletionCallback::WaitForResult() quits the message loop once the > callback > > gets called. > > If the tested class by accident calls the callback multiple times instead of > one, then we won't catch this error with TestCompletionCallback. > > Such error would happen, if we had an error in: > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > GOOD: > 79 if (result != net::ERR_IO_PENDING) { > > BAD: > 79 if (result == net::ERR_IO_PENDING) { > > Using TestCompletionCallback would not let us catch this serious (and easy to > make) mistake. > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > File > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc > (right): > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: > return bytes_read; > On 2014/06/06 03:36:52, hashimoto wrote: > > How about adding here something like: > > > > if (buffer_length >= buffer_size_) > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > I'm worrying, in future, the user code may pass a big value for > |buffer_length| > > and this place may be a hidden bottleneck. > > The buffer length is not user controlled, but it is hard-coded currently to > 32KB. However, if it happened that it is larger than |buffer_size| (512KB) and > we would have this extra-if, then we could crash the renderer process, since the > maximum IPC message is 1MB. > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > Hence, the current code has two purposes - to increase chunk size from 32KB to > 512KB, and to cap it to 512KB, so we won't crash the renderer. @hashimoto: Ping.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/06 04:22:56, mtomasz wrote: > On 2014/06/06 03:36:52, hashimoto wrote: > > On 2014/06/06 01:31:03, mtomasz wrote: > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > No need to have this method? > > > > > > I don't see any danger in using base::Unretained as Logger always > lives > > > > longer > > > > > > than the reader. > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call it > back > > > > with > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle > creates > > > fake > > > > > threads backed up with the same message loop. If we had real threads, > then > > > > this > > > > > could crash (IIUC). I think having a weak ptr is just a safe choice, > since > > > its > > > > > very simple and will always work fine. It doesn't add much overhead > > either. > > > > > WDYT? > > > > > > > > I think it's OK to take advantage of the single threaded test and using > > > WeakPtr > > > > looks like a kind of overkill. > > > > Also, we can even stop Logger for recording results like below. > > > > (the only exception is recording the method calls for the internal reader) > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > out->push_back(result); > > > > } > > > > > > > > TEST(...) { > > > > std::vector<int> results; > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > &results)); > > > > } > > > > > > > > Anyways, it's up to you. > > > > > > Even if all threads are backed by the same message loop, I think it is not > > safe > > > to use base::Unretained. It may happen that tested classes (1) store the > > > callback and run it via PostTask in their destructor, *or* (2) we forget to > > run > > > RunLoopUntilIdle(). As a result, a task will be added to the message loop, > and > > > executed *after* the test case is finished. When TestBrowserThreadBundle is > > > destroyed, all message loops are drained, and then we would get a crash: > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > (1) Here we know that doesn't happen with the tested class. > > (2) To report what happened, tests should behave nicely even when the tested > > class behaves badly, but forgetting to call Run() is a bug of the test itself. > > > There is no point to add another complexity to be robust against badly > > implemented tests. > > > > > > This is not likely to happen in this case, but in general I think we can't > > > safely assume that we can use base::Unretained in unit tests in a single > > > threaded environment. Saying that, always using a weak pointer gives us a > > > guarantee that it will always work, and never crash, whatever is the > > > implementation of tested classes, and whether we forget to call RunLoop() or > > > not. And it costs us only 1 extra line of code. > > Not only one line, but also the vectors and base::Bind. > > Basically, you are reinventing TestCopmletionCallback. > > Your suggestion also uses vectors and base::Bind, but without the class wrapper. > I can't use TestCompletionCallback, since it doesn't register multiple calls. > Saying that, I don't think having a logger class is such a big overhead. It is > self documenting and wrapping all of the logging callbacks and vectors in one > place. > > If you don't have a strong feeling about this, I'd like to go with the current > solution, as all of the tests in chrome/browser/file_system_provider use the > same pattern. I still don't understand why you need this class: 1. When an unexpected call is made against the callback, it's better to get notified about it with a crash instead of the call being silently ignored by WeakPtr. (A crash in a test is worse than a graceful failure because it aborts the test process, but better than silently dismissing hidden failures.) 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods to push the result, 3 getters, lengthy base::Bind everywhere. This class is basically a reinvention of TestCompletionCallback in a less sophisticated way. 3. TestCompletionCallback is used much more widely in the entire code base. Anyways, if you believe this class is necessary, I'm OK with this code at least it works and I'm not the engineer who is responsible to maintain this code until the end of its lifetime. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/06 04:22:56, mtomasz wrote: > On 2014/06/06 03:36:52, hashimoto wrote: > > On 2014/06/05 09:22:27, mtomasz wrote: > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > How about using TestCompletionCallback? > > > > The same goes for other places passing Logger methods as callbacks. > > > > > > With TestCompletionCallback we can't control how many times the callback has > > > been called. > > > > What do you mean? > > TestCompletionCallback::WaitForResult() quits the message loop once the > callback > > gets called. > > If the tested class by accident calls the callback multiple times instead of > one, then we won't catch this error with TestCompletionCallback. At least, we can catch it because it crashes as you described. > > Such error would happen, if we had an error in: > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > GOOD: > 79 if (result != net::ERR_IO_PENDING) { > > BAD: > 79 if (result == net::ERR_IO_PENDING) { > > Using TestCompletionCallback would not let us catch this serious (and easy to > make) mistake. This can be caught by the synchronous version tests. BTW, why do you need to have a code path and parameterized tests to handle results returned synchronously? Doesn't providing extensions always return the result asynchronously? https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/06 04:22:56, mtomasz wrote: > On 2014/06/06 03:36:52, hashimoto wrote: > > How about adding here something like: > > > > if (buffer_length >= buffer_size_) > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > I'm worrying, in future, the user code may pass a big value for > |buffer_length| > > and this place may be a hidden bottleneck. > > The buffer length is not user controlled, but it is hard-coded currently to > 32KB. However, if it happened that it is larger than |buffer_size| (512KB) and > we would have this extra-if, then we could crash the renderer process, since the > maximum IPC message is 1MB. > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" 128MB? > > Hence, the current code has two purposes - to increase chunk size from 32KB to > 512KB, and to cap it to 512KB, so we won't crash the renderer. If it was the purpose, why don't you add a static assert against it?
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/09 11:12:04, hashimoto wrote: > On 2014/06/06 04:22:56, mtomasz wrote: > > On 2014/06/06 03:36:52, hashimoto wrote: > > > On 2014/06/06 01:31:03, mtomasz wrote: > > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > No need to have this method? > > > > > > > I don't see any danger in using base::Unretained as Logger always > > lives > > > > > longer > > > > > > > than the reader. > > > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call it > > back > > > > > with > > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) call > > > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle > > creates > > > > fake > > > > > > threads backed up with the same message loop. If we had real threads, > > then > > > > > this > > > > > > could crash (IIUC). I think having a weak ptr is just a safe choice, > > since > > > > its > > > > > > very simple and will always work fine. It doesn't add much overhead > > > either. > > > > > > WDYT? > > > > > > > > > > I think it's OK to take advantage of the single threaded test and using > > > > WeakPtr > > > > > looks like a kind of overkill. > > > > > Also, we can even stop Logger for recording results like below. > > > > > (the only exception is recording the method calls for the internal > reader) > > > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > > out->push_back(result); > > > > > } > > > > > > > > > > TEST(...) { > > > > > std::vector<int> results; > > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > > &results)); > > > > > } > > > > > > > > > > Anyways, it's up to you. > > > > > > > > Even if all threads are backed by the same message loop, I think it is not > > > safe > > > > to use base::Unretained. It may happen that tested classes (1) store the > > > > callback and run it via PostTask in their destructor, *or* (2) we forget > to > > > run > > > > RunLoopUntilIdle(). As a result, a task will be added to the message loop, > > and > > > > executed *after* the test case is finished. When TestBrowserThreadBundle > is > > > > destroyed, all message loops are drained, and then we would get a crash: > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > (1) Here we know that doesn't happen with the tested class. > > > (2) To report what happened, tests should behave nicely even when the tested > > > class behaves badly, but forgetting to call Run() is a bug of the test > itself. > > > > > There is no point to add another complexity to be robust against badly > > > implemented tests. > > > > > > > > This is not likely to happen in this case, but in general I think we can't > > > > safely assume that we can use base::Unretained in unit tests in a single > > > > threaded environment. Saying that, always using a weak pointer gives us a > > > > guarantee that it will always work, and never crash, whatever is the > > > > implementation of tested classes, and whether we forget to call RunLoop() > or > > > > not. And it costs us only 1 extra line of code. > > > Not only one line, but also the vectors and base::Bind. > > > Basically, you are reinventing TestCopmletionCallback. > > > > Your suggestion also uses vectors and base::Bind, but without the class > wrapper. > > I can't use TestCompletionCallback, since it doesn't register multiple calls. > > Saying that, I don't think having a logger class is such a big overhead. It is > > self documenting and wrapping all of the logging callbacks and vectors in one > > place. > > > > If you don't have a strong feeling about this, I'd like to go with the current > > solution, as all of the tests in chrome/browser/file_system_provider use the > > same pattern. > I still don't understand why you need this class: > 1. When an unexpected call is made against the callback, it's better to get > notified about it with a crash instead of the call being silently ignored by > WeakPtr. (A crash in a test is worse than a graceful failure because it aborts > the test process, but better than silently dismissing hidden failures.) > 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods to push > the result, 3 getters, lengthy base::Bind everywhere. This class is basically a > reinvention of TestCompletionCallback in a less sophisticated way. > 3. TestCompletionCallback is used much more widely in the entire code base. > > Anyways, if you believe this class is necessary, I'm OK with this code at least > it works and I'm not the engineer who is responsible to maintain this code until > the end of its lifetime. I think we are talking about two different things. IIUC you want to use TestCompletionCallback here. But we can't do that, as I said before. In case of line #211, we are checking the second call. If we can't use TestCompletionCallback, then we would have to use suggested by you PushResultToVector, with local vectors. Do you suggest to use TestCompletionCallback for callbacks which we assume are called once, and PushResultToVector for callbacks which do not have such assumption? We would save some lines of code, but isn't having a simple Logger class just neater and cleaner? WDYT? As for the weak pointer, I'll remove it. Catching the crash over ignoring sounds good to me. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/09 11:12:04, hashimoto wrote: > On 2014/06/06 04:22:56, mtomasz wrote: > > On 2014/06/06 03:36:52, hashimoto wrote: > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > How about using TestCompletionCallback? > > > > > The same goes for other places passing Logger methods as callbacks. > > > > > > > > With TestCompletionCallback we can't control how many times the callback > has > > > > been called. > > > > > > What do you mean? > > > TestCompletionCallback::WaitForResult() quits the message loop once the > > callback > > > gets called. > > > > If the tested class by accident calls the callback multiple times instead of > > one, then we won't catch this error with TestCompletionCallback. > At least, we can catch it because it crashes as you described. It would crash. But I don't think crashing is better that detecting it with an assert. Also, we are checking the second result in some places. With TestCompletionCallback we can't do that. > > > > Such error would happen, if we had an error in: > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > GOOD: > > 79 if (result != net::ERR_IO_PENDING) { > > > > BAD: > > 79 if (result == net::ERR_IO_PENDING) { > > > > Using TestCompletionCallback would not let us catch this serious (and easy to > > make) mistake. > This can be caught by the synchronous version tests. > > BTW, why do you need to have a code path and parameterized tests to handle > results returned synchronously? > Doesn't providing extensions always return the result asynchronously? The BufferingFileStreamReader works with any webkit_blob::FileStreamReader, and it has zero dependency on the underlying file stream reader. It makes it simple. We can of course remove the synchronous code paths, and add dependency and assumptions on the implementation of the provided file system reader. Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of code. Is it really worth? https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/09 11:12:04, hashimoto wrote: > On 2014/06/06 04:22:56, mtomasz wrote: > > On 2014/06/06 03:36:52, hashimoto wrote: > > > How about adding here something like: > > > > > > if (buffer_length >= buffer_size_) > > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > > > I'm worrying, in future, the user code may pass a big value for > > |buffer_length| > > > and this place may be a hidden bottleneck. > > > > The buffer length is not user controlled, but it is hard-coded currently to > > 32KB. However, if it happened that it is larger than |buffer_size| (512KB) and > > we would have this extra-if, then we could crash the renderer process, since > the > > maximum IPC message is 1MB. > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" 128MB? > > > > Hence, the current code has two purposes - to increase chunk size from 32KB to > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > If it was the purpose, why don't you add a static assert against it? > You're right, of course 128MB. In such case, the extra if makes of course sense. But how do you want to use a static assert here? Could you clarify?
PTAL. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/09 13:51:16, mtomasz wrote: > On 2014/06/09 11:12:04, hashimoto wrote: > > On 2014/06/06 04:22:56, mtomasz wrote: > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > How about adding here something like: > > > > > > > > if (buffer_length >= buffer_size_) > > > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > |buffer_length| > > > > and this place may be a hidden bottleneck. > > > > > > The buffer length is not user controlled, but it is hard-coded currently to > > > 32KB. However, if it happened that it is larger than |buffer_size| (512KB) > and > > > we would have this extra-if, then we could crash the renderer process, since > > the > > > maximum IPC message is 1MB. > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" 128MB? > > > > > > Hence, the current code has two purposes - to increase chunk size from 32KB > to > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > If it was the purpose, why don't you add a static assert against it? > > > > You're right, of course 128MB. In such case, the extra if makes of course sense. > But how do you want to use a static assert here? Could you clarify? I thought about it again, and IMHO we should have a cap for the buffer size as it is. We can't simply add that if. If the inner buffer already has some data, we have to copy them first, and then if |buffer_length - bytes_read| > buffer_size_, then the rest directly. It is non trivial, and it will require a test case. But we won't use this code, because of the current buffer length size restriction. If it grows to large size in the future, then fetching a lot of data directly from providing extensions will cause request timeouts in case of remote providing extensions. So basically, there is IMHO no reason to add this code path. Hence, I added a comment to make it clear that this class fetches data from the underlying file stream reader in chunks of up to |buffer_length| size.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/09 13:51:15, mtomasz wrote: > On 2014/06/09 11:12:04, hashimoto wrote: > > On 2014/06/06 04:22:56, mtomasz wrote: > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > On 2014/06/06 01:31:03, mtomasz wrote: > > > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > No need to have this method? > > > > > > > > I don't see any danger in using base::Unretained as Logger always > > > lives > > > > > > longer > > > > > > > > than the reader. > > > > > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call > it > > > back > > > > > > with > > > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) > call > > > > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle > > > creates > > > > > fake > > > > > > > threads backed up with the same message loop. If we had real > threads, > > > then > > > > > > this > > > > > > > could crash (IIUC). I think having a weak ptr is just a safe choice, > > > since > > > > > its > > > > > > > very simple and will always work fine. It doesn't add much overhead > > > > either. > > > > > > > WDYT? > > > > > > > > > > > > I think it's OK to take advantage of the single threaded test and > using > > > > > WeakPtr > > > > > > looks like a kind of overkill. > > > > > > Also, we can even stop Logger for recording results like below. > > > > > > (the only exception is recording the method calls for the internal > > reader) > > > > > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > > > out->push_back(result); > > > > > > } > > > > > > > > > > > > TEST(...) { > > > > > > std::vector<int> results; > > > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > > > &results)); > > > > > > } > > > > > > > > > > > > Anyways, it's up to you. > > > > > > > > > > Even if all threads are backed by the same message loop, I think it is > not > > > > safe > > > > > to use base::Unretained. It may happen that tested classes (1) store the > > > > > callback and run it via PostTask in their destructor, *or* (2) we forget > > to > > > > run > > > > > RunLoopUntilIdle(). As a result, a task will be added to the message > loop, > > > and > > > > > executed *after* the test case is finished. When TestBrowserThreadBundle > > is > > > > > destroyed, all message loops are drained, and then we would get a crash: > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > (1) Here we know that doesn't happen with the tested class. > > > > (2) To report what happened, tests should behave nicely even when the > tested > > > > class behaves badly, but forgetting to call Run() is a bug of the test > > itself. > > > > > > > There is no point to add another complexity to be robust against badly > > > > implemented tests. > > > > > > > > > > This is not likely to happen in this case, but in general I think we > can't > > > > > safely assume that we can use base::Unretained in unit tests in a single > > > > > threaded environment. Saying that, always using a weak pointer gives us > a > > > > > guarantee that it will always work, and never crash, whatever is the > > > > > implementation of tested classes, and whether we forget to call > RunLoop() > > or > > > > > not. And it costs us only 1 extra line of code. > > > > Not only one line, but also the vectors and base::Bind. > > > > Basically, you are reinventing TestCopmletionCallback. > > > > > > Your suggestion also uses vectors and base::Bind, but without the class > > wrapper. > > > I can't use TestCompletionCallback, since it doesn't register multiple > calls. > > > Saying that, I don't think having a logger class is such a big overhead. It > is > > > self documenting and wrapping all of the logging callbacks and vectors in > one > > > place. > > > > > > If you don't have a strong feeling about this, I'd like to go with the > current > > > solution, as all of the tests in chrome/browser/file_system_provider use the > > > same pattern. > > I still don't understand why you need this class: > > 1. When an unexpected call is made against the callback, it's better to get > > notified about it with a crash instead of the call being silently ignored by > > WeakPtr. (A crash in a test is worse than a graceful failure because it aborts > > the test process, but better than silently dismissing hidden failures.) > > 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods to > push > > the result, 3 getters, lengthy base::Bind everywhere. This class is basically > a > > reinvention of TestCompletionCallback in a less sophisticated way. > > 3. TestCompletionCallback is used much more widely in the entire code base. > > > > Anyways, if you believe this class is necessary, I'm OK with this code at > least > > it works and I'm not the engineer who is responsible to maintain this code > until > > the end of its lifetime. > > I think we are talking about two different things. IIUC you want to use > TestCompletionCallback here. But we can't do that, as I said before. In case of > line #211, we are checking the second call. Sorry, I forgot to note that inner_read_requested_events_ is the exception. This vector is serving for a totally different purpose while other vectors can be easily replaced with TestCompletionCallback. > > If we can't use TestCompletionCallback, then we would have to use suggested by > you PushResultToVector, with local vectors. Do you suggest to use > TestCompletionCallback for callbacks which we assume are called once, and > PushResultToVector for callbacks which do not have such assumption? We would > save some lines of code, but isn't having a simple Logger class just neater and > cleaner? WDYT? The problem of this class's design is that there is no merit to have read_result_events_ and get_length_result_events_ in this class. These two vectors and related methods can be replaced with TestCompletionCallback or local variables and free functions without any problems. Is there any reason with which you need to put the vector for recording the arguments of function calls made against the inner reader and the vectors for recording the result of function calls made against the external reader? > > As for the weak pointer, I'll remove it. Catching the crash over ignoring sounds > good to me. https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/09 13:51:15, mtomasz wrote: > On 2014/06/09 11:12:04, hashimoto wrote: > > On 2014/06/06 04:22:56, mtomasz wrote: > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > How about using TestCompletionCallback? > > > > > > The same goes for other places passing Logger methods as callbacks. > > > > > > > > > > With TestCompletionCallback we can't control how many times the callback > > has > > > > > been called. > > > > > > > > What do you mean? > > > > TestCompletionCallback::WaitForResult() quits the message loop once the > > > callback > > > > gets called. > > > > > > If the tested class by accident calls the callback multiple times instead of > > > one, then we won't catch this error with TestCompletionCallback. > > At least, we can catch it because it crashes as you described. > > It would crash. But I don't think crashing is better that detecting it with an > assert. > Also, we are checking the second result in some places. With > TestCompletionCallback we can't do that. You cannot prepare for every possible failure which can be caused by the tested class. What if, say, the tested class posts a task to base::WorkerPool and run the callback on a different thread than the main thread? Anyways, as I said, I'm OK with this "logger" as long as you take the responsibility to maintain this code. > > > > > > > Such error would happen, if we had an error in: > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > GOOD: > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > BAD: > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > Using TestCompletionCallback would not let us catch this serious (and easy > to > > > make) mistake. > > This can be caught by the synchronous version tests. > > > > BTW, why do you need to have a code path and parameterized tests to handle > > results returned synchronously? > > Doesn't providing extensions always return the result asynchronously? > > The BufferingFileStreamReader works with any webkit_blob::FileStreamReader, and > it has zero dependency on the underlying file stream reader. It makes it simple. > > We can of course remove the synchronous code paths, and add dependency and > assumptions on the implementation of the provided file system reader. > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of code. Is > it really worth? Didn't you say that only one single "if" statement can be a prone of bugs? Also, please don't forget about the complicated test code and TEST_P used here which often degrades the readability of test failure log quite much. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/10 08:38:42, mtomasz wrote: > On 2014/06/09 13:51:16, mtomasz wrote: > > On 2014/06/09 11:12:04, hashimoto wrote: > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > How about adding here something like: > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > > |buffer_length| > > > > > and this place may be a hidden bottleneck. > > > > > > > > The buffer length is not user controlled, but it is hard-coded currently > to > > > > 32KB. However, if it happened that it is larger than |buffer_size| (512KB) > > and > > > > we would have this extra-if, then we could crash the renderer process, > since > > > the > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" 128MB? > > > > > > > > Hence, the current code has two purposes - to increase chunk size from > 32KB > > to > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > If it was the purpose, why don't you add a static assert against it? > > > > > > > You're right, of course 128MB. In such case, the extra if makes of course > sense. > > But how do you want to use a static assert here? Could you clarify? If it was really a purpose of this class, having a static assert to check that the chunk size is less than the IPC size limit will make your purpose clearer and prevent mysterious crash from happening when someone tries to increase the chunk size in future. > > I thought about it again, and IMHO we should have a cap for the buffer size as > it is. > > We can't simply add that if. If the inner buffer already has some data, we have > to copy them first, and then if |buffer_length - bytes_read| > buffer_size_, > then the rest directly. The code here returns the result without performing actual read when any data is left in the buffer. Why do you think we need to change this behavior for direct read case? > > It is non trivial, and it will require a test case. But we won't use this code, Sorry, I don't understand why you are OK with supporting synchronous readers and having test cases for it which we will never need, while you are feeling nervous about preparing for future chunk size increase which may possibly happen. > because of the current buffer length size restriction. If it grows to large size > in the future, then fetching a lot of data directly from providing extensions > will cause request timeouts in case of remote providing extensions. If it was the case, shouldn't we allow providing extensions to decide how to behave? (e.g. Depending on their servers' capability, capping the chunk size and returning smaller amount of data for read request.) Also, isn't 512KB already large enough to cause timeout with poor network connection? > > So basically, there is IMHO no reason to add this code path. > > Hence, I added a comment to make it clear that this class fetches data from the > underlying file stream reader in chunks of up to |buffer_length| size.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/10 10:43:24, hashimoto wrote: > On 2014/06/09 13:51:15, mtomasz wrote: > > On 2014/06/09 11:12:04, hashimoto wrote: > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > On 2014/06/06 01:31:03, mtomasz wrote: > > > > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > No need to have this method? > > > > > > > > > I don't see any danger in using base::Unretained as Logger > always > > > > lives > > > > > > > longer > > > > > > > > > than the reader. > > > > > > > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can call > > it > > > > back > > > > > > > with > > > > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) > > call > > > > > > > > RunLoop::RunUntilIdle and (2) the current TestBrowserThreadBundle > > > > creates > > > > > > fake > > > > > > > > threads backed up with the same message loop. If we had real > > threads, > > > > then > > > > > > > this > > > > > > > > could crash (IIUC). I think having a weak ptr is just a safe > choice, > > > > since > > > > > > its > > > > > > > > very simple and will always work fine. It doesn't add much > overhead > > > > > either. > > > > > > > > WDYT? > > > > > > > > > > > > > > I think it's OK to take advantage of the single threaded test and > > using > > > > > > WeakPtr > > > > > > > looks like a kind of overkill. > > > > > > > Also, we can even stop Logger for recording results like below. > > > > > > > (the only exception is recording the method calls for the internal > > > reader) > > > > > > > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > > > > out->push_back(result); > > > > > > > } > > > > > > > > > > > > > > TEST(...) { > > > > > > > std::vector<int> results; > > > > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > > > > &results)); > > > > > > > } > > > > > > > > > > > > > > Anyways, it's up to you. > > > > > > > > > > > > Even if all threads are backed by the same message loop, I think it is > > not > > > > > safe > > > > > > to use base::Unretained. It may happen that tested classes (1) store > the > > > > > > callback and run it via PostTask in their destructor, *or* (2) we > forget > > > to > > > > > run > > > > > > RunLoopUntilIdle(). As a result, a task will be added to the message > > loop, > > > > and > > > > > > executed *after* the test case is finished. When > TestBrowserThreadBundle > > > is > > > > > > destroyed, all message loops are drained, and then we would get a > crash: > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > > (1) Here we know that doesn't happen with the tested class. > > > > > (2) To report what happened, tests should behave nicely even when the > > tested > > > > > class behaves badly, but forgetting to call Run() is a bug of the test > > > itself. > > > > > > > > > There is no point to add another complexity to be robust against badly > > > > > implemented tests. > > > > > > > > > > > > This is not likely to happen in this case, but in general I think we > > can't > > > > > > safely assume that we can use base::Unretained in unit tests in a > single > > > > > > threaded environment. Saying that, always using a weak pointer gives > us > > a > > > > > > guarantee that it will always work, and never crash, whatever is the > > > > > > implementation of tested classes, and whether we forget to call > > RunLoop() > > > or > > > > > > not. And it costs us only 1 extra line of code. > > > > > Not only one line, but also the vectors and base::Bind. > > > > > Basically, you are reinventing TestCopmletionCallback. > > > > > > > > Your suggestion also uses vectors and base::Bind, but without the class > > > wrapper. > > > > I can't use TestCompletionCallback, since it doesn't register multiple > > calls. > > > > Saying that, I don't think having a logger class is such a big overhead. > It > > is > > > > self documenting and wrapping all of the logging callbacks and vectors in > > one > > > > place. > > > > > > > > If you don't have a strong feeling about this, I'd like to go with the > > current > > > > solution, as all of the tests in chrome/browser/file_system_provider use > the > > > > same pattern. > > > I still don't understand why you need this class: > > > 1. When an unexpected call is made against the callback, it's better to get > > > notified about it with a crash instead of the call being silently ignored by > > > WeakPtr. (A crash in a test is worse than a graceful failure because it > aborts > > > the test process, but better than silently dismissing hidden failures.) > > > 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods to > > push > > > the result, 3 getters, lengthy base::Bind everywhere. This class is > basically > > a > > > reinvention of TestCompletionCallback in a less sophisticated way. > > > 3. TestCompletionCallback is used much more widely in the entire code base. > > > > > > Anyways, if you believe this class is necessary, I'm OK with this code at > > least > > > it works and I'm not the engineer who is responsible to maintain this code > > until > > > the end of its lifetime. > > > > I think we are talking about two different things. IIUC you want to use > > TestCompletionCallback here. But we can't do that, as I said before. In case > of > > line #211, we are checking the second call. > Sorry, I forgot to note that inner_read_requested_events_ is the exception. > This vector is serving for a totally different purpose while other vectors can > be easily replaced with TestCompletionCallback. 1. inner_read_requested_events_ can't be replaced. We want to check if it is called only once. TestCompletionCallback doesn't allow that. 2. read_result_events can't be replaced - called more than once. In #211. 3. get_length_result_events could be replaced - called up to once, but I think it is better to check if it is not called more times than expected. So, IIUC, at most one of these three vectors can be replaced by TestCompletionCallback. Note, that I'm going to implement adaptive chunk size very soon, as written in the TODO. After doing that, we will have more complex test cases, which would require multiple callback calls. I can split the class to separate utility methods like you suggested before, and declare vectors locally. That would however end up on declaring the same vectors in several test cases. I'm not convinced it is more readable. But let's give it a try. > > > > If we can't use TestCompletionCallback, then we would have to use suggested by > > you PushResultToVector, with local vectors. Do you suggest to use > > TestCompletionCallback for callbacks which we assume are called once, and > > PushResultToVector for callbacks which do not have such assumption? We would > > save some lines of code, but isn't having a simple Logger class just neater > and > > cleaner? WDYT? > The problem of this class's design is that there is no merit to have > read_result_events_ and get_length_result_events_ in this class. > These two vectors and related methods can be replaced with > TestCompletionCallback or local variables and free functions without any > problems. > Is there any reason with which you need to put the vector for recording the > arguments of function calls made against the inner reader and the vectors for > recording the result of function calls made against the external reader? I'm not sure if I understand. We should test if the external reader calls the internal reader properly. Eg. we want to know if it was called with correct arguments (eg. here with larger buffer length). Also, I'm not recording calls made against the external reader. I'm recording the external Read and GetLength callback calls to know if they return correct value. Please clarify. > > > > As for the weak pointer, I'll remove it. Catching the crash over ignoring > sounds > > good to me. > https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/10 10:43:24, hashimoto wrote: > On 2014/06/09 13:51:15, mtomasz wrote: > > On 2014/06/09 11:12:04, hashimoto wrote: > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > How about using TestCompletionCallback? > > > > > > > The same goes for other places passing Logger methods as callbacks. > > > > > > > > > > > > With TestCompletionCallback we can't control how many times the > callback > > > has > > > > > > been called. > > > > > > > > > > What do you mean? > > > > > TestCompletionCallback::WaitForResult() quits the message loop once the > > > > callback > > > > > gets called. > > > > > > > > If the tested class by accident calls the callback multiple times instead > of > > > > one, then we won't catch this error with TestCompletionCallback. > > > At least, we can catch it because it crashes as you described. > > > > It would crash. But I don't think crashing is better that detecting it with an > > assert. > > Also, we are checking the second result in some places. With > > TestCompletionCallback we can't do that. > You cannot prepare for every possible failure which can be caused by the tested > class. > What if, say, the tested class posts a task to base::WorkerPool and run the > callback on a different thread than the main thread? > Anyways, as I said, I'm OK with this "logger" as long as you take the > responsibility to maintain this code. I cannot test everything, but I'm not sure if it is an argument to reduce test coverage. > > > > > > > > > > Such error would happen, if we had an error in: > > > > > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > > > GOOD: > > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > > > BAD: > > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > > > Using TestCompletionCallback would not let us catch this serious (and easy > > to > > > > make) mistake. > > > This can be caught by the synchronous version tests. > > > > > > BTW, why do you need to have a code path and parameterized tests to handle > > > results returned synchronously? > > > Doesn't providing extensions always return the result asynchronously? > > > > The BufferingFileStreamReader works with any webkit_blob::FileStreamReader, > and > > it has zero dependency on the underlying file stream reader. It makes it > simple. > > > > We can of course remove the synchronous code paths, and add dependency and > > assumptions on the implementation of the provided file system reader. > > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of code. Is > > it really worth? > Didn't you say that only one single "if" statement can be a prone of bugs? > Also, please don't forget about the complicated test code and TEST_P used here > which often degrades the readability of test failure log quite much. BufferingFileStreamReader is a thin wrapper. Supporting sync features doesn't add much complexity. Removing sync support would add DCHECKs and possible undefined behaviour in the future, if the inner reader returns something synchronously. I always prefer to avoid writing code which is not used, but here in the thin wrapper, it seems just less bug prone to support all features than cut out some of them. In return we are getting zero dependency, which is a good thing. I don't know about log degradation when using TEST_P. Test case names are very neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/10 10:43:24, hashimoto wrote: > On 2014/06/10 08:38:42, mtomasz wrote: > > On 2014/06/09 13:51:16, mtomasz wrote: > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > How about adding here something like: > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > > > |buffer_length| > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded currently > > to > > > > > 32KB. However, if it happened that it is larger than |buffer_size| > (512KB) > > > and > > > > > we would have this extra-if, then we could crash the renderer process, > > since > > > > the > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" > 128MB? > > > > > > > > > > Hence, the current code has two purposes - to increase chunk size from > > 32KB > > > to > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > If it was the purpose, why don't you add a static assert against it? > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of course > > sense. > > > But how do you want to use a static assert here? Could you clarify? > If it was really a purpose of this class, having a static assert to check that > the chunk size is less than the IPC size limit will make your purpose clearer > and prevent mysterious crash from happening when someone tries to increase the > chunk size in future. Do you want to crash with a static assert in such situations? Why? We want to let providers work, even if the |buffer_length| is too large. > > > > I thought about it again, and IMHO we should have a cap for the buffer size as > > it is. > > > > We can't simply add that if. If the inner buffer already has some data, we > have > > to copy them first, and then if |buffer_length - bytes_read| > buffer_size_, > > then the rest directly. > The code here returns the result without performing actual read when any data is > left in the buffer. We always try to read |buffer_length| from the internal buffer. If there is not enough, then we return partial results. If nothing is read, then we prefetch and return as much as we can. > Why do you think we need to change this behavior for direct read case? If we skip contents in the buffer, then we will lose data. We always *must* return all data from the internal buffer, before calling the internal file stream reader for more data. > > > > It is non trivial, and it will require a test case. But we won't use this > code, > Sorry, I don't understand why you are OK with supporting synchronous readers and > having test cases for it which we will never need, > while you are feeling nervous about preparing for future chunk size increase > which may possibly happen. Support for synchronous readers: + Let's us remove dependency on the internal file stream reader implementation. + Makes code simpler, and DCHECK free. Think of GetLength. - Requires TEST_P. Test cases are same. - We don't need it now. As for "preparing for future chunk size increase": - Requires *new* test cases. - We don't need it now. - We will not need it in the future. (See below) - May cause a lot of problems. (See below) - We can easily add it later if needed. I honestly can't see any advantages. > > because of the current buffer length size restriction. If it grows to large > size > > in the future, then fetching a lot of data directly from providing extensions > > will cause request timeouts in case of remote providing extensions. > If it was the case, shouldn't we allow providing extensions to decide how to > behave? > (e.g. Depending on their servers' capability, capping the chunk size and > returning smaller amount of data for read request.) 1. They already can return data in chunks, so eg. that 512KB they can split to eg. 10x51KB. 2. If we add this "if", then the buffer size may be any in the future. Every developer has to add logic for splitting it. But, I predict that most of them will not. So once we increase the buffer size in Chromium, all remote extensions will start timeouting, because a lot of developers assumed (by observation) that the buffer size is always 512KB. 3. If the buffer is too large, then it will affect copying time between remote providing extensions and between them and Drive. We can't start uploading, before the first chunk is fully downloaded. This means even 2x slower copy if the file size is around the buffer size. 4. Similarly, for too big buffer size, videos will load with a significant delay. 5. For big buffer size, copying progress bars will be very unreliable. Saying that, I'm still not convinced how having a large buffer, like couple of megabytes could be a good thing in general case. I mentioned several serious problems above, and probably there is much more. > Also, isn't 512KB already large enough to cause timeout with poor network > connection? It may be too big. I'm thinking of a solution for this, eg. a dialog like "This operation takes longer than expected. Do you want to wait for it or abort?". > > > > So basically, there is IMHO no reason to add this code path. > > > > Hence, I added a comment to make it clear that this class fetches data from > the > > underlying file stream reader in chunks of up to |buffer_length| size. >
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/10 12:23:55, mtomasz wrote: > On 2014/06/10 10:43:24, hashimoto wrote: > > On 2014/06/09 13:51:15, mtomasz wrote: > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > On 2014/06/06 01:31:03, mtomasz wrote: > > > > > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > > No need to have this method? > > > > > > > > > > I don't see any danger in using base::Unretained as Logger > > always > > > > > lives > > > > > > > > longer > > > > > > > > > > than the reader. > > > > > > > > > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can > call > > > it > > > > > back > > > > > > > > with > > > > > > > > > a PostTask. Using Unretained wouldn't crash, only because we (1) > > > call > > > > > > > > > RunLoop::RunUntilIdle and (2) the current > TestBrowserThreadBundle > > > > > creates > > > > > > > fake > > > > > > > > > threads backed up with the same message loop. If we had real > > > threads, > > > > > then > > > > > > > > this > > > > > > > > > could crash (IIUC). I think having a weak ptr is just a safe > > choice, > > > > > since > > > > > > > its > > > > > > > > > very simple and will always work fine. It doesn't add much > > overhead > > > > > > either. > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > I think it's OK to take advantage of the single threaded test and > > > using > > > > > > > WeakPtr > > > > > > > > looks like a kind of overkill. > > > > > > > > Also, we can even stop Logger for recording results like below. > > > > > > > > (the only exception is recording the method calls for the internal > > > > reader) > > > > > > > > > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > > > > > out->push_back(result); > > > > > > > > } > > > > > > > > > > > > > > > > TEST(...) { > > > > > > > > std::vector<int> results; > > > > > > > > reader.Read(buffer, kChunkSize, &base::Bind(&PushResultToVector, > > > > > > &results)); > > > > > > > > } > > > > > > > > > > > > > > > > Anyways, it's up to you. > > > > > > > > > > > > > > Even if all threads are backed by the same message loop, I think it > is > > > not > > > > > > safe > > > > > > > to use base::Unretained. It may happen that tested classes (1) store > > the > > > > > > > callback and run it via PostTask in their destructor, *or* (2) we > > forget > > > > to > > > > > > run > > > > > > > RunLoopUntilIdle(). As a result, a task will be added to the message > > > loop, > > > > > and > > > > > > > executed *after* the test case is finished. When > > TestBrowserThreadBundle > > > > is > > > > > > > destroyed, all message loops are drained, and then we would get a > > crash: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > > > (1) Here we know that doesn't happen with the tested class. > > > > > > (2) To report what happened, tests should behave nicely even when the > > > tested > > > > > > class behaves badly, but forgetting to call Run() is a bug of the test > > > > itself. > > > > > > > > > > > There is no point to add another complexity to be robust against badly > > > > > > implemented tests. > > > > > > > > > > > > > > This is not likely to happen in this case, but in general I think we > > > can't > > > > > > > safely assume that we can use base::Unretained in unit tests in a > > single > > > > > > > threaded environment. Saying that, always using a weak pointer gives > > us > > > a > > > > > > > guarantee that it will always work, and never crash, whatever is the > > > > > > > implementation of tested classes, and whether we forget to call > > > RunLoop() > > > > or > > > > > > > not. And it costs us only 1 extra line of code. > > > > > > Not only one line, but also the vectors and base::Bind. > > > > > > Basically, you are reinventing TestCopmletionCallback. > > > > > > > > > > Your suggestion also uses vectors and base::Bind, but without the class > > > > wrapper. > > > > > I can't use TestCompletionCallback, since it doesn't register multiple > > > calls. > > > > > Saying that, I don't think having a logger class is such a big overhead. > > It > > > is > > > > > self documenting and wrapping all of the logging callbacks and vectors > in > > > one > > > > > place. > > > > > > > > > > If you don't have a strong feeling about this, I'd like to go with the > > > current > > > > > solution, as all of the tests in chrome/browser/file_system_provider use > > the > > > > > same pattern. > > > > I still don't understand why you need this class: > > > > 1. When an unexpected call is made against the callback, it's better to > get > > > > notified about it with a crash instead of the call being silently ignored > by > > > > WeakPtr. (A crash in a test is worse than a graceful failure because it > > aborts > > > > the test process, but better than silently dismissing hidden failures.) > > > > 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods > to > > > push > > > > the result, 3 getters, lengthy base::Bind everywhere. This class is > > basically > > > a > > > > reinvention of TestCompletionCallback in a less sophisticated way. > > > > 3. TestCompletionCallback is used much more widely in the entire code > base. > > > > > > > > Anyways, if you believe this class is necessary, I'm OK with this code at > > > least > > > > it works and I'm not the engineer who is responsible to maintain this code > > > until > > > > the end of its lifetime. > > > > > > I think we are talking about two different things. IIUC you want to use > > > TestCompletionCallback here. But we can't do that, as I said before. In case > > of > > > line #211, we are checking the second call. > > Sorry, I forgot to note that inner_read_requested_events_ is the exception. > > This vector is serving for a totally different purpose while other vectors can > > be easily replaced with TestCompletionCallback. > > 1. inner_read_requested_events_ can't be replaced. We want to check if it is > called only once. TestCompletionCallback doesn't allow that. > 2. read_result_events can't be replaced - called more than once. In #211. You can do it because you are passing a new callback made by Bind for each method call. > 3. get_length_result_events could be replaced - called up to once, but I think > it is better to check if it is not called more times than expected. > > So, IIUC, at most one of these three vectors can be replaced by > TestCompletionCallback. > Note, that I'm going to implement adaptive chunk size very soon, as written in > the TODO. > After doing that, we will have more complex test cases, which would require > multiple callback calls. You can use TestCompletionCallback multiple times by passing a callback returned by TestCompletionCallback::callback() for each method call. If the test becomes more complex with this "logger", you'll end up having something like: ... ASSERT_EQ(5u, logger.xxx().size()); EXPECT_EQ(kAAA, logger.xxx()[4]); [Do something] ASSERT_EQ(6u, logger.xxx().size()); EXPECT_EQ(kBBB, logger.xxx()[5]); [Do something] ASSERT_EQ(7u, logger.xxx().size()); EXPECT_EQ(kCCC, logger.xxx()[6]); ... The problem is that when you want to remove just one of these steps, you also have to rewrite all the following steps to fix indices. I'm still not sure why you chose this design among the all possible solutions. You can use TestCompletionCallback, or implement a subclass of TestCompletionCallback which overrides SetResult() to check the callback doesn't get called twice (if you really care about it), or do anything else. Anyways, I'm OK with this "logger" as long as you will be responsible to maintain this code as I said. > > I can split the class to separate utility methods like you suggested before, and > declare vectors locally. That would however end up on declaring the same vectors > in several test cases. I'm not convinced it is more readable. But let's give it > a try. > > > > > > > If we can't use TestCompletionCallback, then we would have to use suggested > by > > > you PushResultToVector, with local vectors. Do you suggest to use > > > TestCompletionCallback for callbacks which we assume are called once, and > > > PushResultToVector for callbacks which do not have such assumption? We would > > > save some lines of code, but isn't having a simple Logger class just neater > > and > > > cleaner? WDYT? > > The problem of this class's design is that there is no merit to have > > read_result_events_ and get_length_result_events_ in this class. > > These two vectors and related methods can be replaced with > > TestCompletionCallback or local variables and free functions without any > > problems. > > > Is there any reason with which you need to put the vector for recording the > > arguments of function calls made against the inner reader and the vectors for > > recording the result of function calls made against the external reader? > > I'm not sure if I understand. We should test if the external reader calls the > internal reader properly. Eg. we want to know if it was called with correct > arguments (eg. here with larger buffer length). > > Also, I'm not recording calls made against the external reader. I'm recording > the external Read and GetLength callback calls to know if they return correct > value. > > Please clarify. > > > > > > > As for the weak pointer, I'll remove it. Catching the crash over ignoring > > sounds > > > good to me. > > > https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/10 12:23:55, mtomasz wrote: > On 2014/06/10 10:43:24, hashimoto wrote: > > On 2014/06/09 13:51:15, mtomasz wrote: > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > How about using TestCompletionCallback? > > > > > > > > The same goes for other places passing Logger methods as > callbacks. > > > > > > > > > > > > > > With TestCompletionCallback we can't control how many times the > > callback > > > > has > > > > > > > been called. > > > > > > > > > > > > What do you mean? > > > > > > TestCompletionCallback::WaitForResult() quits the message loop once > the > > > > > callback > > > > > > gets called. > > > > > > > > > > If the tested class by accident calls the callback multiple times > instead > > of > > > > > one, then we won't catch this error with TestCompletionCallback. > > > > At least, we can catch it because it crashes as you described. > > > > > > It would crash. But I don't think crashing is better that detecting it with > an > > > assert. > > > Also, we are checking the second result in some places. With > > > TestCompletionCallback we can't do that. > > You cannot prepare for every possible failure which can be caused by the > tested > > class. > > What if, say, the tested class posts a task to base::WorkerPool and run the > > callback on a different thread than the main thread? > > Anyways, as I said, I'm OK with this "logger" as long as you take the > > responsibility to maintain this code. > > I cannot test everything, but I'm not sure if it is an argument to reduce test > coverage. This doesn't affect the test coverage (i.e. how much code of the tested class is actually covered by the test cases). Just the simplicity of the test code itself. > > > > > > > > > > > > > > Such error would happen, if we had an error in: > > > > > > > > > > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > > > > > GOOD: > > > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > > > > > BAD: > > > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > > > > > Using TestCompletionCallback would not let us catch this serious (and > easy > > > to > > > > > make) mistake. > > > > This can be caught by the synchronous version tests. > > > > > > > > BTW, why do you need to have a code path and parameterized tests to handle > > > > results returned synchronously? > > > > Doesn't providing extensions always return the result asynchronously? > > > > > > The BufferingFileStreamReader works with any webkit_blob::FileStreamReader, > > and > > > it has zero dependency on the underlying file stream reader. It makes it > > simple. > > > > > > We can of course remove the synchronous code paths, and add dependency and > > > assumptions on the implementation of the provided file system reader. > > > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of code. > Is > > > it really worth? > > Didn't you say that only one single "if" statement can be a prone of bugs? > > Also, please don't forget about the complicated test code and TEST_P used here > > which often degrades the readability of test failure log quite much. > > BufferingFileStreamReader is a thin wrapper. Supporting sync features doesn't > add much complexity. Removing sync support would add DCHECKs and possible > undefined behaviour in the future, if the inner reader returns something > synchronously. > > I always prefer to avoid writing code which is not used, but here in the thin > wrapper, it seems just less bug prone to support all features than cut out some > of them. In return we are getting zero dependency, which is a good thing. The existence of this buffer reader itself is depending on a huge assumption. It can be justified to have this buffering mechanism only if the actual read operation is quite slow so that the cost for the redundant data copy becomes negligible. Anyways, if you think having this unused code is acceptable, I'm OK with it as long as you will be responsible to maintain it. > > I don't know about log degradation when using TEST_P. Test case names are very > neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/10 12:23:55, mtomasz wrote: > On 2014/06/10 10:43:24, hashimoto wrote: > > On 2014/06/10 08:38:42, mtomasz wrote: > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > How about adding here something like: > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, callback); > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > > > > |buffer_length| > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded > currently > > > to > > > > > > 32KB. However, if it happened that it is larger than |buffer_size| > > (512KB) > > > > and > > > > > > we would have this extra-if, then we could crash the renderer process, > > > since > > > > > the > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" > > 128MB? > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk size from > > > 32KB > > > > to > > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > > If it was the purpose, why don't you add a static assert against it? > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of course > > > sense. > > > > But how do you want to use a static assert here? Could you clarify? > > If it was really a purpose of this class, having a static assert to check that > > the chunk size is less than the IPC size limit will make your purpose clearer > > and prevent mysterious crash from happening when someone tries to increase the > > chunk size in future. > > Do you want to crash with a static assert in such situations? Why? We want to Static assert failure doesn't cause a crash. It causes a compile error. > let providers work, even if the |buffer_length| is too large. Didn't you say that the browser crashes when the buffer length is larger than the maximum IPC message size? > > > > > > > I thought about it again, and IMHO we should have a cap for the buffer size > as > > > it is. > > > > > > We can't simply add that if. If the inner buffer already has some data, we > > have > > > to copy them first, and then if |buffer_length - bytes_read| > buffer_size_, > > > then the rest directly. > > The code here returns the result without performing actual read when any data > is > > left in the buffer. > > We always try to read |buffer_length| from the internal buffer. If there is not > enough, then we return partial results. If nothing is read, then we prefetch and > return as much as we can. > > > Why do you think we need to change this behavior for direct read case? > > If we skip contents in the buffer, then we will lose data. We always *must* > return all data from the internal buffer, before calling the internal file > stream reader for more data. I'm not saying we should leave data unread in the buffer. What I'm saying is that we should just put a if before the Preload() and let the inner reader directly return the result when appropriate. > > > > > > > It is non trivial, and it will require a test case. But we won't use this > > code, > > Sorry, I don't understand why you are OK with supporting synchronous readers > and > > having test cases for it which we will never need, > > while you are feeling nervous about preparing for future chunk size increase > > which may possibly happen. > > Support for synchronous readers: > + Let's us remove dependency on the internal file stream reader implementation. > + Makes code simpler, and DCHECK free. Think of GetLength. DCHECK cannot cause a malfunction on the product while "if" can. GetLength() is nothing here. All complexity of this class is dedicated to Read(). > - Requires TEST_P. Test cases are same. Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in tests and the fake classes. > - We don't need it now. > > As for "preparing for future chunk size increase": > - Requires *new* test cases. > - We don't need it now. > - We will not need it in the future. (See below) > - May cause a lot of problems. (See below) > - We can easily add it later if needed. The problem here is that it's hard to know when it's needed. When the file system provided by extensions are slow, it's hard to know which part is the bottleneck. It can be the cloud service, the extension implementation, IPC between the provider extension and the browser process, data handling in the browser process, or Files.app. > > I honestly can't see any advantages. > > > > because of the current buffer length size restriction. If it grows to large > > size > > > in the future, then fetching a lot of data directly from providing > extensions > > > will cause request timeouts in case of remote providing extensions. > > If it was the case, shouldn't we allow providing extensions to decide how to > > behave? > > (e.g. Depending on their servers' capability, capping the chunk size and > > returning smaller amount of data for read request.) > > 1. They already can return data in chunks, so eg. that 512KB they can split to > eg. 10x51KB. > 2. If we add this "if", then the buffer size may be any in the future. Every > developer has to add logic for splitting it. But, I predict that most of them > will not. So once we increase the buffer size in Chromium, all remote extensions > will start timeouting, because a lot of developers assumed (by observation) that > the buffer size is always 512KB. We shouldn't care about developers who depend on an undocumented behavior. > 3. If the buffer is too large, then it will affect copying time between remote > providing extensions and between them and Drive. We can't start uploading, > before the first chunk is fully downloaded. This means even 2x slower copy if > the file size is around the buffer size. Our current drive implementation doesn't start uploading until the file gets closed. > 4. Similarly, for too big buffer size, videos will load with a significant > delay. > 5. For big buffer size, copying progress bars will be very unreliable. > > Saying that, I'm still not convinced how having a large buffer, like couple of > megabytes could be a good thing in general case. I mentioned several serious > problems above, and probably there is much more. It's not you who makes this decision. Anyone can modify the user code in fileapi layer to change the buffer size with a good rationale without consulting you. > > > Also, isn't 512KB already large enough to cause timeout with poor network > > connection? > > It may be too big. I'm thinking of a solution for this, eg. a dialog like "This > operation takes longer than expected. Do you want to wait for it or abort?". Why do you need to implement a new UI when the operation is aborted thanks to the timeout? > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > Hence, I added a comment to make it clear that this class fetches data from > > the > > > underlying file stream reader in chunks of up to |buffer_length| size. > > >
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:56: base::WeakPtr<Logger> GetWeakPtr() { return weak_ptr_factory_.GetWeakPtr(); } On 2014/06/11 06:01:03, hashimoto wrote: > On 2014/06/10 12:23:55, mtomasz wrote: > > On 2014/06/10 10:43:24, hashimoto wrote: > > > On 2014/06/09 13:51:15, mtomasz wrote: > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > On 2014/06/06 01:31:03, mtomasz wrote: > > > > > > > > On 2014/06/05 10:07:10, hashimoto wrote: > > > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > > > No need to have this method? > > > > > > > > > > > I don't see any danger in using base::Unretained as Logger > > > always > > > > > > lives > > > > > > > > > longer > > > > > > > > > > > than the reader. > > > > > > > > > > > > > > > > > > > > We are passing it to the BufferingFileStreamReader, which can > > call > > > > it > > > > > > back > > > > > > > > > with > > > > > > > > > > a PostTask. Using Unretained wouldn't crash, only because we > (1) > > > > call > > > > > > > > > > RunLoop::RunUntilIdle and (2) the current > > TestBrowserThreadBundle > > > > > > creates > > > > > > > > fake > > > > > > > > > > threads backed up with the same message loop. If we had real > > > > threads, > > > > > > then > > > > > > > > > this > > > > > > > > > > could crash (IIUC). I think having a weak ptr is just a safe > > > choice, > > > > > > since > > > > > > > > its > > > > > > > > > > very simple and will always work fine. It doesn't add much > > > overhead > > > > > > > either. > > > > > > > > > > WDYT? > > > > > > > > > > > > > > > > > > I think it's OK to take advantage of the single threaded test > and > > > > using > > > > > > > > WeakPtr > > > > > > > > > looks like a kind of overkill. > > > > > > > > > Also, we can even stop Logger for recording results like below. > > > > > > > > > (the only exception is recording the method calls for the > internal > > > > > reader) > > > > > > > > > > > > > > > > > > void PushResultToVector(std::vector<int>* out, int result) { > > > > > > > > > out->push_back(result); > > > > > > > > > } > > > > > > > > > > > > > > > > > > TEST(...) { > > > > > > > > > std::vector<int> results; > > > > > > > > > reader.Read(buffer, kChunkSize, > &base::Bind(&PushResultToVector, > > > > > > > &results)); > > > > > > > > > } > > > > > > > > > > > > > > > > > > Anyways, it's up to you. > > > > > > > > > > > > > > > > Even if all threads are backed by the same message loop, I think > it > > is > > > > not > > > > > > > safe > > > > > > > > to use base::Unretained. It may happen that tested classes (1) > store > > > the > > > > > > > > callback and run it via PostTask in their destructor, *or* (2) we > > > forget > > > > > to > > > > > > > run > > > > > > > > RunLoopUntilIdle(). As a result, a task will be added to the > message > > > > loop, > > > > > > and > > > > > > > > executed *after* the test case is finished. When > > > TestBrowserThreadBundle > > > > > is > > > > > > > > destroyed, all message loops are drained, and then we would get a > > > crash: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/tes... > > > > > > > (1) Here we know that doesn't happen with the tested class. > > > > > > > (2) To report what happened, tests should behave nicely even when > the > > > > tested > > > > > > > class behaves badly, but forgetting to call Run() is a bug of the > test > > > > > itself. > > > > > > > > > > > > > There is no point to add another complexity to be robust against > badly > > > > > > > implemented tests. > > > > > > > > > > > > > > > > This is not likely to happen in this case, but in general I think > we > > > > can't > > > > > > > > safely assume that we can use base::Unretained in unit tests in a > > > single > > > > > > > > threaded environment. Saying that, always using a weak pointer > gives > > > us > > > > a > > > > > > > > guarantee that it will always work, and never crash, whatever is > the > > > > > > > > implementation of tested classes, and whether we forget to call > > > > RunLoop() > > > > > or > > > > > > > > not. And it costs us only 1 extra line of code. > > > > > > > Not only one line, but also the vectors and base::Bind. > > > > > > > Basically, you are reinventing TestCopmletionCallback. > > > > > > > > > > > > Your suggestion also uses vectors and base::Bind, but without the > class > > > > > wrapper. > > > > > > I can't use TestCompletionCallback, since it doesn't register multiple > > > > calls. > > > > > > Saying that, I don't think having a logger class is such a big > overhead. > > > It > > > > is > > > > > > self documenting and wrapping all of the logging callbacks and vectors > > in > > > > one > > > > > > place. > > > > > > > > > > > > If you don't have a strong feeling about this, I'd like to go with the > > > > current > > > > > > solution, as all of the tests in chrome/browser/file_system_provider > use > > > the > > > > > > same pattern. > > > > > I still don't understand why you need this class: > > > > > 1. When an unexpected call is made against the callback, it's better to > > get > > > > > notified about it with a crash instead of the call being silently > ignored > > by > > > > > WeakPtr. (A crash in a test is worse than a graceful failure because it > > > aborts > > > > > the test process, but better than silently dismissing hidden failures.) > > > > > 2. Too many things are repeated. 3 std::vectors, 3 same-looking methods > > to > > > > push > > > > > the result, 3 getters, lengthy base::Bind everywhere. This class is > > > basically > > > > a > > > > > reinvention of TestCompletionCallback in a less sophisticated way. > > > > > 3. TestCompletionCallback is used much more widely in the entire code > > base. > > > > > > > > > > Anyways, if you believe this class is necessary, I'm OK with this code > at > > > > least > > > > > it works and I'm not the engineer who is responsible to maintain this > code > > > > until > > > > > the end of its lifetime. > > > > > > > > I think we are talking about two different things. IIUC you want to use > > > > TestCompletionCallback here. But we can't do that, as I said before. In > case > > > of > > > > line #211, we are checking the second call. > > > Sorry, I forgot to note that inner_read_requested_events_ is the exception. > > > This vector is serving for a totally different purpose while other vectors > can > > > be easily replaced with TestCompletionCallback. > > > > 1. inner_read_requested_events_ can't be replaced. We want to check if it is > > called only once. TestCompletionCallback doesn't allow that. > > 2. read_result_events can't be replaced - called more than once. In #211. > You can do it because you are passing a new callback made by Bind for each > method call. > > 3. get_length_result_events could be replaced - called up to once, but I think > > it is better to check if it is not called more times than expected. > > > > So, IIUC, at most one of these three vectors can be replaced by > > TestCompletionCallback. > > Note, that I'm going to implement adaptive chunk size very soon, as written in > > the TODO. > > After doing that, we will have more complex test cases, which would require > > multiple callback calls. > You can use TestCompletionCallback multiple times by passing a callback returned > by TestCompletionCallback::callback() for each method call. > > If the test becomes more complex with this "logger", you'll end up having > something like: > > ... > ASSERT_EQ(5u, logger.xxx().size()); > EXPECT_EQ(kAAA, logger.xxx()[4]); > [Do something] > ASSERT_EQ(6u, logger.xxx().size()); > EXPECT_EQ(kBBB, logger.xxx()[5]); > [Do something] > ASSERT_EQ(7u, logger.xxx().size()); > EXPECT_EQ(kCCC, logger.xxx()[6]); > ... > > The problem is that when you want to remove just one of these steps, you also > have to rewrite all the following steps to fix indices. > I'm still not sure why you chose this design among the all possible solutions. > You can use TestCompletionCallback, or implement a subclass of > TestCompletionCallback which overrides SetResult() to check the callback doesn't > get called twice (if you really care about it), or do anything else. > Anyways, I'm OK with this "logger" as long as you will be responsible to > maintain this code as I said. > > > > > I can split the class to separate utility methods like you suggested before, > and > > declare vectors locally. That would however end up on declaring the same > vectors > > in several test cases. I'm not convinced it is more readable. But let's give > it > > a try. > > > > > > > > > > If we can't use TestCompletionCallback, then we would have to use > suggested > > by > > > > you PushResultToVector, with local vectors. Do you suggest to use > > > > TestCompletionCallback for callbacks which we assume are called once, and > > > > PushResultToVector for callbacks which do not have such assumption? We > would > > > > save some lines of code, but isn't having a simple Logger class just > neater > > > and > > > > cleaner? WDYT? > > > The problem of this class's design is that there is no merit to have > > > read_result_events_ and get_length_result_events_ in this class. > > > These two vectors and related methods can be replaced with > > > TestCompletionCallback or local variables and free functions without any > > > problems. > > > > > Is there any reason with which you need to put the vector for recording the > > > arguments of function calls made against the inner reader and the vectors > for > > > recording the result of function calls made against the external reader? > > > > I'm not sure if I understand. We should test if the external reader calls the > > internal reader properly. Eg. we want to know if it was called with correct > > arguments (eg. here with larger buffer length). > > > > Also, I'm not recording calls made against the external reader. I'm recording > > the external Read and GetLength callback calls to know if they return correct > > value. > > > > Please clarify. > > > > > > > > > > As for the weak pointer, I'll remove it. Catching the crash over ignoring > > > sounds > > > > good to me. > > > > > > I made the change as you suggested for comparison. https://codereview.chromium.org/334593003/diff2/1:20001/chrome/browser/chrome... Please note how tricky the new code is. We can't use GetResult() in some places since it could cause a freeze (if the callback is not called but was expected). We need to check for have_result() or called_times() before comparing the result, since it would be undefined (sic!). In comparison, the original code is very simple and very easy to understand. No tricks at all. And after all, shorter.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/11 06:01:03, hashimoto wrote: > On 2014/06/10 12:23:55, mtomasz wrote: > > On 2014/06/10 10:43:24, hashimoto wrote: > > > On 2014/06/09 13:51:15, mtomasz wrote: > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > How about using TestCompletionCallback? > > > > > > > > > The same goes for other places passing Logger methods as > > callbacks. > > > > > > > > > > > > > > > > With TestCompletionCallback we can't control how many times the > > > callback > > > > > has > > > > > > > > been called. > > > > > > > > > > > > > > What do you mean? > > > > > > > TestCompletionCallback::WaitForResult() quits the message loop once > > the > > > > > > callback > > > > > > > gets called. > > > > > > > > > > > > If the tested class by accident calls the callback multiple times > > instead > > > of > > > > > > one, then we won't catch this error with TestCompletionCallback. > > > > > At least, we can catch it because it crashes as you described. > > > > > > > > It would crash. But I don't think crashing is better that detecting it > with > > an > > > > assert. > > > > Also, we are checking the second result in some places. With > > > > TestCompletionCallback we can't do that. > > > You cannot prepare for every possible failure which can be caused by the > > tested > > > class. > > > What if, say, the tested class posts a task to base::WorkerPool and run the > > > callback on a different thread than the main thread? > > > Anyways, as I said, I'm OK with this "logger" as long as you take the > > > responsibility to maintain this code. > > > > I cannot test everything, but I'm not sure if it is an argument to reduce test > > coverage. > This doesn't affect the test coverage (i.e. how much code of the tested class is > actually covered by the test cases). > Just the simplicity of the test code itself. It affects how detailed the tests are. IMHO tests should be precise if possible. If we can *easily* test that the callback is not called more times than expected, then we should do that. Argument for losing precision just in order to be able to use TestCompletionCallback doesn't hold water for me. If we lose precision, then it means that TestCompletionCallback is not a correct tool for these tests. > > > > > > > > > > > > > > > > > > Such error would happen, if we had an error in: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > > > > > > > GOOD: > > > > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > > > > > > > BAD: > > > > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > > > > > > > Using TestCompletionCallback would not let us catch this serious (and > > easy > > > > to > > > > > > make) mistake. > > > > > This can be caught by the synchronous version tests. > > > > > > > > > > BTW, why do you need to have a code path and parameterized tests to > handle > > > > > results returned synchronously? > > > > > Doesn't providing extensions always return the result asynchronously? > > > > > > > > The BufferingFileStreamReader works with any > webkit_blob::FileStreamReader, > > > and > > > > it has zero dependency on the underlying file stream reader. It makes it > > > simple. > > > > > > > > We can of course remove the synchronous code paths, and add dependency and > > > > assumptions on the implementation of the provided file system reader. > > > > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of > code. > > Is > > > > it really worth? > > > Didn't you say that only one single "if" statement can be a prone of bugs? > > > Also, please don't forget about the complicated test code and TEST_P used > here > > > which often degrades the readability of test failure log quite much. > > > > BufferingFileStreamReader is a thin wrapper. Supporting sync features doesn't > > add much complexity. Removing sync support would add DCHECKs and possible > > undefined behaviour in the future, if the inner reader returns something > > synchronously. > > > > I always prefer to avoid writing code which is not used, but here in the thin > > wrapper, it seems just less bug prone to support all features than cut out > some > > of them. In return we are getting zero dependency, which is a good thing. > The existence of this buffer reader itself is depending on a huge assumption. > It can be justified to have this buffering mechanism only if the actual read > operation is quite slow so that the cost for the redundant data copy becomes > negligible. > Anyways, if you think having this unused code is acceptable, I'm OK with it as > long as you will be responsible to maintain it. According to the benchmarks I've shared with you in the caching plans document, an extra copy here doesn't make any significant difference here. > > > > I don't know about log degradation when using TEST_P. Test case names are very > > neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc. > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/11 06:01:03, hashimoto wrote: > On 2014/06/10 12:23:55, mtomasz wrote: > > On 2014/06/10 10:43:24, hashimoto wrote: > > > On 2014/06/10 08:38:42, mtomasz wrote: > > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > How about adding here something like: > > > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, > callback); > > > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > > > > > |buffer_length| > > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded > > currently > > > > to > > > > > > > 32KB. However, if it happened that it is larger than |buffer_size| > > > (512KB) > > > > > and > > > > > > > we would have this extra-if, then we could crash the renderer > process, > > > > since > > > > > > the > > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" > > > 128MB? > > > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk size > from > > > > 32KB > > > > > to > > > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > > > If it was the purpose, why don't you add a static assert against it? > > > > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of > course > > > > sense. > > > > > But how do you want to use a static assert here? Could you clarify? > > > If it was really a purpose of this class, having a static assert to check > that > > > the chunk size is less than the IPC size limit will make your purpose > clearer > > > and prevent mysterious crash from happening when someone tries to increase > the > > > chunk size in future. > > > > Do you want to crash with a static assert in such situations? Why? We want to > Static assert failure doesn't cause a crash. It causes a compile error. We can't use them then. We need to compile Chrome, even if the buffer is larger. Buffer size is same for any file stream reader. BufferingFileStreamReader just adjusts it to our preferred size - which is now 512 KB. > > let providers work, even if the |buffer_length| is too large. > Didn't you say that the browser crashes when the buffer length is larger than > the maximum IPC message size? These things are not related. BufferedFileStreamReader works if BufferedFileStreamReader::Read's |buffer_length| is larger than max IPC message size, since the *inner file stream reader's* Read's would be called with |buffer_length| up to 512KB size, which is smaller than IPC message size. If we introduced the if-statement you suggested, then we could crash. > > > > > > > > > > I thought about it again, and IMHO we should have a cap for the buffer > size > > as > > > > it is. > > > > > > > > We can't simply add that if. If the inner buffer already has some data, we > > > have > > > > to copy them first, and then if |buffer_length - bytes_read| > > buffer_size_, > > > > then the rest directly. > > > The code here returns the result without performing actual read when any > data > > is > > > left in the buffer. > > > > We always try to read |buffer_length| from the internal buffer. If there is > not > > enough, then we return partial results. If nothing is read, then we prefetch > and > > return as much as we can. > > > > > Why do you think we need to change this behavior for direct read case? > > > > If we skip contents in the buffer, then we will lose data. We always *must* > > return all data from the internal buffer, before calling the internal file > > stream reader for more data. > I'm not saying we should leave data unread in the buffer. > What I'm saying is that we should just put a if before the Preload() and let the > inner reader directly return the result when appropriate. > > > > > > > > > > It is non trivial, and it will require a test case. But we won't use this > > > code, > > > Sorry, I don't understand why you are OK with supporting synchronous readers > > and > > > having test cases for it which we will never need, > > > while you are feeling nervous about preparing for future chunk size increase > > > which may possibly happen. > > > > Support for synchronous readers: > > + Let's us remove dependency on the internal file stream reader > implementation. > > + Makes code simpler, and DCHECK free. Think of GetLength. > DCHECK cannot cause a malfunction on the product while "if" can. > GetLength() is nothing here. All complexity of this class is dedicated to > Read(). > > - Requires TEST_P. Test cases are same. > Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in tests and > the fake classes. Only one place in test cases. For the fake class, yes we have more, but they are pretty trivial. > > - We don't need it now. > > > > As for "preparing for future chunk size increase": > > - Requires *new* test cases. > > - We don't need it now. > > - We will not need it in the future. (See below) > > - May cause a lot of problems. (See below) > > - We can easily add it later if needed. > The problem here is that it's hard to know when it's needed. > When the file system provided by extensions are slow, it's hard to know which > part is the bottleneck. > It can be the cloud service, the extension implementation, IPC between the > provider extension and the browser process, data handling in the browser > process, or Files.app. You are saying, that the bottleneck can be anywhere, and I agree. Hence, just blindly allowing a larger buffer (just in case) is IMHO not the correct solution for the bottleneck problem. Especially, currently increasing the buffer size over 512KB doesn't help and would move more responsibilities on developers. I'm working on a performance analysis tool in chrome://provided-file-systems, which would say time taken by each of the steps, and then we will be able to optimize the API well. > > > > I honestly can't see any advantages. > > > > > > because of the current buffer length size restriction. If it grows to > large > > > size > > > > in the future, then fetching a lot of data directly from providing > > extensions > > > > will cause request timeouts in case of remote providing extensions. > > > If it was the case, shouldn't we allow providing extensions to decide how to > > > behave? > > > (e.g. Depending on their servers' capability, capping the chunk size and > > > returning smaller amount of data for read request.) > > > > 1. They already can return data in chunks, so eg. that 512KB they can split to > > eg. 10x51KB. > > 2. If we add this "if", then the buffer size may be any in the future. Every > > developer has to add logic for splitting it. But, I predict that most of them > > will not. So once we increase the buffer size in Chromium, all remote > extensions > > will start timeouting, because a lot of developers assumed (by observation) > that > > the buffer size is always 512KB. > We shouldn't care about developers who depend on an undocumented behavior. Shouldn't we? A lot of developers if they see that the buffer is always 512KB, will not split it in smaller requests. If you don't want to care about such developers, then a lot of apps in the Chrome store will be super flaky. And IMHO we don't want this to happen. I believe we should make the API as simple as possible, so developers do not have much chance to break things. And still, we can't expect developers to write perfect code. That's why we have timing out logic in the RequestManager, in case developers do not handle an exception, or forget to call a callback. > > 3. If the buffer is too large, then it will affect copying time between remote > > providing extensions and between them and Drive. We can't start uploading, > > before the first chunk is fully downloaded. This means even 2x slower copy if > > the file size is around the buffer size. > Our current drive implementation doesn't start uploading until the file gets > closed. AFAIR we are going to fix it (crbug.com/126902). Also, file stream providers will support streamed uploading from beginning. > > 4. Similarly, for too big buffer size, videos will load with a significant > > delay. > > 5. For big buffer size, copying progress bars will be very unreliable. > > > > Saying that, I'm still not convinced how having a large buffer, like couple of > > megabytes could be a good thing in general case. I mentioned several serious > > problems above, and probably there is much more. > It's not you who makes this decision. > Anyone can modify the user code in fileapi layer to change the buffer size with > a good rationale without consulting you. I'm not sure if I understand your point. BufferedFileSreamReader sets the buffer to our preferred one, which is 512KB, and we don't care about the buffer size set in the fileapi layer. > > > > > Also, isn't 512KB already large enough to cause timeout with poor network > > > connection? > > > > It may be too big. I'm thinking of a solution for this, eg. a dialog like > "This > > operation takes longer than expected. Do you want to wait for it or abort?". > Why do you need to implement a new UI when the operation is aborted thanks to > the timeout? In case HTTP connection is very slow, and the providing extension didn't finish fetching chunk of data from the server. As you said, it may happen. > > > > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > > > Hence, I added a comment to make it clear that this class fetches data > from > > > the > > > > underlying file stream reader in chunks of up to |buffer_length| size. > > > > > >
@hashimoto: Ping.
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/16 04:12:17, mtomasz wrote: > On 2014/06/11 06:01:03, hashimoto wrote: > > On 2014/06/10 12:23:55, mtomasz wrote: > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > On 2014/06/09 13:51:15, mtomasz wrote: > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > > How about using TestCompletionCallback? > > > > > > > > > > The same goes for other places passing Logger methods as > > > callbacks. > > > > > > > > > > > > > > > > > > With TestCompletionCallback we can't control how many times the > > > > callback > > > > > > has > > > > > > > > > been called. > > > > > > > > > > > > > > > > What do you mean? > > > > > > > > TestCompletionCallback::WaitForResult() quits the message loop > once > > > the > > > > > > > callback > > > > > > > > gets called. > > > > > > > > > > > > > > If the tested class by accident calls the callback multiple times > > > instead > > > > of > > > > > > > one, then we won't catch this error with TestCompletionCallback. > > > > > > At least, we can catch it because it crashes as you described. > > > > > > > > > > It would crash. But I don't think crashing is better that detecting it > > with > > > an > > > > > assert. > > > > > Also, we are checking the second result in some places. With > > > > > TestCompletionCallback we can't do that. > > > > You cannot prepare for every possible failure which can be caused by the > > > tested > > > > class. > > > > What if, say, the tested class posts a task to base::WorkerPool and run > the > > > > callback on a different thread than the main thread? > > > > Anyways, as I said, I'm OK with this "logger" as long as you take the > > > > responsibility to maintain this code. > > > > > > I cannot test everything, but I'm not sure if it is an argument to reduce > test > > > coverage. > > This doesn't affect the test coverage (i.e. how much code of the tested class > is > > actually covered by the test cases). > > Just the simplicity of the test code itself. > > It affects how detailed the tests are. IMHO tests should be precise if possible. > If we can *easily* test that the callback is not called more times than > expected, then we should do that. > > Argument for losing precision just in order to be able to use > TestCompletionCallback doesn't hold water for me. If we lose precision, then it > means that TestCompletionCallback is not a correct tool for these tests. TestCompletionCallback is widely used and it makes the test body short and easy to understand for someone not familiar to this code. How easy it is to understand existing tests, to add new tests, and to fix broken tests is quite important part of the quality of the test. Also, if you really care about this problem, you can create a subclass of TestCompletionCallback to have that check as I said in https://codereview.chromium.org/334593003/. > > > > > > > > > > > > > > > > > > > > > > > Such error would happen, if we had an error in: > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > > > > > > > > > GOOD: > > > > > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > > > > > > > > > BAD: > > > > > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > > > > > > > > > Using TestCompletionCallback would not let us catch this serious > (and > > > easy > > > > > to > > > > > > > make) mistake. > > > > > > This can be caught by the synchronous version tests. > > > > > > > > > > > > BTW, why do you need to have a code path and parameterized tests to > > handle > > > > > > results returned synchronously? > > > > > > Doesn't providing extensions always return the result asynchronously? > > > > > > > > > > The BufferingFileStreamReader works with any > > webkit_blob::FileStreamReader, > > > > and > > > > > it has zero dependency on the underlying file stream reader. It makes it > > > > simple. > > > > > > > > > > We can of course remove the synchronous code paths, and add dependency > and > > > > > assumptions on the implementation of the provided file system reader. > > > > > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of > > code. > > > Is > > > > > it really worth? > > > > Didn't you say that only one single "if" statement can be a prone of bugs? > > > > Also, please don't forget about the complicated test code and TEST_P used > > here > > > > which often degrades the readability of test failure log quite much. > > > > > > BufferingFileStreamReader is a thin wrapper. Supporting sync features > doesn't > > > add much complexity. Removing sync support would add DCHECKs and possible > > > undefined behaviour in the future, if the inner reader returns something > > > synchronously. > > > > > > I always prefer to avoid writing code which is not used, but here in the > thin > > > wrapper, it seems just less bug prone to support all features than cut out > > some > > > of them. In return we are getting zero dependency, which is a good thing. > > The existence of this buffer reader itself is depending on a huge assumption. > > It can be justified to have this buffering mechanism only if the actual read > > operation is quite slow so that the cost for the redundant data copy becomes > > negligible. > > Anyways, if you think having this unused code is acceptable, I'm OK with it as > > long as you will be responsible to maintain it. > > According to the benchmarks I've shared with you in the caching plans document, > an extra copy here doesn't make any significant difference here. What I meant was that when you cannot make some assumption on the code owned by you, how can you make assumption on the code now owned by you. Extension/IPC system eventually may get faster in future. > > > > > > > I don't know about log degradation when using TEST_P. Test case names are > very > > > neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc. > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/16 04:12:18, mtomasz wrote: > On 2014/06/11 06:01:03, hashimoto wrote: > > On 2014/06/10 12:23:55, mtomasz wrote: > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > On 2014/06/10 08:38:42, mtomasz wrote: > > > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > > How about adding here something like: > > > > > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, > > callback); > > > > > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value for > > > > > > > > |buffer_length| > > > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded > > > currently > > > > > to > > > > > > > > 32KB. However, if it happened that it is larger than |buffer_size| > > > > (512KB) > > > > > > and > > > > > > > > we would have this extra-if, then we could crash the renderer > > process, > > > > > since > > > > > > > the > > > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" > > > > 128MB? > > > > > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk size > > from > > > > > 32KB > > > > > > to > > > > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > > > > If it was the purpose, why don't you add a static assert against it? > > > > > > > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of > > course > > > > > sense. > > > > > > But how do you want to use a static assert here? Could you clarify? > > > > If it was really a purpose of this class, having a static assert to check > > that > > > > the chunk size is less than the IPC size limit will make your purpose > > clearer > > > > and prevent mysterious crash from happening when someone tries to increase > > the > > > > chunk size in future. > > > > > > Do you want to crash with a static assert in such situations? Why? We want > to > > Static assert failure doesn't cause a crash. It causes a compile error. > > We can't use them then. We need to compile Chrome, even if the buffer is larger. > Buffer size is same for any file stream reader. BufferingFileStreamReader just > adjusts it to our preferred size - which is now 512 KB. > > > > let providers work, even if the |buffer_length| is too large. > > Didn't you say that the browser crashes when the buffer length is larger than > > the maximum IPC message size? > > These things are not related. BufferedFileStreamReader works if > BufferedFileStreamReader::Read's |buffer_length| is larger than max IPC message > size, since the *inner file stream reader's* Read's would be called with > |buffer_length| up to 512KB size, which is smaller than IPC message size. If we > introduced the if-statement you suggested, then we could crash. What I meant is that if you really care about the 128MB IPC message size limit, you should assert that the chunk size used by this class (512KB currently) is smaller than the IPC size limit (128MB currently) so that it can fail in the compile stage rather than browser_tests when someone tries to change the chunk size to a value larger than 128MB. I don't think we should care about 128MB IPC message size limit though. > > > > > > > > > > > > > > I thought about it again, and IMHO we should have a cap for the buffer > > size > > > as > > > > > it is. > > > > > > > > > > We can't simply add that if. If the inner buffer already has some data, > we > > > > have > > > > > to copy them first, and then if |buffer_length - bytes_read| > > > buffer_size_, > > > > > then the rest directly. > > > > The code here returns the result without performing actual read when any > > data > > > is > > > > left in the buffer. > > > > > > We always try to read |buffer_length| from the internal buffer. If there is > > not > > > enough, then we return partial results. If nothing is read, then we prefetch > > and > > > return as much as we can. > > > > > > > Why do you think we need to change this behavior for direct read case? > > > > > > If we skip contents in the buffer, then we will lose data. We always *must* > > > return all data from the internal buffer, before calling the internal file > > > stream reader for more data. > > I'm not saying we should leave data unread in the buffer. > > What I'm saying is that we should just put a if before the Preload() and let > the > > inner reader directly return the result when appropriate. > > > > > > > > > > > > > It is non trivial, and it will require a test case. But we won't use > this > > > > code, > > > > Sorry, I don't understand why you are OK with supporting synchronous > readers > > > and > > > > having test cases for it which we will never need, > > > > while you are feeling nervous about preparing for future chunk size > increase > > > > which may possibly happen. > > > > > > Support for synchronous readers: > > > + Let's us remove dependency on the internal file stream reader > > implementation. > > > + Makes code simpler, and DCHECK free. Think of GetLength. > > DCHECK cannot cause a malfunction on the product while "if" can. > > GetLength() is nothing here. All complexity of this class is dedicated to > > Read(). > > > - Requires TEST_P. Test cases are same. > > Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in tests > and > > the fake classes. > > Only one place in test cases. For the fake class, yes we have more, but they are > pretty trivial. > > > > - We don't need it now. > > > > > > As for "preparing for future chunk size increase": > > > - Requires *new* test cases. > > > - We don't need it now. > > > - We will not need it in the future. (See below) > > > - May cause a lot of problems. (See below) > > > - We can easily add it later if needed. > > The problem here is that it's hard to know when it's needed. > > When the file system provided by extensions are slow, it's hard to know which > > part is the bottleneck. > > It can be the cloud service, the extension implementation, IPC between the > > provider extension and the browser process, data handling in the browser > > process, or Files.app. > > You are saying, that the bottleneck can be anywhere, and I agree. Hence, just > blindly allowing a larger buffer (just in case) is IMHO not the correct solution > for the bottleneck problem. Especially, currently increasing the buffer size > over 512KB doesn't help and would move more responsibilities on developers. > > I'm working on a performance analysis tool in chrome://provided-file-systems, > which would say time taken by each of the steps, and then we will be able to > optimize the API well. IIUC the purpose of this class was just to reduce the number of IPCs. Why did you suddenly start arguing that it should be capped to 512KB even when fileapi wants to read in a larger sized chunk? I don't see any problems in moving responsibilities to the extension when the larger size is requested. > > > > > > > I honestly can't see any advantages. > > > > > > > > because of the current buffer length size restriction. If it grows to > > large > > > > size > > > > > in the future, then fetching a lot of data directly from providing > > > extensions > > > > > will cause request timeouts in case of remote providing extensions. > > > > If it was the case, shouldn't we allow providing extensions to decide how > to > > > > behave? > > > > (e.g. Depending on their servers' capability, capping the chunk size and > > > > returning smaller amount of data for read request.) > > > > > > 1. They already can return data in chunks, so eg. that 512KB they can split > to > > > eg. 10x51KB. > > > 2. If we add this "if", then the buffer size may be any in the future. Every > > > developer has to add logic for splitting it. But, I predict that most of > them > > > will not. So once we increase the buffer size in Chromium, all remote > > extensions > > > will start timeouting, because a lot of developers assumed (by observation) > > that > > > the buffer size is always 512KB. > > We shouldn't care about developers who depend on an undocumented behavior. > > Shouldn't we? A lot of developers if they see that the buffer is always 512KB, > will not split it in smaller requests. If you don't want to care about such > developers, then a lot of apps in the Chrome store will be super flaky. And IMHO > we don't want this to happen. The API documentation is the only contract made between Chromium and extension developers. If you think this should be 512KB forever (even after you stop maintaining this code and after technology advancement makes 512KB chunk size ridiculously small), you should write it in the API documentation. > > I believe we should make the API as simple as possible, so developers do not > have much chance to break things. And still, we can't expect developers to write > perfect code. That's why we have timing out logic in the RequestManager, in case > developers do not handle an exception, or forget to call a callback. Timeout should be there because, without it, wrongly implemented extensions can damage Chrome itself. > > > > 3. If the buffer is too large, then it will affect copying time between > remote > > > providing extensions and between them and Drive. We can't start uploading, > > > before the first chunk is fully downloaded. This means even 2x slower copy > if > > > the file size is around the buffer size. > > Our current drive implementation doesn't start uploading until the file gets > > closed. > > AFAIR we are going to fix it (crbug.com/126902). Also, file stream providers crbug.com/126902 is about upload for web sites like Gmail, not to Drive. > will support streamed uploading from beginning. Just mentioned to clarify that you shouldn't use our Drive client as an example here. > > > > 4. Similarly, for too big buffer size, videos will load with a significant > > > delay. > > > 5. For big buffer size, copying progress bars will be very unreliable. > > > > > > Saying that, I'm still not convinced how having a large buffer, like couple > of > > > megabytes could be a good thing in general case. I mentioned several serious > > > problems above, and probably there is much more. > > It's not you who makes this decision. > > Anyone can modify the user code in fileapi layer to change the buffer size > with > > a good rationale without consulting you. > > I'm not sure if I understand your point. BufferedFileSreamReader sets the buffer > to our preferred one, which is 512KB, and we don't care about the buffer size > set in the fileapi layer. This class's purpose is to reduce the number of IPCs, right? If the fileapi is changed to use a chunk size larger than 512KB, it means that we will end up doing the opposite thing, even after the technology advancement or something makes 512KB chunk size ridiculously small. Don't you want your code to work without problems for a decade or more? > > > > > > > > Also, isn't 512KB already large enough to cause timeout with poor network > > > > connection? > > > > > > It may be too big. I'm thinking of a solution for this, eg. a dialog like > > "This > > > operation takes longer than expected. Do you want to wait for it or abort?". > > Why do you need to implement a new UI when the operation is aborted thanks to > > the timeout? > > In case HTTP connection is very slow, and the providing extension didn't finish > fetching chunk of data from the server. As you said, it may happen. When the timeout happens, the operation gets aborted automatically. You don't need to implement any new UI for it. > > > > > > > > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > > > > > Hence, I added a comment to make it clear that this class fetches data > > from > > > > the > > > > > underlying file stream reader in chunks of up to |buffer_length| size. > > > > > > > > > >
https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/20001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:155: base::Bind(&Logger::OnReadResult, logger.GetWeakPtr())); On 2014/06/19 07:27:45, hashimoto wrote: > On 2014/06/16 04:12:17, mtomasz wrote: > > On 2014/06/11 06:01:03, hashimoto wrote: > > > On 2014/06/10 12:23:55, mtomasz wrote: > > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > > On 2014/06/09 13:51:15, mtomasz wrote: > > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > > On 2014/06/05 09:22:27, mtomasz wrote: > > > > > > > > > > On 2014/06/05 03:18:36, hashimoto wrote: > > > > > > > > > > > How about using TestCompletionCallback? > > > > > > > > > > > The same goes for other places passing Logger methods as > > > > callbacks. > > > > > > > > > > > > > > > > > > > > With TestCompletionCallback we can't control how many times > the > > > > > callback > > > > > > > has > > > > > > > > > > been called. > > > > > > > > > > > > > > > > > > What do you mean? > > > > > > > > > TestCompletionCallback::WaitForResult() quits the message loop > > once > > > > the > > > > > > > > callback > > > > > > > > > gets called. > > > > > > > > > > > > > > > > If the tested class by accident calls the callback multiple times > > > > instead > > > > > of > > > > > > > > one, then we won't catch this error with TestCompletionCallback. > > > > > > > At least, we can catch it because it crashes as you described. > > > > > > > > > > > > It would crash. But I don't think crashing is better that detecting it > > > with > > > > an > > > > > > assert. > > > > > > Also, we are checking the second result in some places. With > > > > > > TestCompletionCallback we can't do that. > > > > > You cannot prepare for every possible failure which can be caused by the > > > > tested > > > > > class. > > > > > What if, say, the tested class posts a task to base::WorkerPool and run > > the > > > > > callback on a different thread than the main thread? > > > > > Anyways, as I said, I'm OK with this "logger" as long as you take the > > > > > responsibility to maintain this code. > > > > > > > > I cannot test everything, but I'm not sure if it is an argument to reduce > > test > > > > coverage. > > > This doesn't affect the test coverage (i.e. how much code of the tested > class > > is > > > actually covered by the test cases). > > > Just the simplicity of the test code itself. > > > > It affects how detailed the tests are. IMHO tests should be precise if > possible. > > If we can *easily* test that the callback is not called more times than > > expected, then we should do that. > > > > Argument for losing precision just in order to be able to use > > TestCompletionCallback doesn't hold water for me. If we lose precision, then > it > > means that TestCompletionCallback is not a correct tool for these tests. > TestCompletionCallback is widely used and it makes the test body short and easy > to understand for someone not familiar to this code. > How easy it is to understand existing tests, to add new tests, and to fix broken > tests is quite important part of the quality of the test. > Also, if you really care about this problem, you can create a subclass of > TestCompletionCallback to have that check as I said in > https://codereview.chromium.org/334593003/. TestCompletionCallback makes the code nice and short but has side effects. As I said before, it causes (a) loss of precision and (b) timing out instead of asserts/expects. If we want to avoid (a) and (b), we can still use TestCompletionCallback, but the code gets complicated, and difficult to maintain. So, we have three options: 1. Keep Logger, hence detailed and never timeouting tests. The code is very simple and clean, hence easy to maintain. 2. Use TestCompletionCallback and lose precision (a) + have timing out unit tests instead of asserts/expects if a callback is not called (b). The code is simple and clean but with low precision, and we can't use it for the inner reader. For the inner reader we still need to have some kind of a logger. So we have also inconsistency. 3. Use TestCompletionCallback with overriding it + using tricks to avoid (a) and (b). Complex and difficult code to maintain. We can't use it for the inner reader, so we need some logger as in (2) additionally. In total, the code may be longer than (1). Since we already have the logger, I don't understand why are you pushing on using either (2) or (3), where both seem to be more problematic than (1). > > > > > > > > > > > > > > > > > > > > > > > > > > > > Such error would happen, if we had an error in: > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... > > > > > > > > > > > > > > > > GOOD: > > > > > > > > 79 if (result != net::ERR_IO_PENDING) { > > > > > > > > > > > > > > > > BAD: > > > > > > > > 79 if (result == net::ERR_IO_PENDING) { > > > > > > > > > > > > > > > > Using TestCompletionCallback would not let us catch this serious > > (and > > > > easy > > > > > > to > > > > > > > > make) mistake. > > > > > > > This can be caught by the synchronous version tests. > > > > > > > > > > > > > > BTW, why do you need to have a code path and parameterized tests to > > > handle > > > > > > > results returned synchronously? > > > > > > > Doesn't providing extensions always return the result > asynchronously? > > > > > > > > > > > > The BufferingFileStreamReader works with any > > > webkit_blob::FileStreamReader, > > > > > and > > > > > > it has zero dependency on the underlying file stream reader. It makes > it > > > > > simple. > > > > > > > > > > > > We can of course remove the synchronous code paths, and add dependency > > and > > > > > > assumptions on the implementation of the provided file system reader. > > > > > > Surprisingly, we would get 2 extra DCHECKS and remove only 3 lines of > > > code. > > > > Is > > > > > > it really worth? > > > > > Didn't you say that only one single "if" statement can be a prone of > bugs? > > > > > Also, please don't forget about the complicated test code and TEST_P > used > > > here > > > > > which often degrades the readability of test failure log quite much. > > > > > > > > BufferingFileStreamReader is a thin wrapper. Supporting sync features > > doesn't > > > > add much complexity. Removing sync support would add DCHECKs and possible > > > > undefined behaviour in the future, if the inner reader returns something > > > > synchronously. > > > > > > > > I always prefer to avoid writing code which is not used, but here in the > > thin > > > > wrapper, it seems just less bug prone to support all features than cut out > > > some > > > > of them. In return we are getting zero dependency, which is a good thing. > > > The existence of this buffer reader itself is depending on a huge > assumption. > > > It can be justified to have this buffering mechanism only if the actual read > > > operation is quite slow so that the cost for the redundant data copy becomes > > > negligible. > > > Anyways, if you think having this unused code is acceptable, I'm OK with it > as > > > long as you will be responsible to maintain it. > > > > According to the benchmarks I've shared with you in the caching plans > document, > > an extra copy here doesn't make any significant difference here. > What I meant was that when you cannot make some assumption on the code owned by > you, how can you make assumption on the code now owned by you. Extension/IPC > system eventually may get faster in future. Could you clarify? Why can't I make assumptions on the code owned by me? > > > > > > > > > > I don't know about log degradation when using TEST_P. Test case names are > > very > > > > neat. Synchronous/FileSystemProviderBufferingFileStreamTest.Read, etc. > > > > > > https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/19 07:27:45, hashimoto wrote: > On 2014/06/16 04:12:18, mtomasz wrote: > > On 2014/06/11 06:01:03, hashimoto wrote: > > > On 2014/06/10 12:23:55, mtomasz wrote: > > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > > On 2014/06/10 08:38:42, mtomasz wrote: > > > > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > > > How about adding here something like: > > > > > > > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, > > > callback); > > > > > > > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value > for > > > > > > > > > |buffer_length| > > > > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded > > > > currently > > > > > > to > > > > > > > > > 32KB. However, if it happened that it is larger than > |buffer_size| > > > > > (512KB) > > > > > > > and > > > > > > > > > we would have this extra-if, then we could crash the renderer > > > process, > > > > > > since > > > > > > > > the > > > > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * > 1024;" > > > > > 128MB? > > > > > > > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk > size > > > from > > > > > > 32KB > > > > > > > to > > > > > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > > > > > If it was the purpose, why don't you add a static assert against > it? > > > > > > > > > > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of > > > course > > > > > > sense. > > > > > > > But how do you want to use a static assert here? Could you clarify? > > > > > If it was really a purpose of this class, having a static assert to > check > > > that > > > > > the chunk size is less than the IPC size limit will make your purpose > > > clearer > > > > > and prevent mysterious crash from happening when someone tries to > increase > > > the > > > > > chunk size in future. > > > > > > > > Do you want to crash with a static assert in such situations? Why? We want > > to > > > Static assert failure doesn't cause a crash. It causes a compile error. > > > > We can't use them then. We need to compile Chrome, even if the buffer is > larger. > > Buffer size is same for any file stream reader. BufferingFileStreamReader just > > adjusts it to our preferred size - which is now 512 KB. > > > > > > let providers work, even if the |buffer_length| is too large. > > > Didn't you say that the browser crashes when the buffer length is larger > than > > > the maximum IPC message size? > > > > These things are not related. BufferedFileStreamReader works if > > BufferedFileStreamReader::Read's |buffer_length| is larger than max IPC > message > > size, since the *inner file stream reader's* Read's would be called with > > |buffer_length| up to 512KB size, which is smaller than IPC message size. If > we > > introduced the if-statement you suggested, then we could crash. > What I meant is that if you really care about the 128MB IPC message size limit, > you should assert that the chunk size used by this class (512KB currently) is > smaller than the IPC size limit (128MB currently) > so that it can fail in the compile stage rather than browser_tests when someone > tries to change the chunk size to a value larger than 128MB. > I don't think we should care about 128MB IPC message size limit though. > > > > > > > > > > > > > > > > > > I thought about it again, and IMHO we should have a cap for the buffer > > > size > > > > as > > > > > > it is. > > > > > > > > > > > > We can't simply add that if. If the inner buffer already has some > data, > > we > > > > > have > > > > > > to copy them first, and then if |buffer_length - bytes_read| > > > > buffer_size_, > > > > > > then the rest directly. > > > > > The code here returns the result without performing actual read when any > > > data > > > > is > > > > > left in the buffer. > > > > > > > > We always try to read |buffer_length| from the internal buffer. If there > is > > > not > > > > enough, then we return partial results. If nothing is read, then we > prefetch > > > and > > > > return as much as we can. > > > > > > > > > Why do you think we need to change this behavior for direct read case? > > > > > > > > If we skip contents in the buffer, then we will lose data. We always > *must* > > > > return all data from the internal buffer, before calling the internal file > > > > stream reader for more data. > > > I'm not saying we should leave data unread in the buffer. > > > What I'm saying is that we should just put a if before the Preload() and let > > the > > > inner reader directly return the result when appropriate. > > > > > > > > > > > > > > > > It is non trivial, and it will require a test case. But we won't use > > this > > > > > code, > > > > > Sorry, I don't understand why you are OK with supporting synchronous > > readers > > > > and > > > > > having test cases for it which we will never need, > > > > > while you are feeling nervous about preparing for future chunk size > > increase > > > > > which may possibly happen. > > > > > > > > Support for synchronous readers: > > > > + Let's us remove dependency on the internal file stream reader > > > implementation. > > > > + Makes code simpler, and DCHECK free. Think of GetLength. > > > DCHECK cannot cause a malfunction on the product while "if" can. > > > GetLength() is nothing here. All complexity of this class is dedicated to > > > Read(). > > > > - Requires TEST_P. Test cases are same. > > > Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in tests > > and > > > the fake classes. > > > > Only one place in test cases. For the fake class, yes we have more, but they > are > > pretty trivial. > > > > > > - We don't need it now. > > > > > > > > As for "preparing for future chunk size increase": > > > > - Requires *new* test cases. > > > > - We don't need it now. > > > > - We will not need it in the future. (See below) > > > > - May cause a lot of problems. (See below) > > > > - We can easily add it later if needed. > > > The problem here is that it's hard to know when it's needed. > > > When the file system provided by extensions are slow, it's hard to know > which > > > part is the bottleneck. > > > It can be the cloud service, the extension implementation, IPC between the > > > provider extension and the browser process, data handling in the browser > > > process, or Files.app. > > > > You are saying, that the bottleneck can be anywhere, and I agree. Hence, just > > blindly allowing a larger buffer (just in case) is IMHO not the correct > solution > > for the bottleneck problem. Especially, currently increasing the buffer size > > over 512KB doesn't help and would move more responsibilities on developers. > > > > I'm working on a performance analysis tool in chrome://provided-file-systems, > > which would say time taken by each of the steps, and then we will be able to > > optimize the API well. > IIUC the purpose of this class was just to reduce the number of IPCs. > Why did you suddenly start arguing that it should be capped to 512KB even when > fileapi wants to read in a larger sized chunk? > I don't see any problems in moving responsibilities to the extension when the > larger size is requested. I already answered this question. > > > > - Requires *new* test cases. > > > > - We don't need it now. > > > > - We will not need it in the future. (See below) > > > > - May cause a lot of problems. (See below) > > > > - We can easily add it later if needed. > > > > > > > > > > I honestly can't see any advantages. > > > > > > > > > > because of the current buffer length size restriction. If it grows to > > > large > > > > > size > > > > > > in the future, then fetching a lot of data directly from providing > > > > extensions > > > > > > will cause request timeouts in case of remote providing extensions. > > > > > If it was the case, shouldn't we allow providing extensions to decide > how > > to > > > > > behave? > > > > > (e.g. Depending on their servers' capability, capping the chunk size and > > > > > returning smaller amount of data for read request.) > > > > > > > > 1. They already can return data in chunks, so eg. that 512KB they can > split > > to > > > > eg. 10x51KB. > > > > 2. If we add this "if", then the buffer size may be any in the future. > Every > > > > developer has to add logic for splitting it. But, I predict that most of > > them > > > > will not. So once we increase the buffer size in Chromium, all remote > > > extensions > > > > will start timeouting, because a lot of developers assumed (by > observation) > > > that > > > > the buffer size is always 512KB. > > > We shouldn't care about developers who depend on an undocumented behavior. > > > > Shouldn't we? A lot of developers if they see that the buffer is always 512KB, > > will not split it in smaller requests. If you don't want to care about such > > developers, then a lot of apps in the Chrome store will be super flaky. And > IMHO > > we don't want this to happen. > The API documentation is the only contract made between Chromium and extension > developers. > If you think this should be 512KB forever (even after you stop maintaining this > code and after technology advancement makes 512KB chunk size ridiculously > small), you should write it in the API documentation. I'm not planning to write it in the documentation. Developers should chop the requests into chunks, and they shouldn't assume that the buffer size is fixed to any value. But I know that a lot of developers will not chop. Don't you think so? > > > > I believe we should make the API as simple as possible, so developers do not > > have much chance to break things. And still, we can't expect developers to > write > > perfect code. That's why we have timing out logic in the RequestManager, in > case > > developers do not handle an exception, or forget to call a callback. > Timeout should be there because, without it, wrongly implemented extensions can > damage Chrome itself. > > > > > > 3. If the buffer is too large, then it will affect copying time between > > remote > > > > providing extensions and between them and Drive. We can't start uploading, > > > > before the first chunk is fully downloaded. This means even 2x slower copy > > if > > > > the file size is around the buffer size. > > > Our current drive implementation doesn't start uploading until the file gets > > > closed. > > > > AFAIR we are going to fix it (crbug.com/126902). Also, file stream providers > crbug.com/126902 is about upload for web sites like Gmail, not to Drive. Got it. Do we have any plans to support streamed uploading in Drive then? > > will support streamed uploading from beginning. > Just mentioned to clarify that you shouldn't use our Drive client as an example > here. > > > > > > 4. Similarly, for too big buffer size, videos will load with a significant > > > > delay. > > > > 5. For big buffer size, copying progress bars will be very unreliable. > > > > > > > > Saying that, I'm still not convinced how having a large buffer, like > couple > > of > > > > megabytes could be a good thing in general case. I mentioned several > serious > > > > problems above, and probably there is much more. > > > It's not you who makes this decision. > > > Anyone can modify the user code in fileapi layer to change the buffer size > > with > > > a good rationale without consulting you. > > > > I'm not sure if I understand your point. BufferedFileSreamReader sets the > buffer > > to our preferred one, which is 512KB, and we don't care about the buffer size > > set in the fileapi layer. > This class's purpose is to reduce the number of IPCs, right? > If the fileapi is changed to use a chunk size larger than 512KB, it means that > we will end up doing the opposite thing, even after the technology advancement > or something makes 512KB chunk size ridiculously small. > Don't you want your code to work without problems for a decade or more? Purpose of this class is to set the buffer size to our preferred size. You are suggesting to write non-trivial code which will be most probably not used for years and will not improve performance. That's why I'm not convinced. But if you insist, I can add it. > > > > > > > > > > > Also, isn't 512KB already large enough to cause timeout with poor > network > > > > > connection? > > > > > > > > It may be too big. I'm thinking of a solution for this, eg. a dialog like > > > "This > > > > operation takes longer than expected. Do you want to wait for it or > abort?". > > > Why do you need to implement a new UI when the operation is aborted thanks > to > > > the timeout? > > > > In case HTTP connection is very slow, and the providing extension didn't > finish > > fetching chunk of data from the server. As you said, it may happen. > When the timeout happens, the operation gets aborted automatically. > You don't need to implement any new UI for it. We can't blindly timeout each request after 10 seconds. It may happen that a providing extension shows a dialog for credentials, and the operation would take minutes. We don't want to abort in such case. Please read the API proposals as well as the "File System Provider API Request Policy" (slightly outdated) I shared with the team. The timing out problem is described there in details. > > > > > > > > > > > > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > > > > > > > Hence, I added a comment to make it clear that this class fetches data > > > from > > > > > the > > > > > > underlying file stream reader in chunks of up to |buffer_length| size. > > > > > > > > > > > > > > >
@hashimoto: Ping.
Adding @kinaba, since @hashimoto seems busy in MTV. To sum up the long discussion, there are two issues: 1. Using TestCompletionCallback instead of a Logger class in same places for unit tests. (Some demo code for TestCompletionCallback: https://codereview.chromium.org/334593003/) 2. Allowing to fetch buffers of size > 512 KB from the BufferedFileStreamReader. I think all arguments have been already presented, so please take a look and decide, whichever you think is better. Thanks.
On 2014/06/27 00:59:44, mtomasz wrote: > Adding @kinaba, since @hashimoto seems busy in MTV. > > To sum up the long discussion, there are two issues: > > 1. Using TestCompletionCallback instead of a Logger class in same places for > unit tests. > (Some demo code for TestCompletionCallback: > https://codereview.chromium.org/334593003/) > > 2. Allowing to fetch buffers of size > 512 KB from the BufferedFileStreamReader. > > I think all arguments have been already presented, so please take a look and > decide, whichever you think is better. > > Thanks. Note, that since https://codereview.chromium.org/350683002/ is already committed, we shouldn't have problems with timeouting operations anymore, even if the chunk size is too large. Hence, I'm fine with addressing (2).
Sorry for being late to respond. Replying on this issue is becoming more like filling a tax form to me. https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; On 2014/06/20 04:20:23, mtomasz wrote: > On 2014/06/19 07:27:45, hashimoto wrote: > > On 2014/06/16 04:12:18, mtomasz wrote: > > > On 2014/06/11 06:01:03, hashimoto wrote: > > > > On 2014/06/10 12:23:55, mtomasz wrote: > > > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > > > On 2014/06/10 08:38:42, mtomasz wrote: > > > > > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > > > > How about adding here something like: > > > > > > > > > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, > > > > callback); > > > > > > > > > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big value > > for > > > > > > > > > > |buffer_length| > > > > > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > > > > > > > > > The buffer length is not user controlled, but it is hard-coded > > > > > currently > > > > > > > to > > > > > > > > > > 32KB. However, if it happened that it is larger than > > |buffer_size| > > > > > > (512KB) > > > > > > > > and > > > > > > > > > > we would have this extra-if, then we could crash the renderer > > > > process, > > > > > > > since > > > > > > > > > the > > > > > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * > > 1024;" > > > > > > 128MB? > > > > > > > > > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk > > size > > > > from > > > > > > > 32KB > > > > > > > > to > > > > > > > > > > 512KB, and to cap it to 512KB, so we won't crash the renderer. > > > > > > > > > If it was the purpose, why don't you add a static assert against > > it? > > > > > > > > > > > > > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes of > > > > course > > > > > > > sense. > > > > > > > > But how do you want to use a static assert here? Could you > clarify? > > > > > > If it was really a purpose of this class, having a static assert to > > check > > > > that > > > > > > the chunk size is less than the IPC size limit will make your purpose > > > > clearer > > > > > > and prevent mysterious crash from happening when someone tries to > > increase > > > > the > > > > > > chunk size in future. > > > > > > > > > > Do you want to crash with a static assert in such situations? Why? We > want > > > to > > > > Static assert failure doesn't cause a crash. It causes a compile error. > > > > > > We can't use them then. We need to compile Chrome, even if the buffer is > > larger. > > > Buffer size is same for any file stream reader. BufferingFileStreamReader > just > > > adjusts it to our preferred size - which is now 512 KB. > > > > > > > > let providers work, even if the |buffer_length| is too large. > > > > Didn't you say that the browser crashes when the buffer length is larger > > than > > > > the maximum IPC message size? > > > > > > These things are not related. BufferedFileStreamReader works if > > > BufferedFileStreamReader::Read's |buffer_length| is larger than max IPC > > message > > > size, since the *inner file stream reader's* Read's would be called with > > > |buffer_length| up to 512KB size, which is smaller than IPC message size. If > > we > > > introduced the if-statement you suggested, then we could crash. > > What I meant is that if you really care about the 128MB IPC message size > limit, > > you should assert that the chunk size used by this class (512KB currently) is > > smaller than the IPC size limit (128MB currently) > > so that it can fail in the compile stage rather than browser_tests when > someone > > tries to change the chunk size to a value larger than 128MB. > > I don't think we should care about 128MB IPC message size limit though. > > > > > > > > > > > > > > > > > > > > > > I thought about it again, and IMHO we should have a cap for the > buffer > > > > size > > > > > as > > > > > > > it is. > > > > > > > > > > > > > > We can't simply add that if. If the inner buffer already has some > > data, > > > we > > > > > > have > > > > > > > to copy them first, and then if |buffer_length - bytes_read| > > > > > buffer_size_, > > > > > > > then the rest directly. > > > > > > The code here returns the result without performing actual read when > any > > > > data > > > > > is > > > > > > left in the buffer. > > > > > > > > > > We always try to read |buffer_length| from the internal buffer. If there > > is > > > > not > > > > > enough, then we return partial results. If nothing is read, then we > > prefetch > > > > and > > > > > return as much as we can. > > > > > > > > > > > Why do you think we need to change this behavior for direct read case? > > > > > > > > > > If we skip contents in the buffer, then we will lose data. We always > > *must* > > > > > return all data from the internal buffer, before calling the internal > file > > > > > stream reader for more data. > > > > I'm not saying we should leave data unread in the buffer. > > > > What I'm saying is that we should just put a if before the Preload() and > let > > > the > > > > inner reader directly return the result when appropriate. > > > > > > > > > > > > > > > > > > > It is non trivial, and it will require a test case. But we won't use > > > this > > > > > > code, > > > > > > Sorry, I don't understand why you are OK with supporting synchronous > > > readers > > > > > and > > > > > > having test cases for it which we will never need, > > > > > > while you are feeling nervous about preparing for future chunk size > > > increase > > > > > > which may possibly happen. > > > > > > > > > > Support for synchronous readers: > > > > > + Let's us remove dependency on the internal file stream reader > > > > implementation. > > > > > + Makes code simpler, and DCHECK free. Think of GetLength. > > > > DCHECK cannot cause a malfunction on the product while "if" can. > > > > GetLength() is nothing here. All complexity of this class is dedicated to > > > > Read(). > > > > > - Requires TEST_P. Test cases are same. > > > > Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in > tests > > > and > > > > the fake classes. > > > > > > Only one place in test cases. For the fake class, yes we have more, but they > > are > > > pretty trivial. > > > > > > > > - We don't need it now. > > > > > > > > > > As for "preparing for future chunk size increase": > > > > > - Requires *new* test cases. > > > > > - We don't need it now. > > > > > - We will not need it in the future. (See below) > > > > > - May cause a lot of problems. (See below) > > > > > - We can easily add it later if needed. > > > > The problem here is that it's hard to know when it's needed. > > > > When the file system provided by extensions are slow, it's hard to know > > which > > > > part is the bottleneck. > > > > It can be the cloud service, the extension implementation, IPC between the > > > > provider extension and the browser process, data handling in the browser > > > > process, or Files.app. > > > > > > You are saying, that the bottleneck can be anywhere, and I agree. Hence, > just > > > blindly allowing a larger buffer (just in case) is IMHO not the correct > > solution > > > for the bottleneck problem. Especially, currently increasing the buffer size > > > over 512KB doesn't help and would move more responsibilities on developers. > > > > > > I'm working on a performance analysis tool in > chrome://provided-file-systems, > > > which would say time taken by each of the steps, and then we will be able to > > > optimize the API well. > > IIUC the purpose of this class was just to reduce the number of IPCs. > > Why did you suddenly start arguing that it should be capped to 512KB even when > > fileapi wants to read in a larger sized chunk? > > I don't see any problems in moving responsibilities to the extension when the > > larger size is requested. > > I already answered this question. > > > > > - Requires *new* test cases. Adding a test case is better than being the bottleneck. > > > > > - We don't need it now. We may need it in future. > > > > > - We will not need it in the future. (See below) ? > > > > > - May cause a lot of problems. (See below) ? > > > > > - We can easily add it later if needed. The problem is that there is no easy way to know when this place becomes the bottleneck. > > > > > > > > > > > > > > I honestly can't see any advantages. > > > > > > > > > > > > because of the current buffer length size restriction. If it grows > to > > > > large > > > > > > size > > > > > > > in the future, then fetching a lot of data directly from providing > > > > > extensions > > > > > > > will cause request timeouts in case of remote providing extensions. > > > > > > If it was the case, shouldn't we allow providing extensions to decide > > how > > > to > > > > > > behave? > > > > > > (e.g. Depending on their servers' capability, capping the chunk size > and > > > > > > returning smaller amount of data for read request.) > > > > > > > > > > 1. They already can return data in chunks, so eg. that 512KB they can > > split > > > to > > > > > eg. 10x51KB. > > > > > 2. If we add this "if", then the buffer size may be any in the future. > > Every > > > > > developer has to add logic for splitting it. But, I predict that most of > > > them > > > > > will not. So once we increase the buffer size in Chromium, all remote > > > > extensions > > > > > will start timeouting, because a lot of developers assumed (by > > observation) > > > > that > > > > > the buffer size is always 512KB. > > > > We shouldn't care about developers who depend on an undocumented behavior. > > > > > > Shouldn't we? A lot of developers if they see that the buffer is always > 512KB, > > > will not split it in smaller requests. If you don't want to care about such > > > developers, then a lot of apps in the Chrome store will be super flaky. And > > IMHO > > > we don't want this to happen. > > The API documentation is the only contract made between Chromium and extension > > developers. > > If you think this should be 512KB forever (even after you stop maintaining > this > > code and after technology advancement makes 512KB chunk size ridiculously > > small), you should write it in the API documentation. > > I'm not planning to write it in the documentation. Developers should chop the > requests into chunks, and they shouldn't assume that the buffer size is fixed to > any value. But I know that a lot of developers will not chop. Don't you think > so? We shouldn't care about developers who write code which stops working when the read chunk size is changed. They are simply violating the contract. > > > > > > > I believe we should make the API as simple as possible, so developers do not > > > have much chance to break things. And still, we can't expect developers to > > write > > > perfect code. That's why we have timing out logic in the RequestManager, in > > case > > > developers do not handle an exception, or forget to call a callback. > > Timeout should be there because, without it, wrongly implemented extensions > can > > damage Chrome itself. > > > > > > > > 3. If the buffer is too large, then it will affect copying time between > > > remote > > > > > providing extensions and between them and Drive. We can't start > uploading, > > > > > before the first chunk is fully downloaded. This means even 2x slower > copy > > > if > > > > > the file size is around the buffer size. > > > > Our current drive implementation doesn't start uploading until the file > gets > > > > closed. > > > > > > AFAIR we are going to fix it (crbug.com/126902). Also, file stream providers > > crbug.com/126902 is about upload for web sites like Gmail, not to Drive. > Got it. Do we have any plans to support streamed uploading in Drive then? No. > > > > will support streamed uploading from beginning. > > Just mentioned to clarify that you shouldn't use our Drive client as an > example > > here. > > > > > > > > 4. Similarly, for too big buffer size, videos will load with a > significant > > > > > delay. > > > > > 5. For big buffer size, copying progress bars will be very unreliable. > > > > > > > > > > Saying that, I'm still not convinced how having a large buffer, like > > couple > > > of > > > > > megabytes could be a good thing in general case. I mentioned several > > serious > > > > > problems above, and probably there is much more. > > > > It's not you who makes this decision. > > > > Anyone can modify the user code in fileapi layer to change the buffer size > > > with > > > > a good rationale without consulting you. > > > > > > I'm not sure if I understand your point. BufferedFileSreamReader sets the > > buffer > > > to our preferred one, which is 512KB, and we don't care about the buffer > size > > > set in the fileapi layer. > > This class's purpose is to reduce the number of IPCs, right? > > If the fileapi is changed to use a chunk size larger than 512KB, it means that > > we will end up doing the opposite thing, even after the technology advancement > > or something makes 512KB chunk size ridiculously small. > > Don't you want your code to work without problems for a decade or more? > > Purpose of this class is to set the buffer size to our preferred size. You are > suggesting to write non-trivial code which will be most probably not used for > years and will not improve performance. That's why I'm not convinced. But if you > insist, I can add it. This code may be used in years, while the code to handle the synchronously returned results won't be used until the end of universe. Why and how should we decide that the "preferred size" is 512 KB? When the requested data size is larger than 512KB, this class should do nothing. > > > > > > > > > > > > > > > Also, isn't 512KB already large enough to cause timeout with poor > > network > > > > > > connection? > > > > > > > > > > It may be too big. I'm thinking of a solution for this, eg. a dialog > like > > > > "This > > > > > operation takes longer than expected. Do you want to wait for it or > > abort?". > > > > Why do you need to implement a new UI when the operation is aborted thanks > > to > > > > the timeout? > > > > > > In case HTTP connection is very slow, and the providing extension didn't > > finish > > > fetching chunk of data from the server. As you said, it may happen. > > When the timeout happens, the operation gets aborted automatically. > > You don't need to implement any new UI for it. > > We can't blindly timeout each request after 10 seconds. It may happen that a > providing extension shows a dialog for credentials, and the operation would take > minutes. We don't want to abort in such case. Does it mean we show cancel dialog every time when the user takes long time to finish the authentication? Isn't it annoying? Shouldn't we add a dedicated API mechanism to let the provider extension finish the authentication step? > > Please read the API proposals as well as the "File System Provider API Request > Policy" (slightly outdated) I shared with the team. The timing out problem is > described there in details. > > > > > > > > > > > > > > > > > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > > > > > > > > > Hence, I added a comment to make it clear that this class fetches > data > > > > from > > > > > > the > > > > > > > underlying file stream reader in chunks of up to |buffer_length| > size. > > > > > > > > > > > > > > > > > > > > >
https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/80001/chrome/browser/chromeos/... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:38: return bytes_read; TOn 2014/06/27 02:17:58, hashimoto wrote: > On 2014/06/20 04:20:23, mtomasz wrote: > > On 2014/06/19 07:27:45, hashimoto wrote: > > > On 2014/06/16 04:12:18, mtomasz wrote: > > > > On 2014/06/11 06:01:03, hashimoto wrote: > > > > > On 2014/06/10 12:23:55, mtomasz wrote: > > > > > > On 2014/06/10 10:43:24, hashimoto wrote: > > > > > > > On 2014/06/10 08:38:42, mtomasz wrote: > > > > > > > > On 2014/06/09 13:51:16, mtomasz wrote: > > > > > > > > > On 2014/06/09 11:12:04, hashimoto wrote: > > > > > > > > > > On 2014/06/06 04:22:56, mtomasz wrote: > > > > > > > > > > > On 2014/06/06 03:36:52, hashimoto wrote: > > > > > > > > > > > > How about adding here something like: > > > > > > > > > > > > > > > > > > > > > > > > if (buffer_length >= buffer_size_) > > > > > > > > > > > > return file_stream_reader_->Read(buffer, buffer_length, > > > > > callback); > > > > > > > > > > > > > > > > > > > > > > > > I'm worrying, in future, the user code may pass a big > value > > > for > > > > > > > > > > > |buffer_length| > > > > > > > > > > > > and this place may be a hidden bottleneck. > > > > > > > > > > > > > > > > > > > > > > The buffer length is not user controlled, but it is > hard-coded > > > > > > currently > > > > > > > > to > > > > > > > > > > > 32KB. However, if it happened that it is larger than > > > |buffer_size| > > > > > > > (512KB) > > > > > > > > > and > > > > > > > > > > > we would have this extra-if, then we could crash the > renderer > > > > > process, > > > > > > > > since > > > > > > > > > > the > > > > > > > > > > > maximum IPC message is 1MB. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/ipc/ipc_channel.h&... > > > > > > > > > > Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * > > > 1024;" > > > > > > > 128MB? > > > > > > > > > > > > > > > > > > > > > > Hence, the current code has two purposes - to increase chunk > > > size > > > > > from > > > > > > > > 32KB > > > > > > > > > to > > > > > > > > > > > 512KB, and to cap it to 512KB, so we won't crash the > renderer. > > > > > > > > > > If it was the purpose, why don't you add a static assert > against > > > it? > > > > > > > > > > > > > > > > > > > > > > > > > > > > You're right, of course 128MB. In such case, the extra if makes > of > > > > > course > > > > > > > > sense. > > > > > > > > > But how do you want to use a static assert here? Could you > > clarify? > > > > > > > If it was really a purpose of this class, having a static assert to > > > check > > > > > that > > > > > > > the chunk size is less than the IPC size limit will make your > purpose > > > > > clearer > > > > > > > and prevent mysterious crash from happening when someone tries to > > > increase > > > > > the > > > > > > > chunk size in future. > > > > > > > > > > > > Do you want to crash with a static assert in such situations? Why? We > > want > > > > to > > > > > Static assert failure doesn't cause a crash. It causes a compile error. > > > > > > > > We can't use them then. We need to compile Chrome, even if the buffer is > > > larger. > > > > Buffer size is same for any file stream reader. BufferingFileStreamReader > > just > > > > adjusts it to our preferred size - which is now 512 KB. > > > > > > > > > > let providers work, even if the |buffer_length| is too large. > > > > > Didn't you say that the browser crashes when the buffer length is larger > > > than > > > > > the maximum IPC message size? > > > > > > > > These things are not related. BufferedFileStreamReader works if > > > > BufferedFileStreamReader::Read's |buffer_length| is larger than max IPC > > > message > > > > size, since the *inner file stream reader's* Read's would be called with > > > > |buffer_length| up to 512KB size, which is smaller than IPC message size. > If > > > we > > > > introduced the if-statement you suggested, then we could crash. > > > What I meant is that if you really care about the 128MB IPC message size > > limit, > > > you should assert that the chunk size used by this class (512KB currently) > is > > > smaller than the IPC size limit (128MB currently) > > > so that it can fail in the compile stage rather than browser_tests when > > someone > > > tries to change the chunk size to a value larger than 128MB. > > > I don't think we should care about 128MB IPC message size limit though. > > > > > > > > > > > > > > > > > > > > > > > > > > I thought about it again, and IMHO we should have a cap for the > > buffer > > > > > size > > > > > > as > > > > > > > > it is. > > > > > > > > > > > > > > > > We can't simply add that if. If the inner buffer already has some > > > data, > > > > we > > > > > > > have > > > > > > > > to copy them first, and then if |buffer_length - bytes_read| > > > > > > buffer_size_, > > > > > > > > then the rest directly. > > > > > > > The code here returns the result without performing actual read when > > any > > > > > data > > > > > > is > > > > > > > left in the buffer. > > > > > > > > > > > > We always try to read |buffer_length| from the internal buffer. If > there > > > is > > > > > not > > > > > > enough, then we return partial results. If nothing is read, then we > > > prefetch > > > > > and > > > > > > return as much as we can. > > > > > > > > > > > > > Why do you think we need to change this behavior for direct read > case? > > > > > > > > > > > > If we skip contents in the buffer, then we will lose data. We always > > > *must* > > > > > > return all data from the internal buffer, before calling the internal > > file > > > > > > stream reader for more data. > > > > > I'm not saying we should leave data unread in the buffer. > > > > > What I'm saying is that we should just put a if before the Preload() and > > let > > > > the > > > > > inner reader directly return the result when appropriate. > > > > > > > > > > > > > > > > > > > > > > It is non trivial, and it will require a test case. But we won't > use > > > > this > > > > > > > code, > > > > > > > Sorry, I don't understand why you are OK with supporting synchronous > > > > readers > > > > > > and > > > > > > > having test cases for it which we will never need, > > > > > > > while you are feeling nervous about preparing for future chunk size > > > > increase > > > > > > > which may possibly happen. > > > > > > > > > > > > Support for synchronous readers: > > > > > > + Let's us remove dependency on the internal file stream reader > > > > > implementation. > > > > > > + Makes code simpler, and DCHECK free. Think of GetLength. > > > > > DCHECK cannot cause a malfunction on the product while "if" can. > > > > > GetLength() is nothing here. All complexity of this class is dedicated > to > > > > > Read(). > > > > > > - Requires TEST_P. Test cases are same. > > > > > Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in > > tests > > > > and > > > > > the fake classes. > > > > > > > > Only one place in test cases. For the fake class, yes we have more, but > they > > > are > > > > pretty trivial. > > > > > > > > > > - We don't need it now. > > > > > > > > > > > > As for "preparing for future chunk size increase": > > > > > > - Requires *new* test cases. > > > > > > - We don't need it now. > > > > > > - We will not need it in the future. (See below) > > > > > > - May cause a lot of problems. (See below) > > > > > > - We can easily add it later if needed. > > > > > The problem here is that it's hard to know when it's needed. > > > > > When the file system provided by extensions are slow, it's hard to know > > > which > > > > > part is the bottleneck. > > > > > It can be the cloud service, the extension implementation, IPC between > the > > > > > provider extension and the browser process, data handling in the browser > > > > > process, or Files.app. > > > > > > > > You are saying, that the bottleneck can be anywhere, and I agree. Hence, > > just > > > > blindly allowing a larger buffer (just in case) is IMHO not the correct > > > solution > > > > for the bottleneck problem. Especially, currently increasing the buffer > size > > > > over 512KB doesn't help and would move more responsibilities on > developers. > > > > > > > > I'm working on a performance analysis tool in > > chrome://provided-file-systems, > > > > which would say time taken by each of the steps, and then we will be able > to > > > > optimize the API well. > > > IIUC the purpose of this class was just to reduce the number of IPCs. > > > Why did you suddenly start arguing that it should be capped to 512KB even > when > > > fileapi wants to read in a larger sized chunk? > > > I don't see any problems in moving responsibilities to the extension when > the > > > larger size is requested. > > > > I already answered this question. > > > > > > - Requires *new* test cases. > Adding a test case is better than being the bottleneck. Removing the 512KB limit may cause another bottlenecks (streamed uploading), which I explained before. But I'll just add this code so we can move forward. > > > > > > - We don't need it now. > We may need it in future. > > > > > > - We will not need it in the future. (See below) > ? > > > > > > - May cause a lot of problems. (See below) > ? What does this question mark mean here? > > > > > > - We can easily add it later if needed. > The problem is that there is no easy way to know when this place becomes the > bottleneck. > > > > > > > > > > > > > > > > > > I honestly can't see any advantages. > > > > > > > > > > > > > > because of the current buffer length size restriction. If it grows > > to > > > > > large > > > > > > > size > > > > > > > > in the future, then fetching a lot of data directly from providing > > > > > > extensions > > > > > > > > will cause request timeouts in case of remote providing > extensions. > > > > > > > If it was the case, shouldn't we allow providing extensions to > decide > > > how > > > > to > > > > > > > behave? > > > > > > > (e.g. Depending on their servers' capability, capping the chunk size > > and > > > > > > > returning smaller amount of data for read request.) > > > > > > > > > > > > 1. They already can return data in chunks, so eg. that 512KB they can > > > split > > > > to > > > > > > eg. 10x51KB. > > > > > > 2. If we add this "if", then the buffer size may be any in the future. > > > Every > > > > > > developer has to add logic for splitting it. But, I predict that most > of > > > > them > > > > > > will not. So once we increase the buffer size in Chromium, all remote > > > > > extensions > > > > > > will start timeouting, because a lot of developers assumed (by > > > observation) > > > > > that > > > > > > the buffer size is always 512KB. > > > > > We shouldn't care about developers who depend on an undocumented > behavior. > > > > > > > > Shouldn't we? A lot of developers if they see that the buffer is always > > 512KB, > > > > will not split it in smaller requests. If you don't want to care about > such > > > > developers, then a lot of apps in the Chrome store will be super flaky. > And > > > IMHO > > > > we don't want this to happen. > > > The API documentation is the only contract made between Chromium and > extension > > > developers. > > > If you think this should be 512KB forever (even after you stop maintaining > > this > > > code and after technology advancement makes 512KB chunk size ridiculously > > > small), you should write it in the API documentation. > > > > I'm not planning to write it in the documentation. Developers should chop the > > requests into chunks, and they shouldn't assume that the buffer size is fixed > to > > any value. But I know that a lot of developers will not chop. Don't you think > > so? > We shouldn't care about developers who write code which stops working when the > read chunk size is changed. > They are simply violating the contract. > > > > > > > > > > I believe we should make the API as simple as possible, so developers do > not > > > > have much chance to break things. And still, we can't expect developers to > > > write > > > > perfect code. That's why we have timing out logic in the RequestManager, > in > > > case > > > > developers do not handle an exception, or forget to call a callback. > > > Timeout should be there because, without it, wrongly implemented extensions > > can > > > damage Chrome itself. > > > > > > > > > > 3. If the buffer is too large, then it will affect copying time > between > > > > remote > > > > > > providing extensions and between them and Drive. We can't start > > uploading, > > > > > > before the first chunk is fully downloaded. This means even 2x slower > > copy > > > > if > > > > > > the file size is around the buffer size. > > > > > Our current drive implementation doesn't start uploading until the file > > gets > > > > > closed. > > > > > > > > AFAIR we are going to fix it (crbug.com/126902). Also, file stream > providers > > > crbug.com/126902 is about upload for web sites like Gmail, not to Drive. > > Got it. Do we have any plans to support streamed uploading in Drive then? > No. Got it. > > > > > > will support streamed uploading from beginning. > > > Just mentioned to clarify that you shouldn't use our Drive client as an > > example > > > here. > > > > > > > > > > 4. Similarly, for too big buffer size, videos will load with a > > significant > > > > > > delay. > > > > > > 5. For big buffer size, copying progress bars will be very unreliable. > > > > > > > > > > > > Saying that, I'm still not convinced how having a large buffer, like > > > couple > > > > of > > > > > > megabytes could be a good thing in general case. I mentioned several > > > serious > > > > > > problems above, and probably there is much more. > > > > > It's not you who makes this decision. > > > > > Anyone can modify the user code in fileapi layer to change the buffer > size > > > > with > > > > > a good rationale without consulting you. > > > > > > > > I'm not sure if I understand your point. BufferedFileSreamReader sets the > > > buffer > > > > to our preferred one, which is 512KB, and we don't care about the buffer > > size > > > > set in the fileapi layer. > > > This class's purpose is to reduce the number of IPCs, right? > > > If the fileapi is changed to use a chunk size larger than 512KB, it means > that > > > we will end up doing the opposite thing, even after the technology > advancement > > > or something makes 512KB chunk size ridiculously small. > > > Don't you want your code to work without problems for a decade or more? > > > > Purpose of this class is to set the buffer size to our preferred size. You are > > suggesting to write non-trivial code which will be most probably not used for > > years and will not improve performance. That's why I'm not convinced. But if > you > > insist, I can add it. > This code may be used in years, while the code to handle the synchronously > returned results won't be used until the end of universe. I have plans to clean up file_system_provider/fileapi/file_stream_reader.cc, which would return values synchronously, so not end of universe, but couple of weeks? :) > Why and how should we decide that the "preferred size" is 512 KB? Do you have a suggestion for a better value than 512 KB? I decided upon experiments. I guess similar was made for 32 KB as a buffer size for FileStreamReaders. > When the requested data size is larger than 512KB, this class should do nothing. In general I prefer arguments over "should". But anyway, I'm going to implement it, to move forward. > > > > > > > > > > > > > > > > > > > Also, isn't 512KB already large enough to cause timeout with poor > > > network > > > > > > > connection? > > > > > > > > > > > > It may be too big. I'm thinking of a solution for this, eg. a dialog > > like > > > > > "This > > > > > > operation takes longer than expected. Do you want to wait for it or > > > abort?". > > > > > Why do you need to implement a new UI when the operation is aborted > thanks > > > to > > > > > the timeout? > > > > > > > > In case HTTP connection is very slow, and the providing extension didn't > > > finish > > > > fetching chunk of data from the server. As you said, it may happen. > > > When the timeout happens, the operation gets aborted automatically. > > > You don't need to implement any new UI for it. > > > > We can't blindly timeout each request after 10 seconds. It may happen that a > > providing extension shows a dialog for credentials, and the operation would > take > > minutes. We don't want to abort in such case. > Does it mean we show cancel dialog every time when the user takes long time to > finish the authentication? > Isn't it annoying? > Shouldn't we add a dedicated API mechanism to let the provider extension finish > the authentication step? Yes. If authentication takes long, a notification will pop up. More sophisticated solution may come in the future. > > > > Please read the API proposals as well as the "File System Provider API Request > > Policy" (slightly outdated) I shared with the team. The timing out problem is > > described there in details. > > > > > > > > > > > > > > > > > > > > > > > > > > > > So basically, there is IMHO no reason to add this code path. > > > > > > > > > > > > > > > > Hence, I added a comment to make it clear that this class fetches > > data > > > > > from > > > > > > > the > > > > > > > > underlying file stream reader in chunks of up to |buffer_length| > > size. > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Not sure if I've fully followed the whole discussion, so allow me to post a piece-wise comment. * Allowing to fetch > 512 KB should be fine. If I'm not missing, there's no chance that user-specified chunk size is not directly wired to the level of FileStreamReader, and thus we can assume the possible buffer size is always modest (or intentionally set large in future code inside Chrome.) FileStreamReader is used in two places. One is cross-filesystem copy like fsp-to-Drive in fileBrowserPrivate.startCopy(). The other is in URLRequest to the file system URL (which includes XHR or reading via JavaScript FileReader http://www.chromium.org/developers/code-reading/how-html5-s-filereader-works). Both look to be employing 32KB buffer size. * Using TestCompletionCallback instead of a Logger I'll take a look on the other WIP CL and comment there later.
On 2014/06/27 06:18:19, kinaba wrote: > should be fine. If I'm not missing, there's no chance that user-specified > chunk size is not directly wired to the level of FileStreamReader, and I mean, no change that is is directly passed to FileStream.
On 2014/06/27 06:19:12, kinaba wrote: > On 2014/06/27 06:18:19, kinaba wrote: > > should be fine. If I'm not missing, there's no chance that user-specified > > chunk size is not directly wired to the level of FileStreamReader, and > I mean, no change that is is directly passed to FileStream. @hashimoto: Ping. I removed support for returning results synchronously + added code to skip the inner buffer, as you requested. Please let me know what exactly you want me to fix in the Logger class, so I can move forward with this CL.
On 2014/07/02 04:22:10, mtomasz wrote: > On 2014/06/27 06:19:12, kinaba wrote: > > On 2014/06/27 06:18:19, kinaba wrote: > > > should be fine. If I'm not missing, there's no chance that user-specified > > > chunk size is not directly wired to the level of FileStreamReader, and > > I mean, no change that is is directly passed to FileStream. > > @hashimoto: Ping. Sorry I thought you were waiting for kinaba's comment on https://codereview.chromium.org/334593003/. > > I removed support for returning results synchronously + added code to skip the > inner buffer, as you requested. Didn't you say "I have plans to clean up file_system_provider/fileapi/file_stream_reader.cc, which would return values synchronously"? > Please let me know what exactly you want me to fix in the Logger class, so I can > move forward with this CL.
On 2014/07/02 09:05:47, hashimoto wrote: > On 2014/07/02 04:22:10, mtomasz wrote: > > On 2014/06/27 06:19:12, kinaba wrote: > > > On 2014/06/27 06:18:19, kinaba wrote: > > > > should be fine. If I'm not missing, there's no chance that user-specified > > > > chunk size is not directly wired to the level of FileStreamReader, and > > > I mean, no change that is is directly passed to FileStream. > > > > @hashimoto: Ping. > Sorry I thought you were waiting for kinaba's comment on > https://codereview.chromium.org/334593003/. > > > > I removed support for returning results synchronously + added code to skip the > > inner buffer, as you requested. > Didn't you say "I have plans to clean up > file_system_provider/fileapi/file_stream_reader.cc, > which would return values synchronously"? > > Please let me know what exactly you want me to fix in the Logger class, so I > can > > move forward with this CL. There is no restriction that Read() will not be called again after it returned an error. Similarly, there is no restriction that Read() must not be called, if GetLength() returned an error. If a file system is gone during either Read or GetLength call, all consecutive calls should return an error (net::ERR_ABORT) which can be done synchronously. However, I gave up this idea since it seems that we never call either of these methods after receiving an error. This may however change in the future. WDYT?
@hashimoto, @kinaba: I've removed the Logger class. PTAL.
To me it looks ok, but please wait fpr @hashimoto's comments. https://codereview.chromium.org/318563002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:36: CopyFromBuffer(make_scoped_refptr(buffer), buffer_length); maybe: CopyFromBuffer -> CopyFromPreloadingBuffer? It is a little bit confusing that CopyFromBuffer(buffer, ...); does not copy from |buffer|, rather it copies into |buffer| from |preloading_buffer_|.
https://codereview.chromium.org/318563002/diff/140001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/140001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:36: CopyFromBuffer(make_scoped_refptr(buffer), buffer_length); On 2014/07/07 06:37:29, kinaba wrote: > maybe: CopyFromBuffer -> CopyFromPreloadingBuffer? > > It is a little bit confusing that CopyFromBuffer(buffer, ...); does not copy > from |buffer|, > rather it copies into |buffer| from |preloading_buffer_|. Good point. Done.
lgtm https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:9: #include "base/message_loop/message_loop.h" nit: No need to include this. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:10: #include "base/memory/weak_ptr.h" nit: No need to include this. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:11: #include "base/message_loop/message_loop.h" nit: This should be message_loop_proxy https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:41: FakeFileStreamReader(std::vector<int>* log, bool return_error) How about replacing |return_error| with net::Error (or int)? This way there is no need to have /* return_error */ comments everywhere. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:153: EXPECT_EQ(kBufferSize, inner_read_log[0]); Shouldn't this be inner_read_log[1]? BTW, how about calling inner_read_log.clear() before calling Read()? It should eliminate this kind of mistakes.
https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc (right): https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc:9: #include "base/message_loop/message_loop.h" On 2014/07/08 11:12:55, hashimoto wrote: > nit: No need to include this. Done. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... File chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc (right): https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:10: #include "base/memory/weak_ptr.h" On 2014/07/08 11:12:55, hashimoto wrote: > nit: No need to include this. Done. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:11: #include "base/message_loop/message_loop.h" On 2014/07/08 11:12:55, hashimoto wrote: > nit: This should be message_loop_proxy Done. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:41: FakeFileStreamReader(std::vector<int>* log, bool return_error) On 2014/07/08 11:12:55, hashimoto wrote: > How about replacing |return_error| with net::Error (or int)? > This way there is no need to have /* return_error */ comments everywhere. Done. https://codereview.chromium.org/318563002/diff/160001/chrome/browser/chromeos... chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader_unittest.cc:153: EXPECT_EQ(kBufferSize, inner_read_log[0]); On 2014/07/08 11:12:55, hashimoto wrote: > Shouldn't this be inner_read_log[1]? > > BTW, how about calling inner_read_log.clear() before calling Read()? > It should eliminate this kind of mistakes. Good idea. Done.
The CQ bit was checked by mtomasz@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mtomasz@chromium.org/318563002/180001
Message was sent while issue was closed.
Change committed as 282290 |
