New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 846238 link

Starred by 1 user
Status: Fixed
Owner:
treib@chromium.org
Closed: Jul 30
Cc:
mastiz@chromium.org
treib@chromium.org
█ tschumann@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

Sync: don't needlessly enforce thread-affinity

Project Member Reported by treib@chromium.org, May 24 2018 
Currently, ProfileSyncService and a bunch of other Sync classes use ThreadChecker, ThreadTaskRunnerHandle etc, which enforce thread-affinity. As far as I have seen, nothing actually depends on thread-affinity though, sequence-affinity should be enough.

The comment on ThreadChecker explains the difference:
https://cs.chromium.org/chromium/src/base/threading/thread_checker.h?rcl=d0f93908cfdf65236cf0bd33405c4b93bff204af&l=12
Project Member

Comment 1 by bugdroid1@chromium.org, May 24 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d396c10afe483fd24eb0204883393ae8efa9ea7a

commit d396c10afe483fd24eb0204883393ae8efa9ea7a
Author: Marc Treib <treib@chromium.org>
Date: Thu May 24 10:54:03 2018

ProfileSyncService: only require sequence-affinity, not thread-affinity

Nothing here requires thread-affinity, so let's relax the restriction
and go with the recommended way.

This includes using a SequenceChecker instead of a ThreadChecker, and
using SequencedTaskRunnerHandler::Get() instead of
ThreadTaskRunnerHandler::Get().

Bug:  846238 
Change-Id: I6175c7cff26b4dad1329bd22bef816a0ed33e320
Reviewed-on: https://chromium-review.googlesource.com/1069358
Commit-Queue: Marc Treib <treib@chromium.org>
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561455}
[modify] https://crrev.com/d396c10afe483fd24eb0204883393ae8efa9ea7a/components/browser_sync/profile_sync_service.cc
[modify] https://crrev.com/d396c10afe483fd24eb0204883393ae8efa9ea7a/components/browser_sync/profile_sync_service.h
[modify] https://crrev.com/d396c10afe483fd24eb0204883393ae8efa9ea7a/components/sync/base/bind_to_task_runner.h
Project Member

Comment 2 by bugdroid1@chromium.org, May 24 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0f1693ae51c93d6ceb81405a599796432404386c

commit 0f1693ae51c93d6ceb81405a599796432404386c
Author: Marc Treib <treib@chromium.org>
Date: Thu May 24 13:18:08 2018

Sync: don't require thread-affinity in DataTypeController and subclasses

This replaces a ThreadChecker with a SequenceChecker, and a bunch of
ThreadTaskRunnerHandle::Get() calls with SequencedTaskRunnerHandle::Get().

It includes changes in the DataTypeController subclasses
ModelTypeController and AsyncDirectoryTypeController, and also (not a
subclass but related) in DataTypeErrorHandlerImpl.

Bug:  846238 
Change-Id: Ib7158c5e5c8a6c06a5b25f89bdb81ce5aa2ad914
Reviewed-on: https://chromium-review.googlesource.com/1070971
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561481}
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/driver/async_directory_type_controller.cc
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/driver/data_type_controller.cc
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/driver/frontend_data_type_controller.cc
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/driver/model_type_controller.cc
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/model/data_type_error_handler_impl.cc
[modify] https://crrev.com/0f1693ae51c93d6ceb81405a599796432404386c/components/sync/model/data_type_error_handler_impl.h
Project Member

Comment 3 by bugdroid1@chromium.org, May 24 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4464df06ad5b7a258f821aaf773b1395ca04f43f

commit 4464df06ad5b7a258f821aaf773b1395ca04f43f
Author: Marc Treib <treib@chromium.org>
Date: Thu May 24 13:25:39 2018

Cleanup: Remove syncer::BindToCurrentThread

It's not used anymore, all calls have been replaced by
BindToCurrentSequence.
This also changes all unit tests to exercise BindToCurrentSequence,
and adds "Repeating" to Callback and Bind.

Bug:  846238 
Change-Id: Iec89fe4b4587fbdf9049345d5fe3581725caa13b
Reviewed-on: https://chromium-review.googlesource.com/1071629
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561482}
[modify] https://crrev.com/4464df06ad5b7a258f821aaf773b1395ca04f43f/components/sync/base/bind_to_task_runner.h
[modify] https://crrev.com/4464df06ad5b7a258f821aaf773b1395ca04f43f/components/sync/base/bind_to_task_runner_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Jun 21 2018 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d0e129278e83bb3dba7fc4cf44fb8b916f8cb52d

commit d0e129278e83bb3dba7fc4cf44fb8b916f8cb52d
Author: Marc Treib <treib@chromium.org>
Date: Thu Jun 21 13:48:32 2018

Various small cleanups in Sync's StartController

This reorders and renames some members, extracts logic into helpers,
removes an unnecessary ForTest method, etc. No functional changes.

While we're here, also switch from ThreadTaskRunnerHandle to
SequencedTaskRunnerHandle - nothing here requires thread affinity.

Bug:  846238 ,  854978 
Change-Id: Iaeaf7664ec78a23acbc0dfaf84122dea9ca4ef38
Reviewed-on: https://chromium-review.googlesource.com/1109897
Reviewed-by: Jan Krcal <jkrcal@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#569245}
[modify] https://crrev.com/d0e129278e83bb3dba7fc4cf44fb8b916f8cb52d/components/sync/driver/startup_controller.cc
[modify] https://crrev.com/d0e129278e83bb3dba7fc4cf44fb8b916f8cb52d/components/sync/driver/startup_controller.h
[modify] https://crrev.com/d0e129278e83bb3dba7fc4cf44fb8b916f8cb52d/components/sync/driver/startup_controller_unittest.cc

Comment 5 by mastiz@chromium.org, Jul 3

Labels: sync-fixit-2018q3

Comment 6 by treib@chromium.org, Jul 5

Cc: treib@chromium.org
Owner: ----
Status: Available (was: Started)
(unassigning in case somebody else wants to pick it up during the fixit)

Comment 7 by treib@chromium.org, Jul 10

Owner: treib@chromium.org
Status: Started (was: Available)
I'll continue hacking on this for a bit.
Project Member

Comment 8 by bugdroid1@chromium.org, Jul 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/95851f140e1532050909cdef0f5ea4cd128b20fb

commit 95851f140e1532050909cdef0f5ea4cd128b20fb
Author: Marc Treib <treib@chromium.org>
Date: Wed Jul 11 19:44:29 2018

Remove unnecessary thread-affinity in /components/sync*

This CL replaces many instances of ThreadTaskRunnerHandle by
SequencedTaskRunnerHandle, and SingleThreadTaskRunner by
SequencedTaskRunner, in /components/sync/, /components/sync_bookmarks/,
and /components/sync_sessions/.

There are still several ThreadCheckers around which should become
SequenceCheckers. That will follow in another CL soon.

Bug:  846238 
Change-Id: I180185e0e4d380f69246ccf52b6dd9a1b4cfbee8
Reviewed-on: https://chromium-review.googlesource.com/1131474
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574289}
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/backend_migrator.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/data_type_controller.h
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/data_type_manager_impl.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/fake_data_type_controller.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/frontend_data_type_controller_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/glue/sync_backend_host_impl_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/model_type_controller_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/shared_change_processor.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/shared_change_processor.h
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/sync_service_crypto.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/driver/sync_stopped_reporter.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine/fake_sync_manager.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/model_type_connector_proxy.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/model_type_registry.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/sync_encryption_handler_impl.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/sync_encryption_handler_impl.h
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/sync_manager_impl.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/sync_scheduler_impl.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/engine_impl/sync_scheduler_impl_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/model_impl/client_tag_based_model_type_processor.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/model_impl/client_tag_based_model_type_processor_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync/model_impl/model_type_store_impl.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync_bookmarks/bookmark_data_type_controller_unittest.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync_bookmarks/bookmark_model_type_processor.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync_sessions/session_data_type_controller.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync_sessions/session_sync_bridge.cc
[modify] https://crrev.com/95851f140e1532050909cdef0f5ea4cd128b20fb/components/sync_sessions/sessions_sync_manager.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Jul 11 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8c479395ead17e469f8e1406f420a3d058461dc0

commit 8c479395ead17e469f8e1406f420a3d058461dc0
Author: Marc Treib <treib@chromium.org>
Date: Wed Jul 11 20:19:01 2018

Remove unnecessary thread-affinity in /components/browser_sync

This switches a bunch of places in /components/browser_sync from
ThreadTaskRunnerHandle/SingleThreadTaskRunner to
SequencedTaskRunnerHandle/SequencedTaskRunner. A few references to
SingleThreadTaskRunner remain, where it makes sense (e.g. UIModelWorker)
or where lower layers require it (e.g. WebDataServiceBase).

While we're here, this also changes some Callbacks in
SigninConfirmationHelper to OnceCallbacks.

Bug:  846238 
Change-Id: I951d95ec442b6b6df4ba3be08674a9eefd4da02c
Reviewed-on: https://chromium-review.googlesource.com/1131189
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574306}
[modify] https://crrev.com/8c479395ead17e469f8e1406f420a3d058461dc0/components/browser_sync/abstract_profile_sync_service_test.cc
[modify] https://crrev.com/8c479395ead17e469f8e1406f420a3d058461dc0/components/browser_sync/profile_sync_test_util.cc
[modify] https://crrev.com/8c479395ead17e469f8e1406f420a3d058461dc0/components/browser_sync/profile_sync_test_util.h
[modify] https://crrev.com/8c479395ead17e469f8e1406f420a3d058461dc0/components/browser_sync/signin_confirmation_helper.cc
[modify] https://crrev.com/8c479395ead17e469f8e1406f420a3d058461dc0/components/browser_sync/signin_confirmation_helper.h
Project Member

Comment 10 by bugdroid1@chromium.org, Jul 12 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc8a763a002339d6a7821e3c69d45ad8642dafc1

commit fc8a763a002339d6a7821e3c69d45ad8642dafc1
Author: Marc Treib <treib@chromium.org>
Date: Thu Jul 12 06:54:48 2018

Replace ThreadChecker by SequenceChecker in /components/sync*

https://crrev.com/c/1131474 removed the unnecessary thread-affinity,
this one adjusts the checks accordingly.

Bug:  846238 
Change-Id: If4e1145712fe99f4d8236d93e4f645e69c3e7655
Reviewed-on: https://chromium-review.googlesource.com/1131738
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Mikel Astiz <mastiz@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574498}
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/glue/sync_backend_host_core.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/glue/sync_backend_host_core.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/glue/sync_backend_host_impl.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/sync_service_crypto.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/driver/sync_service_crypto.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine/sync_backend_registrar.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine/sync_backend_registrar.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/debug_info_event_listener.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/debug_info_event_listener.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/model_type_worker.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/model_type_worker.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/net/server_connection_manager.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/net/server_connection_manager.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/sync_manager_impl.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/sync_manager_impl.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/engine_impl/sync_manager_impl_unittest.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/test/engine/fake_model_worker.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync/test/engine/fake_model_worker.h
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync_bookmarks/bookmark_change_processor.cc
[modify] https://crrev.com/fc8a763a002339d6a7821e3c69d45ad8642dafc1/components/sync_bookmarks/bookmark_change_processor.h

Comment 11 by treib@chromium.org, Jul 16 

This is mostly done now. Places that still use ThreadTaskRunnerHandle:
- WeakHandleCoreBase [1]
- DirectoryBackingStore [2]
- BookmarkModelAssociator [3]

I suspect that none of these actually required thread-affinity, but some investigation is required.

[1] https://cs.chromium.org/chromium/src/components/sync/base/weak_handle.cc?rcl=db22bae6f7dcdfbdbb26007baaee9b44a05277e4&l=17
[2] https://cs.chromium.org/chromium/src/components/sync/syncable/directory_backing_store.cc?rcl=4a0cf6516ff4f2a8249f479979b8f284c1eab1ee&l=309
[3] https://cs.chromium.org/chromium/src/components/sync_bookmarks/bookmark_model_associator.cc?rcl=e33ca74720ac42b0a8c69eb543261882791da12d&l=890
Project Member

Comment 12 by bugdroid1@chromium.org, Jul 26 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2ff44cdb7defcd1189f55e9002701a09bed81631

commit 2ff44cdb7defcd1189f55e9002701a09bed81631
Author: Marc Treib <treib@chromium.org>
Date: Thu Jul 26 15:56:21 2018

Remove some more unnecessary thread-affinity in components/sync/

This replaces most remaining ThreadTaskRunnerHandle/SingleThreadTaskRunner
by SequencedTaskRunnerHandler/SequencedTaskRunner. There are some places
that actually need thread-affinity (e.g. network stuff), or where it makes
sense (UIModelWorker which is specifically for running on the UI thread).

One remaining place I'm not sure about is WeakHandle.

Bug:  846238 
Change-Id: I704406fbf2c462537971eda0102c40626503b793
Reviewed-on: https://chromium-review.googlesource.com/1151296
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578312}
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/driver/async_directory_type_controller_unittest.cc
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/driver/glue/sync_backend_host_impl.h
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/engine/net/http_bridge.cc
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/engine/sync_backend_registrar_unittest.cc
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/engine/sync_engine.h
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/model/model_type_store_test_util.cc
[modify] https://crrev.com/2ff44cdb7defcd1189f55e9002701a09bed81631/components/sync/model/test_model_type_store_service.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Jul 27 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/89b8e7c6cbb785fb0e940dac887153b4a1469632

commit 89b8e7c6cbb785fb0e940dac887153b4a1469632
Author: Marc Treib <treib@chromium.org>
Date: Fri Jul 27 16:56:20 2018

In syncer::WeakHandle, switch from SingleThreadTaskRunner to SequencedTaskRunner

WeakHandle is a wrapper around WeakPtr, which only cares about
sequence-affinity, not thread-affinity.
Also fixes two unrelated-looking-but-actually-related IWYU violations.

Bug:  846238 
Change-Id: Iafffea16a38728f0c3dd6852954da509e0eab633
Reviewed-on: https://chromium-review.googlesource.com/1152821
Reviewed-by: Mikel Astiz <mastiz@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Commit-Queue: Marc Treib <treib@chromium.org>
Cr-Commit-Position: refs/heads/master@{#578676}
[modify] https://crrev.com/89b8e7c6cbb785fb0e940dac887153b4a1469632/chrome/browser/google/google_search_domain_mixing_metrics_emitter.h
[modify] https://crrev.com/89b8e7c6cbb785fb0e940dac887153b4a1469632/components/browser_sync/profile_sync_components_factory_impl.h
[modify] https://crrev.com/89b8e7c6cbb785fb0e940dac887153b4a1469632/components/sync/base/weak_handle.cc
[modify] https://crrev.com/89b8e7c6cbb785fb0e940dac887153b4a1469632/components/sync/base/weak_handle.h

Comment 14 by treib@chromium.org, Jul 30

Status: Fixed (was: Started)
Let's call this done. DirectoryBackingStore still uses thread-affinity, but SQLite might actually require that. BookmarkModelAssociator still uses thread-affinity and probably doesn't need to, but that class will go away soon with the bookmarks USS migration.
►

Sign in to add a comment