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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 years, 11 months ago by tim (not reviewing)
Modified:
2 years, 10 months ago
Reviewers:
rlarocque, akalin
CC:
chromium-reviews, Raghu Simha, haitaol1, tim (not reviewing)
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) Patch
M sync/engine/sync_scheduler.h View 1 2 3 4 1 chunk +7 lines, -2 lines 0 comments Download
M sync/engine/sync_scheduler_impl.h View 1 2 3 4 5 6 7 8 chunks +51 lines, -75 lines 0 comments Download
M sync/engine/sync_scheduler_impl.cc View 1 2 3 4 5 6 7 27 chunks +356 lines, -341 lines 0 comments 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 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 Download
A sync/engine/sync_session_job.h View 1 2 3 1 chunk +124 lines, -0 lines 0 comments Download
A sync/engine/sync_session_job.cc View 1 2 3 4 5 6 7 1 chunk +184 lines, -0 lines 0 comments 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 Download
M sync/engine/syncer.h View 1 2 3 4 5 6 7 8 1 chunk +3 lines, -1 line 0 comments Download
M sync/engine/syncer.cc View 1 2 3 4 5 6 7 8 2 chunks +6 lines, -7 lines 0 comments Download
M sync/engine/syncer_unittest.cc View 1 2 3 4 5 6 7 8 3 chunks +7 lines, -8 lines 0 comments 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 Download
M sync/internal_api/public/sessions/model_neutral_state.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M sync/internal_api/public/sessions/model_neutral_state.cc View 1 2 3 4 1 chunk +8 lines, -0 lines 0 comments 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 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 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 Download
M sync/sessions/session_state_unittest.cc View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M sync/sessions/sync_session.h View 1 2 3 4 5 6 4 chunks +10 lines, -29 lines 0 comments Download
M sync/sessions/sync_session.cc View 1 2 3 4 5 6 5 chunks +16 lines, -42 lines 0 comments Download
M sync/sessions/sync_session_unittest.cc View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M sync/sessions/test_util.cc View 3 chunks +7 lines, -1 line 0 comments Download
M sync/sync.gyp View 1 2 3 4 5 6 7 8 2 chunks +3 lines, -0 lines 0 comments Download
Project "None" does not have a commit queue.

Messages

Total messages: 23 (0 generated)
tim (not reviewing)
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 ...
2 years, 11 months ago (2012-09-13 09:00:47 UTC) #1
tim (not reviewing)
Ready for review. Asking for double review - initially I thought Richard could focus on ...
2 years, 11 months ago (2012-09-22 01:49:51 UTC) #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): ...
2 years, 11 months ago (2012-09-24 21:15:14 UTC) #3
rlarocque
We should talk in person/over VC about some of the job ownership semantics. I intend ...
2 years, 11 months ago (2012-09-24 21:56:47 UTC) #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 ...
2 years, 11 months ago (2012-09-24 22:01:45 UTC) #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| ...
2 years, 11 months ago (2012-09-25 22:37:24 UTC) #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 ...
2 years, 10 months ago (2012-10-03 00:11:34 UTC) #7
tim (not reviewing)
Addressed all comments. Please take a look, also, happy to chat in person about future ...
2 years, 10 months ago (2012-10-08 00:20:03 UTC) #8
rlarocque
A few comments. Some are in response to the old diff, others on the new. ...
2 years, 10 months ago (2012-10-08 19:18:04 UTC) #9
tim (not reviewing)
Addressed Richard's followups, though no new code here (I filed + updated bugs). Fred, have ...
2 years, 10 months ago (2012-10-11 17:35:14 UTC) #10
rlarocque
I still haven't reviewed the job ownership semantics very thoroughly, though I did take a ...
2 years, 10 months ago (2012-10-11 18:15:08 UTC) #11
tim (not reviewing)
ping!
2 years, 10 months ago (2012-10-16 17:29:27 UTC) #12
akalin
On 2012/10/16 17:29:27, timsteele wrote: > ping! will look later today!
2 years, 10 months ago (2012-10-16 18:00:54 UTC) #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* ...
2 years, 10 months ago (2012-10-17 17:39:17 UTC) #14
tim (not reviewing)
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: ...
2 years, 10 months ago (2012-10-17 20:33:32 UTC) #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: ...
2 years, 10 months ago (2012-10-17 22:55:45 UTC) #16
tim (not reviewing)
Addressed Fred's last review comments as per offline discussion, by splitting ShouldRunJobSaveIfNecessary into DecideOnJob and ...
2 years, 10 months ago (2012-10-24 23:39:17 UTC) #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 ...
2 years, 10 months ago (2012-10-26 06:52:29 UTC) #18
tim (not reviewing)
Addressed all your points as requested minus one, which I commented below. I'm planning to ...
2 years, 10 months ago (2012-10-26 22:52:27 UTC) #19
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/56004
2 years, 10 months ago (2012-10-26 23:11:28 UTC) #20
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
2 years, 10 months ago (2012-10-27 03:09:59 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tim@chromium.org/10917234/68001
2 years, 10 months ago (2012-10-27 23:43:57 UTC) #22
commit-bot: I haz the power
2 years, 10 months ago (2012-10-28 13:05:43 UTC) #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 a4c4f45