New issue
Advanced search Search tips

Issue 752144 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 15
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocked on:
issue 736531



Sign in to add a comment

Update Sites docs TaskScheduler migration

Project Member Reported by michae...@chromium.org, Aug 3 2017

Issue description

A couple Sites pages have not been updated in a while. They might be out-of-date with respect to task scheduling and BrowserThread usage.

https://sites.google.com/a/chromium.org/dev/developers/coding-style/important-abstractions-and-data-structures?pli=1

https://sites.google.com/a/chromium.org/dev/developers/cpp-in-chromium-101-codelab?pli=1
 

Comment 1 by tzik@chromium.org, Aug 4 2017

Hm, we should probably convert them to .md and move them to //doc?
Labels: -Pri-3 Pri-2
Well now that you mention //docs...

"Subtle Threading Bugs" [1] and "Threading" [2] are out-of-date too, to the point of having wrong code and outdated suggestions.

E.g.: "file_thread: A general process thread for file operations. When you want to do blocking filesystem operations (for example, requesting an icon for a file type, or writing downloaded files to disk), dispatch to this thread."

and: "You should always use BrowserThread to post tasks between threads."

So IMO, at least a large "This is out-of-date" with links to the correct documentation is called for, given how complicated the state of things already seems [3]. Bumping to Pri-2 because docs are wrong.

[1] https://cs.chromium.org/chromium/src/docs/subtle_threading_bugs.md
[2] https://chromium.googlesource.com/chromium/src/+/master/docs/design/threading.md
[3] https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/RPHKykJ4qb0 ;-)

Comment 3 by gab@chromium.org, Sep 14 2017

Status: Started (was: Untriaged)

Comment 4 by gab@chromium.org, Sep 14 2017

Components: Internals>TaskScheduler
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 22 2017

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

commit 6127a86f3a3b0d4096b3c616d379e088effe8170
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Sep 22 14:33:59 2017

Remove all remaining deprecated Web/BrowserThreads!!

(except BrowserThread::FILE which has a handful use cases left still and
will follow suit shortly)

(BrowserThread::PROCESS_LAUNCHER migration was put on hold because of
Android specific requirements around process launching)

Remaining usage of TestBrowserThreadBundle::REAL_DB_THREAD could simply
be removed as all of the product code was already migrated to TaskScheduler
which always uses real threads in unit tests.

Bug:  689520 , 365909,  752144 
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be

NOPRESUBMIT=True (for content/browser/browser_thread_impl.cc:211)

Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
Reviewed-on: https://chromium-review.googlesource.com/610880
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Dirk Pranke <dpranke@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#503732}
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/PRESUBMIT.py
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/chrome/browser/metrics/thread_watcher.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/chrome/browser/metrics/thread_watcher_report_hang.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/chrome/browser/metrics/thread_watcher_report_hang.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/call_stack_profile_metrics_provider.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/call_stack_profile_params.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/proto/execution_context.proto
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/public/cpp/call_stack_profile_struct_traits.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/components/metrics/public/interfaces/call_stack_profile_collector.mojom
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/browser/browser_main_loop.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/browser/browser_thread_impl.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/public/browser/browser_thread.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/public/test/test_browser_thread_bundle.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/content/public/test/test_browser_thread_bundle.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/docs/design/threading.md
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller_unittest.mm
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/app/web_main_loop.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/public/test/test_web_thread_bundle.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/public/web_thread.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/web_thread_impl.cc
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/ios/web/web_thread_impl.h
[modify] https://crrev.com/6127a86f3a3b0d4096b3c616d379e088effe8170/tools/win/ShowThreadNames/ReadMe.txt

Project Member

Comment 6 by bugdroid1@chromium.org, Sep 28 2017

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

commit 82a076e04a1a13d1c398815189a1147932cfcb5a
Author: Matt Falkenhagen <falken@chromium.org>
Date: Thu Sep 28 08:08:19 2017

Revert "Remove all remaining deprecated Web/BrowserThreads!!"

This reverts commit 6127a86f3a3b0d4096b3c616d379e088effe8170.

Reason for revert: Suspect this caused spike in ThreadWatcher hang reports: crbug.com/768886

Original change's description:
> Remove all remaining deprecated Web/BrowserThreads!!
> 
> (except BrowserThread::FILE which has a handful use cases left still and
> will follow suit shortly)
> 
> (BrowserThread::PROCESS_LAUNCHER migration was put on hold because of
> Android specific requirements around process launching)
> 
> Remaining usage of TestBrowserThreadBundle::REAL_DB_THREAD could simply
> be removed as all of the product code was already migrated to TaskScheduler
> which always uses real threads in unit tests.
> 
> Bug:  689520 , 365909,  752144 
> Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
> Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
> 
> NOPRESUBMIT=True (for content/browser/browser_thread_impl.cc:211)
> 
> Change-Id: Id70b722aa35e8568795d5aac16947392ac1597be
> Reviewed-on: https://chromium-review.googlesource.com/610880
> Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
> Reviewed-by: John Abd-El-Malek <jam@chromium.org>
> Reviewed-by: Dirk Pranke <dpranke@chromium.org>
> Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#503732}

TBR=gab@chromium.org,jam@chromium.org,dpranke@chromium.org,asvitkine@chromium.org,sdefresne@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

NOPRESUBMIT=True (for restoring the deprecated browser threads)

Bug:  689520 , 365909,  752144 , 768886
Change-Id: I53d93572118303743e1dde71b2a473350717b215
Cq-Include-Trybots: master.tryserver.chromium.android:android_cronet_tester
Reviewed-on: https://chromium-review.googlesource.com/689254
Commit-Queue: Matt Falkenhagen <falken@chromium.org>
Reviewed-by: Matt Falkenhagen <falken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#504939}
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/PRESUBMIT.py
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/chrome/browser/metrics/thread_watcher.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/chrome/browser/metrics/thread_watcher_report_hang.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/chrome/browser/metrics/thread_watcher_report_hang.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/call_stack_profile_metrics_provider.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/call_stack_profile_params.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/proto/execution_context.proto
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/public/cpp/call_stack_profile_struct_traits.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/public/cpp/call_stack_profile_struct_traits_unittest.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/components/metrics/public/interfaces/call_stack_profile_collector.mojom
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/browser/browser_main_loop.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/browser/browser_thread_impl.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/browser/browser_thread_impl.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/public/browser/browser_thread.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/public/test/test_browser_thread_bundle.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/content/public/test/test_browser_thread_bundle.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/docs/design/threading.md
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller_unittest.mm
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/app/web_main_loop.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/public/test/test_web_thread_bundle.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/public/web_thread.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/web_thread_impl.cc
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/ios/web/web_thread_impl.h
[modify] https://crrev.com/82a076e04a1a13d1c398815189a1147932cfcb5a/tools/win/ShowThreadNames/ReadMe.txt

Project Member

Comment 7 by bugdroid1@chromium.org, Oct 25 2017

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

commit e9748f27c35ecbbdf99d5552dca5a451706328cb
Author: Gabriel Charette <gab@chromium.org>
Date: Wed Oct 25 19:31:15 2017

Uncontroversial and easy to carve out bits of deprecated BrowserThreads cleanup.

Overall reland is at https://chromium-review.googlesource.com/c/chromium/src/+/705775,
these are the easy bits that can land first (and wrap up contributions to issues
365909 and 752144 as well as to the ios/ side of 689520).

NOPRESUBMIT=TRUE (for BrowserThread presubmit triggered by touching related code)
TBR=jam@chromium.org, jsbell@chromium.org, sdefresne@chromium.org, asvitkine@chromium.org

Bug:  689520 , 365909,  752144 
Change-Id: I9b2b5326a12de5f7ffa22f8ac3332c8e57a6e14f
Reviewed-on: https://chromium-review.googlesource.com/738469
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Sylvain Defresne <sdefresne@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511551}
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/chrome/browser/media/webrtc/webrtc_rtp_dump_writer_unittest.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/chrome/browser/metrics/thread_watcher_report_hang.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/chrome/browser/metrics/thread_watcher_report_hang.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/browser/browser_main_loop.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/browser/browser_thread_unittest.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/browser/fileapi/file_system_operation_runner_unittest.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/browser/indexed_db/indexed_db_internals_ui.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/public/browser/browser_thread.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/public/test/test_browser_thread_bundle.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/content/public/test/test_browser_thread_bundle.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/docs/design/threading.md
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/chrome/browser/ui/settings/password_details_collection_view_controller_unittest.mm
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/chrome/browser/ui/settings/save_passwords_collection_view_controller_unittest.mm
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/app/web_main_loop.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/app/web_main_loop.mm
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/public/test/test_web_thread_bundle.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/public/web_thread.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/test/test_web_thread_bundle.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/web_thread_impl.cc
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/ios/web/web_thread_impl.h
[modify] https://crrev.com/e9748f27c35ecbbdf99d5552dca5a451706328cb/tools/win/ShowThreadNames/ReadMe.txt

Project Member

Comment 8 by bugdroid1@chromium.org, Feb 16 2018

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

commit 90480312989695ffeabbaa3d5a4a947170d4e54c
Author: Gabriel Charette <gab@chromium.org>
Date: Fri Feb 16 15:10:05 2018

Bring useful paragraphs from old threading docs into new threading docs.

As requested @ https://chromium-review.googlesource.com/c/chromium/src/+/738469/8/docs/design/threading.md#b122

Already had "Keep the Browser Responsive" and "Use Sequences instead of Locks"
sections, but amended them with a bit more text from the old docs.

R=jam@chromium.org

Bug:  752144 
Change-Id: I67857ad21f8cc5884aeb65fe08fe184f63deed44
Reviewed-on: https://chromium-review.googlesource.com/921041
Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537307}
[modify] https://crrev.com/90480312989695ffeabbaa3d5a4a947170d4e54c/docs/callback.md
[modify] https://crrev.com/90480312989695ffeabbaa3d5a4a947170d4e54c/docs/threading_and_tasks.md

Comment 9 by gab@chromium.org, May 3 2018

Blockedon: 736531
Status: Fixed (was: Started)
Documentation is up to date as of today.

Sign in to add a comment