Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(66)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 386523002: [SyncFS] Refine SyncProcessRunner's throttling algorithm for parallel task support (Closed)

Created:
6 years, 5 months ago by tzik
Modified:
6 years, 5 months ago
Reviewers:
peria, nhiroki
CC:
chromium-reviews, kinuko+fileapi, nhiroki, tzik
Project:
chromium
Visibility:
Public.

Description

[SyncFS] Refine SyncProcessRunner's throttling algorithm for parallel task support This CL extends task throttling algorithm to multiple task case. Comparing to previous algorithm, new one backs off the throttling duration by elapsed time. So that consecutive failure doesn't throttle the next task too long time. BUG=344769 TEST=unit_tests --gtest_filter="SyncProcessRunnerTest.*" Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282934

Patch Set 1 #

Patch Set 2 : #

Patch Set 3 : fix #

Patch Set 4 : rebase #

Total comments: 2

Patch Set 5 : #

Patch Set 6 : rebase #

Patch Set 7 : drop unused |delay| #

Patch Set 8 : fix #

Total comments: 2

Patch Set 9 : s/int/size_t/ #

Patch Set 10 : #

Total comments: 2

Patch Set 11 : s/deque/queue/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+205 lines, -72 lines) Patch
M chrome/browser/sync_file_system/sync_file_system_service.cc View 1 2 chunks +2 lines, -4 lines 0 comments Download
M chrome/browser/sync_file_system/sync_process_runner.h View 1 2 3 4 5 6 7 8 3 chunks +17 lines, -6 lines 0 comments Download
M chrome/browser/sync_file_system/sync_process_runner.cc View 1 2 3 4 5 6 7 8 3 chunks +89 lines, -53 lines 0 comments Download
M chrome/browser/sync_file_system/sync_process_runner_unittest.cc View 1 2 3 4 5 6 7 8 9 10 4 chunks +97 lines, -9 lines 0 comments Download

Messages

Total messages: 12 (0 generated)
tzik
PTL
6 years, 5 months ago (2014-07-10 06:35:46 UTC) #1
peria
lgtm
6 years, 5 months ago (2014-07-10 07:24:49 UTC) #2
nhiroki
Looks good overall. Please explain the algorithm in the description as you done in the ...
6 years, 5 months ago (2014-07-10 08:17:22 UTC) #3
tzik
I updated the implementation and added a test for it. Could you take another look? ...
6 years, 5 months ago (2014-07-11 06:00:48 UTC) #4
nhiroki
lgtm https://codereview.chromium.org/386523002/diff/130001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc File chrome/browser/sync_file_system/sync_process_runner_unittest.cc (right): https://codereview.chromium.org/386523002/diff/130001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc#newcode267 chrome/browser/sync_file_system/sync_process_runner_unittest.cc:267: fake_timer->GetCurrentDelay()); How about testing DelayMax case, that is, ...
6 years, 5 months ago (2014-07-14 02:27:06 UTC) #5
tzik
Peria-san, I modified the CL after your review significantly. Could you take another look? https://codereview.chromium.org/386523002/diff/130001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc ...
6 years, 5 months ago (2014-07-14 04:52:04 UTC) #6
peria
lgtm https://codereview.chromium.org/386523002/diff/170001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc File chrome/browser/sync_file_system/sync_process_runner_unittest.cc (right): https://codereview.chromium.org/386523002/diff/170001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc#newcode7 chrome/browser/sync_file_system/sync_process_runner_unittest.cc:7: #include <deque> no mandatory: <queue>?
6 years, 5 months ago (2014-07-14 05:19:30 UTC) #7
tzik
https://codereview.chromium.org/386523002/diff/170001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc File chrome/browser/sync_file_system/sync_process_runner_unittest.cc (right): https://codereview.chromium.org/386523002/diff/170001/chrome/browser/sync_file_system/sync_process_runner_unittest.cc#newcode7 chrome/browser/sync_file_system/sync_process_runner_unittest.cc:7: #include <deque> On 2014/07/14 05:19:30, peria wrote: > no ...
6 years, 5 months ago (2014-07-14 06:12:49 UTC) #8
tzik
The CQ bit was checked by tzik@chromium.org
6 years, 5 months ago (2014-07-14 06:12:53 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/386523002/190001
6 years, 5 months ago (2014-07-14 06:13:38 UTC) #10
commit-bot: I haz the power
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win_chromium_rel on tryserver.chromium ...
6 years, 5 months ago (2014-07-14 10:24:33 UTC) #11
commit-bot: I haz the power
6 years, 5 months ago (2014-07-14 12:16:01 UTC) #12
Message was sent while issue was closed. 
Change committed as 282934

Powered by Google App Engine
This is Rietveld 408576698