[fsp] Add throttling for number of opened files.

[fsp] Add throttling for number of opened files.

This patch adds a limit for number of opened files at once. The reason for that
is that provided file systems may not support multiple opened files per once due
to performance reasons. Enqueuing files is on the extension side is complicated
as extensions are event pages, and state may get lost after a suspend.

Note, that this CL only adds the C++ side. The opened_files_limit option is not
exposed via File System Provider API yet. This will be done separately.

Finally, the Queue class will be reused for enqueuing watcher events, which are
currently not safe (there are TODO about adding a queue). This will be also done
separately.

TEST=unit_tests: *FileSystemProviderQueue*, *FileSystemProviderThrottled*
BUG=445125
Committed: https://crrev.com/3dd2edf4a117d03d2ec45bb3dcf205cb7561a7e1
Cr-Commit-Position: refs/heads/master@{#310717}

[mtomasz, 2015-01-07 05:51:57]

mtomasz@chromium.org changed reviewers:
+ hirono@chromium.org

[mtomasz, 2015-01-07 05:52:14]

@hirono: PTAL. Thanks. Sorry a for large CL.

[hirono, 2015-01-07 06:57:37]

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:147:
last_abort_callback.Run(base::Bind(
Can we ensure last_abort_callback.is_null() == false?
How about adding DCHECK?

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer.cc:112:
last_abort_callback.Run(base::Bind(
ditto

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:201:
std::set<int> opened_files_;
Please add comment about what the int means.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/throttled_file_system.cc:190: // If
the file is opened successfully then hold the queue token the file is
queue token the -> queue token until the ?

[hirono, 2015-01-07 07:20:05]

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:54: AbortCallback
Enqueue(int token, const AbortableCallback& callback);
Could you also mention about AbortCallback? After calling the abort callback, do
the client code need to call Remove() or not?

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:58: // Otherwise returns
false. Note, that Remove() must be called in order to
When do Complete return false? The task can be completed before it starts?

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:64: bool Remove(int token);
Should we allow to remove uncompleted tasks?

[mtomasz, 2015-01-07 09:08:41]

Thanks!

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_reader.cc:147:
last_abort_callback.Run(base::Bind(
On 2015/01/07 06:57:36, hirono wrote:
> Can we ensure last_abort_callback.is_null() == false?
> How about adding DCHECK?

It's checked in #136 already.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/fileapi/file_stream_writer.cc:112:
last_abort_callback.Run(base::Bind(
On 2015/01/07 06:57:36, hirono wrote:
> ditto

Acknowledged.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:201:
std::set<int> opened_files_;
On 2015/01/07 06:57:36, hirono wrote:
> Please add comment about what the int means.

Done.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:54: AbortCallback
Enqueue(int token, const AbortableCallback& callback);
On 2015/01/07 07:20:05, hirono wrote:
> Could you also mention about AbortCallback? After calling the abort callback,
do
> the client code need to call Remove() or not?

The abort callback removes the task from the queue entirely. It is because the
abort callback is designed to be passed to many places, which do not have access
to the queue, so they can't call Remove().

I added more comments. Done.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:58: // Otherwise returns
false. Note, that Remove() must be called in order to
On 2015/01/07 07:20:05, hirono wrote:
> When do Complete return false? The task can be completed before it starts?

Queue doesn't know when a task is completed. That would require complex variable
length templates, which I wanted to avoid. Instead, the client of the queue has
to simply notify the queue that the asynchronous task is completed, so the queue
can forbid aborting (as the task is completed, there is nothing to abort).

Saying that, task cannot be completed before it starts. I renamed running to
executed, which sounds more suitable here as we have |executed_|.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:64: bool Remove(int token);
On 2015/01/07 07:20:05, hirono wrote:
> Should we allow to remove uncompleted tasks?

I can't think of any use cases. Instead of removing uncompleted tasks, aborting
callback should be used.

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
(right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/throttled_file_system.cc:190: // If
the file is opened successfully then hold the queue token the file is
On 2015/01/07 06:57:36, hirono wrote:
> queue token the -> queue token until the ?

Done.

[hirono, 2015-01-07 10:34:29]

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:64: bool Remove(int token);
On 2015/01/07 09:08:41, mtomasz wrote:
> On 2015/01/07 07:20:05, hirono wrote:
> > Should we allow to remove uncompleted tasks?
> 
> I can't think of any use cases. Instead of removing uncompleted tasks,
aborting
> callback should be used.

So could you updated the comment and add DCHECK in Remove so that the method
dose not try to remove uncompleted task?

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:201:
std::set<int> opened_files_;  // List of queue tokens for opened files.
nit: Set of queue tokens... ?

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.h:63: bool Complete(int
token);
Do we need to know the return value of Complete?
If it's false, it means the task is aborted or completed before, so the someone
who need to manage the state of task has already known the results of task.
e.g. ThrottledFileSystem::OnOpenFileCompleted is not called if the task is
aborted, because CancelableTaskTracker manages the state.

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.h:67: bool Remove(int token);
ditto: Do we need to know the return value?

[mtomasz, 2015-01-08 02:18:27]

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/80001/chrome/browser/chromeos/...
chrome/browser/chromeos/file_system_provider/queue.h:64: bool Remove(int token);
On 2015/01/07 10:34:29, hirono wrote:
> On 2015/01/07 09:08:41, mtomasz wrote:
> > On 2015/01/07 07:20:05, hirono wrote:
> > > Should we allow to remove uncompleted tasks?
> > 
> > I can't think of any use cases. Instead of removing uncompleted tasks,
> aborting
> > callback should be used.
> 
> So could you updated the comment and add DCHECK in Remove so that the method
> dose not try to remove uncompleted task?

Done.

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system.h
(right):

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system.h:201:
std::set<int> opened_files_;  // List of queue tokens for opened files.
On 2015/01/07 10:34:29, hirono wrote:
> nit: Set of queue tokens... ?

Removed. This is supposed to be only in throttled_file_system.h. Done.

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.h (right):

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.h:63: bool Complete(int
token);
On 2015/01/07 10:34:29, hirono wrote:
> Do we need to know the return value of Complete?
> If it's false, it means the task is aborted or completed before, so the
someone
> who need to manage the state of task has already known the results of task.
> e.g. ThrottledFileSystem::OnOpenFileCompleted is not called if the task is
> aborted, because CancelableTaskTracker manages the state.
> 

That's right. I added more comments, and changed to DCHECK. Done.

https://codereview.chromium.org/829553002/diff/120001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.h:67: bool Remove(int token);
On 2015/01/07 10:34:29, hirono wrote:
> ditto: Do we need to know the return value?

Done.

[hirono, 2015-01-08 06:28:09]

This is the last round. Thanks!

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:41:
pending_.push_back(Task(token, callback));
nit: How about DCHECKing the queue does not have task with having the specified
|token|?

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:63: if (executed_.size()
== static_cast<size_t>(max_in_parallel_) ||
nit: How about turning max_in_parallel_ into size_t instead of casting?

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue_unittest.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue_unittest.cc:166: //
Completing and removing the second task, which however hasn't started.
nit: the second -> the first?

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue_unittest.cc:302:
base::RunLoop().RunUntilIdle();
Is this line needed?

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
(right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/throttled_file_system.cc:206: if
(result == base::File::FILE_OK) {
If the file fails to close, the JavaScript code should call close method again
to close the file?

[mtomasz, 2015-01-08 07:07:50]

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:41:
pending_.push_back(Task(token, callback));
On 2015/01/08 06:28:09, hirono wrote:
> nit: How about DCHECKing the queue does not have task with having the
specified
> |token|?

Done.

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:63: if (executed_.size()
== static_cast<size_t>(max_in_parallel_) ||
On 2015/01/08 06:28:09, hirono wrote:
> nit: How about turning max_in_parallel_ into size_t instead of casting?

I tried it before, but it caused casting in other place from int to size_t. I'm
not sure which is better, but in general I find using "int" as a general purpose
number type the easiest. WDYT?

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue_unittest.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue_unittest.cc:166: //
Completing and removing the second task, which however hasn't started.
On 2015/01/08 06:28:09, hirono wrote:
> nit: the second -> the first?

Done.

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue_unittest.cc:302:
base::RunLoop().RunUntilIdle();
On 2015/01/08 06:28:09, hirono wrote:
> Is this line needed?

Done.

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/throttled_file_system.cc
(right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/throttled_file_system.cc:206: if
(result == base::File::FILE_OK) {
On 2015/01/08 06:28:09, hirono wrote:
> If the file fails to close, the JavaScript code should call close method again
> to close the file?

Good catch. The behavior is not defined. fsp::FileStreamReader and
fsp::FileStreamWriter do not retry, so we should follow this pattern. I filed
crbug.com/447093 to update the documentation.

Fixed here. Done.

[hirono, 2015-01-08 07:35:29]

lgtm with a nit! Thanks!

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:63: if (executed_.size()
== static_cast<size_t>(max_in_parallel_) ||
On 2015/01/08 07:07:50, mtomasz wrote:
> On 2015/01/08 06:28:09, hirono wrote:
> > nit: How about turning max_in_parallel_ into size_t instead of casting?
> 
> I tried it before, but it caused casting in other place from int to size_t.
I'm
> not sure which is better, but in general I find using "int" as a general
purpose
> number type the easiest. WDYT?

I don't have strong opinion, but sometimes we use size_t for the number of
something. Up to you.
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...

[mtomasz, 2015-01-08 08:37:51]

Done. I also updated tests in registry_unittest.cc. PTAL. Thanks!

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/queue.cc (right):

https://codereview.chromium.org/829553002/diff/140001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/queue.cc:63: if (executed_.size()
== static_cast<size_t>(max_in_parallel_) ||
On 2015/01/08 07:35:29, hirono wrote:
> On 2015/01/08 07:07:50, mtomasz wrote:
> > On 2015/01/08 06:28:09, hirono wrote:
> > > nit: How about turning max_in_parallel_ into size_t instead of casting?
> > 
> > I tried it before, but it caused casting in other place from int to size_t.
> I'm
> > not sure which is better, but in general I find using "int" as a general
> purpose
> > number type the easiest. WDYT?
> 
> I don't have strong opinion, but sometimes we use size_t for the number of
> something. Up to you.
>
https://code.google.com/p/chromium/codesearch#chromium/src/base/threading/seq...

I thought about it again, and let's go with size_t as you suggested. I changed
also token token size_t, but kept MountOptions::opened_files_limit and
ProvidedFileSystemInfo::opened_files_limit as int, since we can't store size_t
in preferences (base::DictionaryValue).

[hirono, 2015-01-08 08:57:58]

Thanks. lgtm with a nit!

https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc
(right):

https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc:22:
DCHECK_LE(0, opened_files_limit);
The check should be at #40?
Here opned_files_limit is always 0.

[mtomasz, 2015-01-08 09:01:00]

https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
File chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc
(right):

https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc:22:
DCHECK_LE(0, opened_files_limit);
On 2015/01/08 08:57:58, hirono wrote:
> The check should be at #40?
> Here opned_files_limit is always 0.

Right! Done.

[mtomasz, 2015-01-08 09:01:06]

The CQ bit was checked by mtomasz@chromium.org

[mtomasz, 2015-01-08 09:01:11]

On 2015/01/08 09:01:00, mtomasz wrote:
>
https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
> File chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc
> (right):
> 
>
https://codereview.chromium.org/829553002/diff/180001/chrome/browser/chromeos...
> chrome/browser/chromeos/file_system_provider/provided_file_system_info.cc:22:
> DCHECK_LE(0, opened_files_limit);
> On 2015/01/08 08:57:58, hirono wrote:
> > The check should be at #40?
> > Here opned_files_limit is always 0.
> 
> Right! Done.

Thanks!

[commit-bot: I haz the power, 2015-01-08 09:01:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829553002/200001

[commit-bot: I haz the power, 2015-01-08 09:33:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-01-08 09:33:23]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mtomasz, 2015-01-09 05:21:48]

The CQ bit was checked by mtomasz@chromium.org

[commit-bot: I haz the power, 2015-01-09 05:22:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/829553002/240001

[commit-bot: I haz the power, 2015-01-09 06:18:55]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-01-09 06:19:36]

Patchset 13 (id:??) landed as
https://crrev.com/3dd2edf4a117d03d2ec45bb3dcf205cb7561a7e1
Cr-Commit-Position: refs/heads/master@{#310717}