New issue
Advanced search Search tips

Issue 749312 link

Starred by 2 users

Issue metadata

Status: Available
Owner: ----
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Feature

Blocking:
issue 749310



Sign in to add a comment

MessageLoop + MessagePump Cleanup and Refactor

Project Member Reported by robliao@chromium.org, Jul 26 2017

Issue description

The bug covers MessageLoop and MessagePump cleanup during the code audit.

Design and Planning Doc:
https://docs.google.com/document/d/1Vy7kz_9evp6xOFRQBD6M1h4qmk4EEBu0cutipQ3e6kA/edit?usp=sharing
 
Project Member

Comment 1 by bugdroid1@chromium.org, Aug 7 2017

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

commit baca1a8cf2f9382857ba94863d94e333674526b9
Author: Robert Liao <robliao@chromium.org>
Date: Mon Aug 07 23:07:30 2017

Modernize and Remove message_loop_test.(cc|h)

message_loop_test.(cc|h) used a preprocessor driven way to run the same
message loop tests for the default, IO, and UI message loops.

This change moves all of those tests to GTEST parameterized tests and
removes a dependency on creating a MessageLoop with a custom
MessagePump.

BUG=749312
TBR=sky@chromium.org
BUILD.gn cleanup for mojo/common/BUILD.gn

NOPRESUBMIT=true
This is just moving around existing tests that use RunLoop::QuitCurrent*().

Change-Id: I8b61aade06d20f366f66ab9b00cc9856ccfde1ec
Reviewed-on: https://chromium-review.googlesource.com/602609
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#492438}
[modify] https://crrev.com/baca1a8cf2f9382857ba94863d94e333674526b9/base/BUILD.gn
[delete] https://crrev.com/413281f9fe19af923931f3b9893c1517a9141bdd/base/message_loop/message_loop_test.cc
[delete] https://crrev.com/413281f9fe19af923931f3b9893c1517a9141bdd/base/message_loop/message_loop_test.h
[modify] https://crrev.com/baca1a8cf2f9382857ba94863d94e333674526b9/base/message_loop/message_loop_unittest.cc
[modify] https://crrev.com/baca1a8cf2f9382857ba94863d94e333674526b9/mojo/common/BUILD.gn

Project Member

Comment 2 by bugdroid1@chromium.org, Aug 14 2017

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

commit 801b1a6478d9626a55cc126579d94cd7ccbfdd32
Author: Robert Liao <robliao@chromium.org>
Date: Mon Aug 14 17:08:01 2017

Fix base_perftests PostTaskTest Crash

IncomingTaskQueues need a call to WillDestroyCurrentMessageLoop before
releasing.

BUG=749312

Change-Id: I5cf63ec6658b567402b528ff6431d0ebc4414030
Reviewed-on: https://chromium-review.googlesource.com/611054
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#494080}
[modify] https://crrev.com/801b1a6478d9626a55cc126579d94cd7ccbfdd32/base/message_loop/message_pump_perftest.cc

Project Member

Comment 3 by bugdroid1@chromium.org, Aug 17 2017

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

commit 48790275181e75b3d8027ad8023dfeb08e096cd9
Author: Robert Liao <robliao@chromium.org>
Date: Thu Aug 17 22:47:39 2017

Remove MessageLoop HasHighResolutionTasks() and Fix the High Resolution Timer Unit Test

No one was calling this method in production and the unit test really
just wants to test to see if high resolution timers are in use.

BUG=749312

Change-Id: Icbc5c0bd265dbf45f0e0b1a8c8c083afdb135110
Reviewed-on: https://chromium-review.googlesource.com/618143
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#495361}
[modify] https://crrev.com/48790275181e75b3d8027ad8023dfeb08e096cd9/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/48790275181e75b3d8027ad8023dfeb08e096cd9/base/message_loop/incoming_task_queue.h
[modify] https://crrev.com/48790275181e75b3d8027ad8023dfeb08e096cd9/base/message_loop/message_loop.cc
[modify] https://crrev.com/48790275181e75b3d8027ad8023dfeb08e096cd9/base/message_loop/message_loop.h
[modify] https://crrev.com/48790275181e75b3d8027ad8023dfeb08e096cd9/base/message_loop/message_loop_unittest.cc

Labels: -Pri-3 Pri-2
Project Member

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

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

commit 97621d9c31b719379ccdabb8220cca66aad58b8e
Author: Robert Liao <robliao@chromium.org>
Date: Fri Sep 01 17:05:18 2017

Move the TaskAnnotator from MessageLoop to IncomingTaskQueue

The IncomingTaskQueue will be taking more responsibility of processing
tasks as part of the TaskScheduler integration.

BUG=749312

Change-Id: I9fe2fb00bba2587a06556a408b991332f3c86229
Reviewed-on: https://chromium-review.googlesource.com/639233
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#499222}
[modify] https://crrev.com/97621d9c31b719379ccdabb8220cca66aad58b8e/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/97621d9c31b719379ccdabb8220cca66aad58b8e/base/message_loop/incoming_task_queue.h
[modify] https://crrev.com/97621d9c31b719379ccdabb8220cca66aad58b8e/base/message_loop/message_loop.cc
[modify] https://crrev.com/97621d9c31b719379ccdabb8220cca66aad58b8e/base/message_loop/message_loop.h

Project Member

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

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

commit 4ed5a2c9991c97fa165b8ed6116decac3bc70798
Author: Robert Liao <robliao@chromium.org>
Date: Tue Sep 12 05:13:57 2017

Fix Sequence Checking in IncomingTaskQueue

BUG=749312

Change-Id: I71a3f2e8ff9dd26c5cb4e47605d8a3abda65a7f1
Reviewed-on: https://chromium-review.googlesource.com/658402
Commit-Queue: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501190}
[modify] https://crrev.com/4ed5a2c9991c97fa165b8ed6116decac3bc70798/base/message_loop/incoming_task_queue.cc

Project Member

Comment 7 by bugdroid1@chromium.org, Sep 12 2017

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

commit d301af71fa12496dd9ab550454ed36647ac85af2
Author: Robert Liao <robliao@chromium.org>
Date: Tue Sep 12 23:07:27 2017

Remove ReadWriteLock from IncomingTaskQueue

Equivalent functionality can be achieved by adding a bool without any
additional locking in the IncomingTaskQueue::PostPendingTask path.

One fewer lock is required if there is no need to schedule work.

BUG=749312, 758721 

Change-Id: I812db615ef2d1772c077266240d3a1927c97565c
Reviewed-on: https://chromium-review.googlesource.com/639158
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Francois Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501441}
[modify] https://crrev.com/d301af71fa12496dd9ab550454ed36647ac85af2/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/d301af71fa12496dd9ab550454ed36647ac85af2/base/message_loop/incoming_task_queue.h

Project Member

Comment 8 by bugdroid1@chromium.org, Sep 14 2017

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

commit cd396079070e38e24e88ee9ffda680e525b88978
Author: Robert Liao <robliao@chromium.org>
Date: Thu Sep 14 03:09:58 2017

Add MessageLoopPerfTests

This synthetic benchmark measures the task processing speed of
MessageLoop via PostTask().

BUG=749312

Change-Id: I543b1561619db82e464d7a2d068c483dea48ac86
Reviewed-on: https://chromium-review.googlesource.com/656141
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Francois Doray <fdoray@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501854}
[modify] https://crrev.com/cd396079070e38e24e88ee9ffda680e525b88978/base/BUILD.gn
[add] https://crrev.com/cd396079070e38e24e88ee9ffda680e525b88978/base/message_loop/message_loop_perftest.cc

Project Member

Comment 9 by bugdroid1@chromium.org, Sep 15 2017

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

commit a6d31c02dcf08fe03a0efac62bbdf10ee47b5e21
Author: Robert Liao <robliao@chromium.org>
Date: Fri Sep 15 18:52:18 2017

Cleanup IncomingTaskQueue Declaration

This is a refactor/no-op change.

BUG=749312

Change-Id: Ic4b685d4f527adadd332a921a85bd908e6ae802b
Reviewed-on: https://chromium-review.googlesource.com/648452
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502320}
[modify] https://crrev.com/a6d31c02dcf08fe03a0efac62bbdf10ee47b5e21/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/a6d31c02dcf08fe03a0efac62bbdf10ee47b5e21/base/message_loop/incoming_task_queue.h

Description: Show this description
Project Member

Comment 11 by bugdroid1@chromium.org, Oct 11 2017

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

commit c003d0037127c6295053d108820551ef249eedbf
Author: Robert Liao <robliao@chromium.org>
Date: Wed Oct 11 19:26:51 2017

Refactor MessageLoop Queues to IncomingTaskQueue

This change moves all queue data structures from MessageLoop to
IncomingTaskQueue to prepare for a wholesale replacement of
IncomingTaskQueue with a scheduler-aware task queue.

Two approaches were considered in this refactor:
* A full template based solution with substitutable policies.
* A virtual method based solution (this one).

The full template solution resulted in better performance than the existing
MessageLoop at the cost of code readability.

The virtual method solution resulted in slightly lower performance (on the order
of .05 us per task on a Z840) but more readable code.

Given it's nice to have readable code, this code is transitional, and .05us per
task is not signficiant when we're talking on the order of ms for painting, this
should be fine.

BUG=749312

Change-Id: I0e895ca9ecdf3df321c3a7238f228312823f978b
Reviewed-on: https://chromium-review.googlesource.com/681634
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#508064}
[modify] https://crrev.com/c003d0037127c6295053d108820551ef249eedbf/base/message_loop/incoming_task_queue.cc
[modify] https://crrev.com/c003d0037127c6295053d108820551ef249eedbf/base/message_loop/incoming_task_queue.h
[modify] https://crrev.com/c003d0037127c6295053d108820551ef249eedbf/base/message_loop/message_loop.cc
[modify] https://crrev.com/c003d0037127c6295053d108820551ef249eedbf/base/message_loop/message_loop.h

Project Member

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

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

commit 030e252e81d5a50d3f0ca5562a61c17b5a3b27fd
Author: Robert Liao <robliao@chromium.org>
Date: Fri Feb 16 19:27:45 2018

Rename TaskTracker::RunTask to TaskTracker::RunAndPopNextTask

This change helps disambiguate between TaskTracker::RunTask and the
upcoming TaskTracker::DirectlyRunTask, which will run a task outside
the context of a sequence.

Bug=749312

Change-Id: I9996d8764c5e505325a64dd0a211e8c84393397b
Reviewed-on: https://chromium-review.googlesource.com/923163
Commit-Queue: François Doray <fdoray@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#537376}
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/platform_native_worker_pool_win.cc
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/scheduler_worker.cc
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/task_tracker.cc
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/task_tracker.h
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/task_tracker_posix_unittest.cc
[modify] https://crrev.com/030e252e81d5a50d3f0ca5562a61c17b5a3b27fd/base/task_scheduler/task_tracker_unittest.cc

Project Member

Comment 13 by bugdroid1@chromium.org, Feb 22 2018

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

commit 858c0ce43fc61f9bc7b2dab867293430e853ebb1
Author: Robert Liao <robliao@chromium.org>
Date: Thu Feb 22 04:47:55 2018

Move the AdjustWorkerCapacity PostDelayedTask Out of the SchedulerLock

It is unsafe to hold the SchedulerLock and call PostDelayedTask() on a
task runner as the PostDelayedTask() may itself acquire a
SchedulerLock.

BUG=749312, 813857 

Change-Id: Ie908fc34c0cbf0487de5e7991084e38007f0021d
Reviewed-on: https://chromium-review.googlesource.com/924386
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#538349}
[modify] https://crrev.com/858c0ce43fc61f9bc7b2dab867293430e853ebb1/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/858c0ce43fc61f9bc7b2dab867293430e853ebb1/base/task_scheduler/scheduler_worker_pool_impl.h

Project Member

Comment 14 by bugdroid1@chromium.org, Mar 27 2018

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

commit 089385b65f91ae1ec33f281ec68cb6a52ef247be
Author: Robert Liao <robliao@chromium.org>
Date: Tue Mar 27 16:29:13 2018

Convert BrowsingDataRemover Test Utils from MessageLoopRunner to RunLoop

This makes migrating to FlushAsyncForTesting() clearer.

BUG=668707,749312

Change-Id: I074c71c21e2cbbc3fc2b1ea87d08ebd957b6c7f7
Reviewed-on: https://chromium-review.googlesource.com/978839
Reviewed-by: Scott Violet <sky@chromium.org>
Commit-Queue: Robert Liao <robliao@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546130}
[modify] https://crrev.com/089385b65f91ae1ec33f281ec68cb6a52ef247be/content/public/test/browsing_data_remover_test_util.cc
[modify] https://crrev.com/089385b65f91ae1ec33f281ec68cb6a52ef247be/content/public/test/browsing_data_remover_test_util.h

Project Member

Comment 15 by bugdroid1@chromium.org, Apr 3 2018

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

commit c5f3562059310ff33a9cecb95eb6e5c9de421dad
Author: Robert Liao <robliao@chromium.org>
Date: Tue Apr 03 20:41:05 2018

Migrate BrowsingDataRemover TestUtils from FlushForTesting to FlushAsyncForTesting

ProfileHelperTest.DeleteInactiveProfile and UkmBrowserTest.HistoryDeleteCheck
need the waiting thread to process tasks or they will deadlock.

BUG=749312

Change-Id: I9d18fd55647d32e6e6a33a72be2a0fa8fcd89524
Reviewed-on: https://chromium-review.googlesource.com/988243
Commit-Queue: Robert Liao <robliao@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#547819}
[modify] https://crrev.com/c5f3562059310ff33a9cecb95eb6e5c9de421dad/content/public/test/browsing_data_remover_test_util.cc
[modify] https://crrev.com/c5f3562059310ff33a9cecb95eb6e5c9de421dad/content/public/test/browsing_data_remover_test_util.h

Comment 16 by gab@chromium.org, May 7 2018

Labels: -Pri-2 Pri-3
Owner: ----
Status: Available (was: Assigned)
Dropping all top-level features to P3 while reorganizing.
Project Member

Comment 17 by bugdroid1@chromium.org, May 23 2018

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

commit a923acb1002ef1ee0ad0fe57f022ac5969017699
Author: Wez <wez@chromium.org>
Date: Wed May 23 22:26:11 2018

Update ExtensionTestMessageListener to quit internal RunLoops properly.

Migrate the helper from using RunLoop::QuitCurrentWhenIdleDeprecated()
to the safer QuitWhenIdleClosure().

Bug: 749312
Change-Id: Iabf72ec460bed51307074aa2af85123eb647efaf
Reviewed-on: https://chromium-review.googlesource.com/1070604
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#561281}
[modify] https://crrev.com/a923acb1002ef1ee0ad0fe57f022ac5969017699/extensions/test/extension_test_message_listener.cc
[modify] https://crrev.com/a923acb1002ef1ee0ad0fe57f022ac5969017699/extensions/test/extension_test_message_listener.h

Project Member

Comment 18 by bugdroid1@chromium.org, May 25 2018

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

commit 25fd35f2fb3113198c239b3b99ea732b4d3a43c6
Author: Wez <wez@chromium.org>
Date: Fri May 25 22:57:41 2018

Quit browser RunLoop via injected Closure rather than via QuitCurrent*.

Use of QuitCurrent*Deprecated() is ambiguous, since RunLoops can be
nested, and so is unsafe to use in general.

TBR: bartfab
Bug:  845966 ,  844016 , 749312
Change-Id: I537862ea02c4ed67887db32993f40e53f614b3cd
Reviewed-on: https://chromium-review.googlesource.com/1019967
Reviewed-by: Wez <wez@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Trent Apted <tapted@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#562046}
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/apps/app_shim/app_shim_quit_interactive_uitest_mac.mm
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/apps/app_window_interactive_uitest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/browser_process_impl.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/browser_process_impl.h
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chrome_browser_main.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chrome_browser_main.h
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/login/auto_launched_kiosk_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/login/kiosk_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/login/oobe_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/policy/device_system_use_24hour_clock_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/policy/display_rotation_default_handler_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/policy/unaffiliated_arc_allowed_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/policy/user_cloud_policy_manager_chromeos_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/chromeos/shutdown_policy_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/ui/cocoa/first_run_dialog.mm
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/browser/ui/tab_modal_confirm_dialog_browsertest.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/test/base/in_process_browser_test.cc
[modify] https://crrev.com/25fd35f2fb3113198c239b3b99ea732b4d3a43c6/chrome/test/base/in_process_browser_test.h

Sign in to add a comment