Audio player: Throttle concurrent loading of audio metadata.

Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to 25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f0c44416f9a7bd9328b93456f7f88f5c26461269
Cr-Commit-Position: refs/heads/master@{#428329}

[fukino, 2016-10-27 21:27:47]

Description was changed from

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files.
==========

to

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-10-27 21:28:11]

fukino@chromium.org changed reviewers:
+ oka@chromium.org

[oka, 2016-10-28 07:10:18]

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/js/audio_player.js (right):

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:54: this.loadMetadataQueue_ =
new AsyncUtil.ConcurrentQueue(25);
Could you explain why 25 is chosen?

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:225:
this.loadMetadataQueue_.run(function(trackIndex, callback) {
fetchMetadata_ looks like more appropriate place to have throttle.

[oka, 2016-10-28 07:13:43]

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/js/audio_player.js (right):

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:230: }.bind(this, track));
I wonder if binding |track| is needed.
Can't you use |track| from the closure?

[fukino, 2016-10-28 09:31:51]

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
File ui/file_manager/audio_player/js/audio_player.js (right):

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:54: this.loadMetadataQueue_ =
new AsyncUtil.ConcurrentQueue(25);
On 2016/10/28 07:10:18, oka wrote:
> Could you explain why 25 is chosen?

I elaborated it a bit more.

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:225:
this.loadMetadataQueue_.run(function(trackIndex, callback) {
On 2016/10/28 07:10:18, oka wrote:
> fetchMetadata_ looks like more appropriate place to have throttle.

I think loadMetadata_ is better.
We want to throttle the initial bulk metadata load, but when a user changed the
current track, loading metadata for the current track should be done before we
completely finish the bulk load.
(For example, when there are 1000 tracks in a directory and the user click
"Prev" button after the audio player is opened, the last song will be selected
and we want to show the metadata asap.)

https://codereview.chromium.org/2456153002/diff/1/ui/file_manager/audio_playe...
ui/file_manager/audio_player/js/audio_player.js:230: }.bind(this, track));
On 2016/10/28 07:13:43, oka wrote:
> I wonder if binding |track| is needed.
> Can't you use |track| from the closure?

You're right. Thanks!

[oka, 2016-10-28 09:39:48]

lgtm

[oka, 2016-10-28 09:40:40]

On 2016/10/28 09:39:48, oka wrote:
> lgtm

Could you add how long it took on TEST?

[fukino, 2016-10-28 10:01:58]

Description was changed from

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to
load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[fukino, 2016-10-28 10:02:27]

Thanks!

> Could you add how long it took on TEST?
Done

[fukino, 2016-10-28 10:02:30]

The CQ bit was checked by fukino@chromium.org

[commit-bot: I haz the power, 2016-10-28 10:02:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-28 10:38:30]

Description was changed from

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to
load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to
load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[commit-bot: I haz the power, 2016-10-28 10:38:31]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-28 10:40:29]

Description was changed from

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to
load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
Audio player: Throttle concurrent loading of audio metadata.

When the audio player is opened, it starts loading audio metadata of all files
in the same directory.
This can result in running out of the limit for open file descriptors.
This CL throttles the metadata loading, limiting the number of open files up to
25.

BUG=654769
TEST=manually tested on a folder which has 1500 mp3 files (took 30 seconds to
load all metadata on chell).
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/f0c44416f9a7bd9328b93456f7f88f5c26461269
Cr-Commit-Position: refs/heads/master@{#428329}
==========

[commit-bot: I haz the power, 2016-10-28 10:40:30]

Patchset 2 (id:??) landed as
https://crrev.com/f0c44416f9a7bd9328b93456f7f88f5c26461269
Cr-Commit-Position: refs/heads/master@{#428329}