[fsp] Introduce BufferingFileStreamReader to read files in bigger chunks.

chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc

Index: chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc
diff --git a/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc
new file mode 100644
index 0000000000000000000000000000000000000000..df8c3af846bc1be07758c016d11459160d60974b
--- /dev/null
+++ b/chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.cc
@@ -0,0 +1,102 @@
+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/chromeos/file_system_provider/fileapi/buffering_file_stream_reader.h"
+
+#include <algorithm>
+
+#include "base/message_loop/message_loop.h"
+#include "net/base/io_buffer.h"
+#include "net/base/net_errors.h"
+
+namespace chromeos {
+namespace file_system_provider {
+
+BufferingFileStreamReader::BufferingFileStreamReader(
+    scoped_ptr<webkit_blob::FileStreamReader> file_stream_reader,
+    int buffer_size)
+    : file_stream_reader_(file_stream_reader.Pass()),
+      buffer_size_(buffer_size),
+      preloading_buffer_(new net::IOBuffer(buffer_size_)),
+      preloading_buffer_offset_(0),
+      buffered_bytes_(0),
+      weak_ptr_factory_(this) {
+}
+
+BufferingFileStreamReader::~BufferingFileStreamReader() {
+}
+
+int BufferingFileStreamReader::Read(net::IOBuffer* buffer,
+                                    int buffer_length,
+                                    const net::CompletionCallback& callback) {
+  // Return as much as available in the internal buffer. It may be less than
+  // |buffer_length|, what is valid.
+  const int bytes_read =
+      CopyFromBuffer(make_scoped_refptr(buffer), buffer_length);
+  if (bytes_read)
+    return bytes_read;

[hashimoto, 2014/06/06 03:36:52]

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.

[mtomasz, 2014/06/06 04:22:56]

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, 2014/06/09 11:12:04]

Isn't "static const size_t kMaximumMessageSize = 128 * 1024 * 1024;" 128MB?
If it was the purpose, why don't you add a static assert against it?

[mtomasz, 2014/06/09 13:51:16]

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?

[mtomasz, 2014/06/10 08:38:42]

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.

[hashimoto, 2014/06/10 10:43:24]

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.
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?
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.
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?

[mtomasz, 2014/06/10 12:23:55]

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.
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.
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.
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.
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.
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?".

[hashimoto, 2014/06/11 06:01:03]

Static assert failure doesn't cause a crash. It causes a compile error.
Didn't you say that the browser crashes when the buffer length is larger than
the maximum IPC message size?
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.
DCHECK cannot cause a malfunction on the product while "if" can.
GetLength() is nothing here. All complexity of this class is dedicated to
Read().
Not exactly the same. There are a number of "mode_ == SYNCHRONOUS" in tests and
the fake classes.
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.
We shouldn't care about developers who depend on an undocumented behavior.
Our current drive implementation doesn't start uploading until the file gets
closed.
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.
Why do you need to implement a new UI when the operation is aborted thanks to
the timeout?

[mtomasz, 2014/06/16 04:12:18]

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.
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.
Only one place in test cases. For the fake class, yes we have more, but they are
pretty trivial.
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.
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.
AFAIR we are going to fix it (crbug.com/126902). Also, file stream providers
will support streamed uploading from beginning.
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.
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.

[hashimoto, 2014/06/19 07:27:45]

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.
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.
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.
Timeout should be there because, without it, wrongly implemented extensions can
damage Chrome itself.
crbug.com/126902 is about upload for web sites like Gmail, not to Drive.
Just mentioned to clarify that you shouldn't use our Drive client as an example
here.
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?
When the timeout happens, the operation gets aborted automatically.
You don't need to implement any new UI for it.

[mtomasz, 2014/06/20 04:20:23]

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'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?
Got it. Do we have any plans to support streamed uploading in Drive then?
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.
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.

[hashimoto, 2014/06/27 02:17:58]

Adding a test case is better than being the bottleneck.
We may need it in future.
?
?
The problem is that there is no easy way to know when this place becomes the
bottleneck.
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.
No.
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.
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?

[mtomasz, 2014/06/27 05:00:25]

TOn 2014/06/27 02:17:58, hashimoto wrote:
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.
What does this question mark mean here?
Got it.
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? :)
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.
In general I prefer arguments over "should". But anyway, I'm going to implement
it, to move forward.
Yes. If authentication takes long, a notification will pop up. More
sophisticated solution may come in the future.

+
+  // Nothing copied, so contents have to be preloaded.
+  Preload(base::Bind(&BufferingFileStreamReader::OnPreloadCompleted,
+                     weak_ptr_factory_.GetWeakPtr(),
+                     make_scoped_refptr(buffer),
+                     buffer_length,
+                     callback));
+
+  return net::ERR_IO_PENDING;
+}
+
+int64 BufferingFileStreamReader::GetLength(
+    const net::Int64CompletionCallback& callback) {
+  return file_stream_reader_->GetLength(callback);
+}
+
+int BufferingFileStreamReader::CopyFromBuffer(
+    scoped_refptr<net::IOBuffer> buffer,
+    int buffer_length) {
+  const int read_bytes = std::min(buffer_length, buffered_bytes_);
+
+  memcpy(buffer->data(),
+         preloading_buffer_->data() + preloading_buffer_offset_,
+         read_bytes);
+  preloading_buffer_offset_ += read_bytes;
+  buffered_bytes_ -= read_bytes;
+
+  return read_bytes;
+}
+
+void BufferingFileStreamReader::Preload(
+    const net::CompletionCallback& callback) {
+  // TODO(mtomasz): Dynamically calculate the chunk size. Start from a small
+  // one, then increase for consecutive requests. That would improve performance
+  // when reading just small chunks, instead of the entire file.
+  const int preload_bytes = buffer_size_;
+
+  const int result =
+      file_stream_reader_->Read(preloading_buffer_, preload_bytes, callback);
+
+  if (result != net::ERR_IO_PENDING) {
+    base::MessageLoopProxy::current()->PostTask(FROM_HERE,
+                                                base::Bind(callback, result));
+  }
+}
+
+void BufferingFileStreamReader::OnPreloadCompleted(
+    scoped_refptr<net::IOBuffer> buffer,
+    int buffer_length,
+    const net::CompletionCallback& callback,
+    int result) {
+  if (result < 0) {
+    callback.Run(result);
+    return;
+  }
+
+  preloading_buffer_offset_ = 0;
+  buffered_bytes_ = result;
+
+  callback.Run(CopyFromBuffer(buffer, buffer_length));
+}
+
+}  // namespace file_system_provider
+}  // namespace chromeos