Skip to content

Commit

Permalink
[IOTask] Throttle IOTask events by 200ms
Browse files Browse the repository at this point in the history
This mitigates the bug linked below to the point
where the UI does not freeze or slow down at all
when copying 5+ large files (~200mb) simultaneously.

Manually tested on a kevin device.

Bug: b:268422173
Test: unit_tests *IOTaskControllerTest*
Change-Id: I4255a7d201478b9604c988988108fcc907057c25
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4247884
Commit-Queue: Marcello Salomao <msalomao@google.com>
Reviewed-by: Luciano Pacheco <lucmult@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1107285}
  • Loading branch information
Marcello Salomao authored and Chromium LUCI CQ committed Feb 19, 2023
1 parent 9dfc746 commit c0d92b5
Show file tree
Hide file tree
Showing 3 changed files with 31 additions and 15 deletions.
28 changes: 24 additions & 4 deletions chrome/browser/ash/file_manager/io_task_controller.cc
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@

#include "base/task/bind_post_task.h"
#include "base/task/sequenced_task_runner.h"
#include "base/time/time.h"
#include "content/public/browser/device_service.h"
#include "services/device/public/mojom/wake_lock_provider.mojom.h"

namespace file_manager {

namespace io_task {

constexpr auto kThrottleInterval = base::Milliseconds(200);

IOTaskController::IOTaskController() {
DETACH_FROM_SEQUENCE(sequence_checker_);
}
Expand All @@ -21,15 +24,27 @@ IOTaskController::~IOTaskController() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
}

void IOTaskController::MaybeNotifyIOTaskObservers(
const ProgressStatus& status) {
auto last_update = tasks_last_update_[status.task_id];

if (base::Time::Now() - last_update < kThrottleInterval) {
return;
}

NotifyIOTaskObservers(status);
}

void IOTaskController::NotifyIOTaskObservers(const ProgressStatus& status) {
for (IOTaskController::Observer& observer : observers_) {
observer.OnIOTaskStatus(status);
}
tasks_last_update_[status.task_id] = base::Time::Now();
}

void IOTaskController::OnIOTaskProgress(const ProgressStatus& status) {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
NotifyIOTaskObservers(status);
MaybeNotifyIOTaskObservers(status);
}

void IOTaskController::OnIOTaskComplete(IOTaskId task_id,
Expand Down Expand Up @@ -59,6 +74,10 @@ IOTaskId IOTaskController::Add(std::unique_ptr<IOTask> task) {
// Notify observers that the task has been queued.
NotifyIOTaskObservers(task->progress());

// Make sure the first "in progress" event after "queued" is always sent.
// Some listeners require at least one in progress event.
tasks_last_update_[task_id] -= kThrottleInterval;

// TODO(b/199807189): Queue the task.
PutIOTask(task_id, std::move(task))
->Execute(base::BindRepeating(&IOTaskController::OnIOTaskProgress,
Expand Down Expand Up @@ -98,9 +117,9 @@ void IOTaskController::Cancel(IOTaskId task_id) {
void IOTaskController::ProgressPausedTasks() {
DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);

// TODO(b/255264604): TaskId order is potentially racey when mutliple files
// app windows open. Fix this: develop a concept of the current PAUSED task
// in this code, and always progress that task.
// TODO(b/255264604): TaskId order is potentially racey when multiple
// files app windows open. Fix this: develop a concept of the current
// PAUSED task in this code, and always progress that task.
for (auto it = tasks_.begin(); it != tasks_.end(); ++it) {
IOTask* task = it->second.get();
if (task->progress().IsPaused()) {
Expand Down Expand Up @@ -138,6 +157,7 @@ IOTask* IOTaskController::PutIOTask(const IOTaskId task_id,
}

void IOTaskController::RemoveIOTask(const IOTaskId task_id) {
tasks_last_update_.erase(task_id);
tasks_.erase(task_id);

// TODO(b/255264604): fix me: PAUSED tasks can hold the wake lock and
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/ash/file_manager/io_task_controller.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,13 @@
#include <map>
#include <memory>

#include "base/containers/flat_map.h"
#include "base/functional/callback.h"
#include "base/memory/weak_ptr.h"
#include "base/observer_list.h"
#include "base/sequence_checker.h"
#include "base/task/sequenced_task_runner.h"
#include "base/time/time.h"
#include "chrome/browser/ash/file_manager/io_task.h"
#include "mojo/public/cpp/bindings/remote.h"
#include "services/device/public/mojom/wake_lock.mojom.h"
Expand Down Expand Up @@ -62,6 +64,7 @@ class IOTaskController {
}

private:
void MaybeNotifyIOTaskObservers(const ProgressStatus& status);
void NotifyIOTaskObservers(const ProgressStatus& status);
void OnIOTaskProgress(const ProgressStatus& status);
void OnIOTaskComplete(IOTaskId task_id, ProgressStatus status);
Expand Down Expand Up @@ -90,6 +93,9 @@ class IOTaskController {

int wake_lock_counter_for_tests_ = 0;

// A map of when each task has been last notified to its observers.
base::flat_map<IOTaskId, base::Time> tasks_last_update_;

base::WeakPtrFactory<IOTaskController> weak_ptr_factory_{this};
};

Expand Down
12 changes: 1 addition & 11 deletions chrome/browser/ash/file_manager/io_task_controller_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -82,17 +82,7 @@ TEST_F(IOTaskControllerTest, SimpleQueueing) {
std::make_unique<DummyIOTask>(source_urls, dest, OperationType::kCopy));
EXPECT_EQ(1, io_task_controller_.wake_lock_counter_for_tests());

// Wait for the two callbacks posted to the main sequence to finish.
{
base::RunLoop run_loop;
EXPECT_CALL(observer,
OnIOTaskStatus(AllOf(
Field(&ProgressStatus::state, State::kInProgress),
Field(&ProgressStatus::task_id, task_id), base_matcher)))
.WillOnce(RunClosure(run_loop.QuitClosure()));
run_loop.Run();
}

// Wait for the callbacks posted to the main sequence to finish.
{
base::RunLoop run_loop;
EXPECT_CALL(observer, OnIOTaskStatus(AllOf(
Expand Down

0 comments on commit c0d92b5

Please sign in to comment.