Chromium Code Reviews
Help | Chromium Project | Sign in
(368)

Issue 10917234: sync: make scheduling logic and job ownership more obvious. (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year, 7 months ago by timsteele
Modified:
1 year, 5 months ago
Reviewers:
rlarocque, akalin
CC:
chromium-reviews_chromium.org, Raghu Simha, haitaol1, timsteele
Visibility:
Public.

Description

sync: make scheduling logic and job ownership more obvious.

- inlines (and removes) SaveJob, to perform only the relevant "saving" bits
where needed. As it turns out, most of it was only necessary for ShouldRunJob
(which I'd like to rename, as it's destructive), and it's a lot easier to read
what's happening and why. Note also removed TODO at SaveJob callsites.

- inlines (and removes) InitOrCoalescePendingJob to perform only the relevant
bits at each callsite.

- pulls SyncSessionJob into a class, to handle basic responsibilities that need
the job Purpose + session (like "Succeeded()" and "Finish") that were previously
strewn about and partially copy/pasted in the scheduler.

- meticulously transfers ownership (removes linked_ptr usage!) of jobs instead
of making many implicit struct copies, as it resulted in non-obvious behavior.
To do this, ownership is given to the scheduling mechanism (the MessageLoop for
ScheduleSyncSessionJob, WaitInterval::timer for canary jobs), instead of jobs
sitting around without knowing whether they're scheduled or not. There are a
few cases where we can't actually "schedule" a job -- which wasn't even obvious
from the old code, and other interesting revelations, like fact that we were
actually pre-empt nudge canary jobs and "unscheduling" them in certain cases.

- removes DoPendingJobIfPossible, since it is no longer needed for
DoCanaryJob/Unthrottle cases, and make the behavior more explicit in the other
cases (mode-switch and SCM error change), see TakePendingJobForCurrentMode.

- removes the ability to create jobs as canary, since this wasn't needed and
was adding complexity (see DoCanaryJob / GrantCanaryPrivilege).

- removes retry_scheduled as it wasn't used anywhere

- adds lots of comments.

Also discovered that THROTTLED intervals seem to never fire a canary job with
today's code (broken), config jobs are immune to per-data-type throttling, and
the had_nudge allowance was defeated by extra IsBackingOff check in
ScheduleNudgeImpl (see comment on review).

BUG=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=164565

Patch Set 1 : #

Total comments: 1

Patch Set 2 : self review #

Patch Set 3 : test + comment + rebase #

Total comments: 59

Patch Set 4 : review #

Patch Set 5 : rebase #

Patch Set 6 : tweak #

Total comments: 19

Patch Set 7 : address review #

Total comments: 16

Patch Set 8 : review #

Patch Set 9 : rebase #

Patch Set 10 : eof #

Unified diffs Side-by-side diffs Delta from patch set Stats (+1092 lines, -567 lines) Lint Patch
M sync/engine/sync_scheduler.h View 1 2 3 4 1 chunk +7 lines, -2 lines 0 comments ? errors Download
M sync/engine/sync_scheduler_impl.h View 1 2 3 4 5 6 7 8 chunks +51 lines, -75 lines 0 comments ? errors Download
M sync/engine/sync_scheduler_impl.cc View 1 2 3 4 5 6 7 27 chunks +356 lines, -341 lines 0 comments ? errors Download
M sync/engine/sync_scheduler_unittest.cc View 1 2 3 4 5 6 7 8 14 chunks +36 lines, -13 lines 0 comments ? errors Download
M sync/engine/sync_scheduler_whitebox_unittest.cc View 1 2 3 4 5 6 7 13 chunks +25 lines, -30 lines 0 comments ? errors Download
A sync/engine/sync_session_job.h View 1 2 3 1 chunk +124 lines, -0 lines 0 comments 1 errors Download
A sync/engine/sync_session_job.cc View 1 2 3 4 5 6 7 1 chunk +184 lines, -0 lines 0 comments 1 errors Download
A sync/engine/sync_session_job_unittest.cc View 1 2 3 4 5 6 7 8 9 1 chunk +239 lines, -0 lines 0 comments 4 errors Download
M sync/engine/syncer.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments ? errors Download
M sync/engine/syncer.cc View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -7 lines 0 comments ? errors Download
M sync/engine/syncer_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -8 lines 0 comments ? errors Download
M sync/internal_api/js_sync_manager_observer_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments ? errors Download
M sync/internal_api/public/sessions/model_neutral_state.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M sync/internal_api/public/sessions/model_neutral_state.cc View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments ? errors Download
M sync/internal_api/public/sessions/sync_session_snapshot.h View 1 2 3 4 5 6 3 chunks +1 line, -4 lines 0 comments ? errors Download
M sync/internal_api/public/sessions/sync_session_snapshot.cc View 1 2 3 4 5 6 4 chunks +1 line, -8 lines 0 comments ? errors Download
M sync/internal_api/public/util/syncer_error.h View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments ? errors Download
M sync/sessions/session_state_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments ? errors Download
M sync/sessions/sync_session.h View 1 2 3 4 5 6 4 chunks +10 lines, -29 lines 0 comments ? errors Download
M sync/sessions/sync_session.cc View 1 2 3 4 5 6 5 chunks +16 lines, -42 lines 0 comments ? errors Download
M sync/sessions/sync_session_unittest.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments ? errors Download
M sync/sessions/test_util.cc View 3 chunks +7 lines, -1 line 0 comments ? errors Download
M sync/sync.gyp View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments ? errors Download
Commit:

Messages

Total messages: 23
timsteele
http://codereview.chromium.org/10917234/diff/2001/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (left): http://codereview.chromium.org/10917234/diff/2001/sync/engine/sync_scheduler_impl.cc#oldcode655 sync/engine/sync_scheduler_impl.cc:655: if (IsBackingOff() && delay > TimeDelta::FromSeconds(1)) { Two cases ...
1 year, 7 months ago #1
timsteele
Ready for review. Asking for double review - initially I thought Richard could focus on ...
1 year, 7 months ago #2
rlarocque
Still reviewing. Hitting publish early because this could take a while. http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h File sync/engine/sync_scheduler.h (right): ...
1 year, 7 months ago #3
rlarocque
We should talk in person/over VC about some of the job ownership semantics. I intend ...
1 year, 7 months ago #4
rlarocque
http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_impl.h File sync/engine/sync_scheduler_impl.h (right): http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_impl.h#newcode305 sync/engine/sync_scheduler_impl.h:305: scoped_ptr<SyncSessionJob> unscheduled_nudge_storage_; I didn't see anywhere in this patch ...
1 year, 7 months ago #5
akalin
no substantial comments yet. Still digesting... http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h File sync/engine/sync_scheduler.h (right): http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler.h#newcode88 sync/engine/sync_scheduler.h:88: // NOTE: |delay| ...
1 year, 7 months ago #6
akalin
A more substantial comment/question. http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (right): http://codereview.chromium.org/10917234/diff/7029/sync/engine/sync_scheduler_impl.cc#newcode775 sync/engine/sync_scheduler_impl.cc:775: FinishSyncSessionJob(job.get(), premature_exit); can you make ...
1 year, 6 months ago #7
timsteele
Addressed all comments. Please take a look, also, happy to chat in person about future ...
1 year, 6 months ago #8
rlarocque
A few comments. Some are in response to the old diff, others on the new. ...
1 year, 6 months ago #9
timsteele
Addressed Richard's followups, though no new code here (I filed + updated bugs). Fred, have ...
1 year, 6 months ago #10
rlarocque
I still haven't reviewed the job ownership semantics very thoroughly, though I did take a ...
1 year, 6 months ago #11
timsteele
ping!
1 year, 6 months ago #12
akalin
On 2012/10/16 17:29:27, timsteele wrote: > ping! will look later today!
1 year, 6 months ago #13
akalin
Few more comments, some possibly naive questions. http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (right): http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc#newcode368 sync/engine/sync_scheduler_impl.cc:368: SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* ...
1 year, 6 months ago #14
timsteele
http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (right): http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc#newcode368 sync/engine/sync_scheduler_impl.cc:368: SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* job) { On 2012/10/17 17:39:17, akalin wrote: ...
1 year, 6 months ago #15
akalin
http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (right): http://codereview.chromium.org/10917234/diff/35003/sync/engine/sync_scheduler_impl.cc#newcode368 sync/engine/sync_scheduler_impl.cc:368: SyncSchedulerImpl::DecideWhileInWaitInterval(const SyncSessionJob* job) { On 2012/10/17 20:33:32, timsteele wrote: ...
1 year, 6 months ago #16
timsteele
Addressed Fred's last review comments as per offline discussion, by splitting ShouldRunJobSaveIfNecessary into DecideOnJob and ...
1 year, 6 months ago #17
akalin
okay i think this lgtm after nits http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler_impl.cc File sync/engine/sync_scheduler_impl.cc (right): http://codereview.chromium.org/10917234/diff/51001/sync/engine/sync_scheduler_impl.cc#newcode278 sync/engine/sync_scheduler_impl.cc:278: pending->mutable_session()->Coalesce(*(session)); just ...
1 year, 5 months ago #18
timsteele
Addressed all your points as requested minus one, which I commented below. I'm planning to ...
1 year, 5 months ago #19
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/56004
1 year, 5 months ago #20
I haz the power (commit-bot)
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
1 year, 5 months ago #21
I haz the power (commit-bot)
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/68001
1 year, 5 months ago #22
I haz the power (commit-bot)
1 year, 5 months ago #23
Change committed as 164565
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 1280:2d3e6564b7b6