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

Issue 897751 link

Starred by 2 users
Status: Started
Owner:
█ alexclarke@chromium.org
Last visit > 30 days ago
Cc:
altimin@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocked on:
issue 900219
Blocking:
issue 863341



Sign in to add a comment

The SequenceManager is slow

Project Member Reported by alexclarke@chromium.org, Oct 22 
It's currently nearly 4x the cost of base::MessageLoop on Linux and almost 6x on Android.  Fortunately it looks like we understand most of where the performance is going: 

* On Linux PlatformThread::CurrentId() is surprisingly slow.
* We are collecting several UMAs which are slow. I propose removing them.
* The locking for GracefulQueueShutdownHelper is slow, I propose to use PostTask instead to deal with thread hops.
* ThreadControllerWithMessagePumpImpl::DoWork is unnecessarily calling pump_->ScheduleWork.  All it needs to do is return true.
* SequenceManagerImpl::MainThreadOnly::task_execution_stack ought to be a vector instead of a list to avoid unnecessary memory allocations.
* Recording task start and end time is probably not interesting for non-blink work and isn't for free.

Comment 1 by alexclarke@chromium.org, Oct 22

Blocking: 863341
Project Member

Comment 2 by bugdroid1@chromium.org, Oct 24 

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

commit e023a69e07ce5b4144427188a6452a7b0b3fb09f
Author: Alex Clarke <alexclarke@chromium.org>
Date: Wed Oct 24 14:40:36 2018

Avoid redundant calls to ScheduleWork

There's no need for ThreadControllerWithMessagePumpImpl::DoWork
to call ScheduleWork.  Typically it's called in a for loop, or
something equivalent.

Redundant calls to ScheduleWork are not free and show up on
profiles of SequenceManagerPerfTest.

Bug: 897751
Change-Id: Iafebf0b574d991ea3b89dfc101aa7326cd35b3c1
Reviewed-on: https://chromium-review.googlesource.com/c/1297137
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602332}
[modify] https://crrev.com/e023a69e07ce5b4144427188a6452a7b0b3fb09f/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
Project Member

Comment 3 by bugdroid1@chromium.org, Oct 24 

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

commit a6bce5f003f0fc8cb68e2a47b0ff488fc013aa4c
Author: Alex Clarke <alexclarke@chromium.org>
Date: Wed Oct 24 15:44:54 2018

Remove expensive UMAs from SequenceManager

These UMAs where taking almost 25% of CPU time in our perf test,
lets remove them for now.  We can reland later if we're more
cautious about how frequetly we sample.

Bug: 897751
Change-Id: Ib8cd6b572d818fe62d4b19ede0274916558a3099
Reviewed-on: https://chromium-review.googlesource.com/c/1293910
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602349}
[modify] https://crrev.com/a6bce5f003f0fc8cb68e2a47b0ff488fc013aa4c/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/a6bce5f003f0fc8cb68e2a47b0ff488fc013aa4c/base/task/sequence_manager/task_queue_selector.cc
[modify] https://crrev.com/a6bce5f003f0fc8cb68e2a47b0ff488fc013aa4c/base/task/sequence_manager/task_queue_selector_unittest.cc
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 25 

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

commit 8a72c46da60ffd35322db6dece5bd3f844a0448e
Author: Alex Clarke <alexclarke@chromium.org>
Date: Thu Oct 25 16:08:20 2018

Require TaskQueue (not TaskRunner) shutdown on associated thread

This means we no longer need the GracefulQueueShutdownHelper which
had locks that showed up on profiles.

We also need to modify worker shutdown to delete the WorkerScheduler
on the worker thread.

TBR=gab@chromium.org

Bug: 897751
Change-Id: I002d69761d1d9f46d49768f2a9dc6e2093952f2d
Reviewed-on: https://chromium-review.googlesource.com/c/1293580
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#602730}
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/BUILD.gn
[delete] https://crrev.com/52e5b08f852f91e72598557316ca5d3212420b89/base/task/sequence_manager/graceful_queue_shutdown_helper.cc
[delete] https://crrev.com/52e5b08f852f91e72598557316ca5d3212420b89/base/task/sequence_manager/graceful_queue_shutdown_helper.h
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/sequence_manager_impl.h
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/sequence_manager_impl_unittest.cc
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/task_queue.cc
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/task_queue.h
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/8a72c46da60ffd35322db6dece5bd3f844a0448e/base/task/sequence_manager/test/sequence_manager_for_test.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 26 

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

commit cc3c410dd5a66316c7ec7c13075e9e15d48032d2
Author: Alex Clarke <alexclarke@chromium.org>
Date: Fri Oct 26 11:39:58 2018

SequenceManager: Only record task time if we need to

We don't need task timing on the UI thread, so this patch arranges for
time only to be sampled if we have an observer that needs it.

Bug: 897751
Change-Id: I5915efdb903813a574ca05cee316cb30fa75b39c
Reviewed-on: https://chromium-review.googlesource.com/c/1297425
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603059}
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/base/task/sequence_manager/sequence_manager_impl.h
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/base/task/sequence_manager/sequence_manager_perftest.cc
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_task_queue.cc
[modify] https://crrev.com/cc3c410dd5a66316c7ec7c13075e9e15d48032d2/third_party/blink/renderer/platform/scheduler/worker/non_main_thread_task_queue.cc
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 26 

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

commit 81ee07598ecff4c25d1dcf6f6ef99120a33d2237
Author: Alex Clarke <alexclarke@chromium.org>
Date: Fri Oct 26 14:05:34 2018

Add deduplication logic to ThreadControllerWithMessagePumpImpl

This patch adds ScheduleWork / ScheduleDelayedWork deduplication which
improves the perf test results.

Unlike ThreadControllerImpl this deduplication logic is main thread only
because I'm assuming the overhead of the locks isn't worth it.

Bug: 897751
Change-Id: I879ad904e41c820180712702c66c4afd66cb3fae
Reviewed-on: https://chromium-review.googlesource.com/c/1301462
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603077}
[modify] https://crrev.com/81ee07598ecff4c25d1dcf6f6ef99120a33d2237/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
[modify] https://crrev.com/81ee07598ecff4c25d1dcf6f6ef99120a33d2237/base/task/sequence_manager/thread_controller_with_message_pump_impl.h
Project Member

Comment 7 by bugdroid1@chromium.org, Oct 26 

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

commit d76fd852776eea8670f3c1d13f519227b3de97b8
Author: Alex Clarke <alexclarke@chromium.org>
Date: Fri Oct 26 14:22:42 2018

SequenceManager: Remove IsCancelled crash keys

They didn't reveal any leads, and are somewhat expensive.

Bug: 798554, 897751
Change-Id: Id22b59ccd43b823902c48a4b5f324ccea07e9649
Reviewed-on: https://chromium-review.googlesource.com/c/1301463
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603082}
[modify] https://crrev.com/d76fd852776eea8670f3c1d13f519227b3de97b8/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/d76fd852776eea8670f3c1d13f519227b3de97b8/base/task/sequence_manager/sequence_manager_impl.h
[modify] https://crrev.com/d76fd852776eea8670f3c1d13f519227b3de97b8/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/d76fd852776eea8670f3c1d13f519227b3de97b8/base/task/sequence_manager/work_queue.cc
Project Member

Comment 8 by bugdroid1@chromium.org, Oct 30 

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

commit 9a4121a630766d46f392203b712034007616c824
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Oct 30 14:12:10 2018

Benchmark various types of messageloop

Add menchmarks for UI and IO message loops

Bug: 897751
Change-Id: I7b487055aecfb6a2f486c7d241aaddc8f472813f
Reviewed-on: https://chromium-review.googlesource.com/c/1304521
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603888}
[modify] https://crrev.com/9a4121a630766d46f392203b712034007616c824/base/task/sequence_manager/sequence_manager_perftest.cc
Project Member

Comment 9 by bugdroid1@chromium.org, Oct 30 

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

commit 1ce3a7284c8a38593fb0c38a7b45120a26c48f02
Author: Ioana Pandele <ioanap@chromium.org>
Date: Tue Oct 30 14:58:12 2018

Revert "Benchmark various types of messageloop"

This reverts commit 9a4121a630766d46f392203b712034007616c824.

Reason for revert: Compile failure on mac_dbg
https://logs.chromium.org/logs/chromium/buildbucket/cr-buildbucket.appspot.com/8931231951930679168/+/steps/compile/0/stdout

Original change's description:
> Benchmark various types of messageloop
> 
> Add menchmarks for UI and IO message loops
> 
> Bug: 897751
> Change-Id: I7b487055aecfb6a2f486c7d241aaddc8f472813f
> Reviewed-on: https://chromium-review.googlesource.com/c/1304521
> Commit-Queue: Alex Clarke <alexclarke@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#603888}

TBR=alexclarke@chromium.org,altimin@chromium.org

Change-Id: Id3608cad30a14e68255227d9d51690ec66c86c97
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 897751
Reviewed-on: https://chromium-review.googlesource.com/c/1307506
Reviewed-by: Ioana Pandele <ioanap@chromium.org>
Commit-Queue: Ioana Pandele <ioanap@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603895}
[modify] https://crrev.com/1ce3a7284c8a38593fb0c38a7b45120a26c48f02/base/task/sequence_manager/sequence_manager_perftest.cc
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 30 

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

commit fd9595913a7e219e20ac9178d248ed0776e88e8f
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Oct 30 15:32:46 2018

[Reland] Benchmark various types of messageloop

Add benchmarks for UI and IO message loops.

TBR=altimin@chromium.org

Bug: 897751
Change-Id: I617f329afae84107df3c10c9262139946a6037b9
Reviewed-on: https://chromium-review.googlesource.com/c/1307507
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603906}
[modify] https://crrev.com/fd9595913a7e219e20ac9178d248ed0776e88e8f/base/task/sequence_manager/sequence_manager_perftest.cc

Comment 11 by alexclarke@chromium.org, Oct 30

Blockedon: 900219
Project Member

Comment 12 by bugdroid1@chromium.org, Oct 31 

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

commit 3cf999432eb0ef2e309a93d8d35a241e3c0d9563
Author: Alex Clarke <alexclarke@chromium.org>
Date: Wed Oct 31 11:57:03 2018

Extend the SequenceManagerPerf to support cross thread posting

Cross thread immediate task posting performance matters for the UI
thread and the Blink main thread.  This patch adds a perf test for this
case.

Also refactors things into classes which hopefully makes it a bit more
manageable.

Bug: 863341, 897751
Change-Id: I5cc1492fb1988582085af0c4ff04ac7e729a4fd7
Reviewed-on: https://chromium-review.googlesource.com/c/1307500
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604215}
[modify] https://crrev.com/3cf999432eb0ef2e309a93d8d35a241e3c0d9563/base/task/sequence_manager/sequence_manager_perftest.cc
Project Member

Comment 13 by bugdroid1@chromium.org, Nov 1 

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

commit 2b7d547721dd1caee1a1111b7a97d4445a648ebe
Author: Alex Clarke <alexclarke@chromium.org>
Date: Thu Nov 01 18:42:04 2018

Make DelayedFences an opt in feature because sampling now isn't free

Now you have to set SetDelayedFenceAllowed in the spec if you want to use
delayed fences on a particular TaskQueue.

Bug: 897751
Change-Id: I5ca9869ae6999fde264e2553e451c1b08fe90204
Reviewed-on: https://chromium-review.googlesource.com/c/1309776
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#604651}
[modify] https://crrev.com/2b7d547721dd1caee1a1111b7a97d4445a648ebe/base/task/sequence_manager/sequence_manager_impl_unittest.cc
[modify] https://crrev.com/2b7d547721dd1caee1a1111b7a97d4445a648ebe/base/task/sequence_manager/task_queue.h
[modify] https://crrev.com/2b7d547721dd1caee1a1111b7a97d4445a648ebe/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/2b7d547721dd1caee1a1111b7a97d4445a648ebe/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/2b7d547721dd1caee1a1111b7a97d4445a648ebe/third_party/blink/renderer/platform/scheduler/main_thread/main_thread_task_queue.h
Project Member

Comment 14 by bugdroid1@chromium.org, Nov 2 

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

commit 125179af12bfc17676be47ad4d9d19fb89b312af
Author: Alex Clarke <alexclarke@chromium.org>
Date: Fri Nov 02 19:06:35 2018

Avoid redundant main thread checks when posting tasks

There's no need to do the TLS lookup twice.

Bug: 897751
Change-Id: I628297659ce38971bd16f204e29bace267679b71
Reviewed-on: https://chromium-review.googlesource.com/c/1314593
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605005}
[modify] https://crrev.com/125179af12bfc17676be47ad4d9d19fb89b312af/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/125179af12bfc17676be47ad4d9d19fb89b312af/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/125179af12bfc17676be47ad4d9d19fb89b312af/base/task/sequence_manager/task_queue_proxy.cc
[modify] https://crrev.com/125179af12bfc17676be47ad4d9d19fb89b312af/base/task/sequence_manager/task_queue_proxy.h
Project Member

Comment 15 by bugdroid1@chromium.org, Nov 5 

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

commit 6d22abccbc1a8c398ca3987974f31fcf3eb8a631
Author: Alex Clarke <alexclarke@chromium.org>
Date: Mon Nov 05 13:24:41 2018

TaskQueueImpl: Make sequence_manager const

This simplification is a step towards a lock-free main thread PostTask path.

Split off from: https://chromium-review.googlesource.com/c/chromium/src/+/1304557

Bug: 897751
Change-Id: I6aa2fd98b000b634785f2fee94afa3b11e70423d
Reviewed-on: https://chromium-review.googlesource.com/c/1317568
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605311}
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/base/task/sequence_manager/sequence_manager_impl_unittest.cc
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/base/task/sequence_manager/time_domain_unittest.cc
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/base/task/sequence_manager/work_queue_unittest.cc
[modify] https://crrev.com/6d22abccbc1a8c398ca3987974f31fcf3eb8a631/third_party/blink/renderer/platform/scheduler/worker/worker_thread_scheduler.cc
Project Member

Comment 17 by bugdroid1@chromium.org, Nov 7 

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

commit 2af43e66d1952f61d0b4fc82b865f12724b6407e
Author: Sami Kyostila <skyostil@chromium.org>
Date: Wed Nov 07 01:40:49 2018

Speed up message loop perf test and disable failing waitable event perf test

In order to reduce cycle time on the performance waterfall, drop the
perf test measurement period from 30s to 5s.

Bug: 897751, 848499
Change-Id: I3cd921b327044f02767bc8a43a6e99e5a8a6f48f
Reviewed-on: https://chromium-review.googlesource.com/c/1315967
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605922}
[modify] https://crrev.com/2af43e66d1952f61d0b4fc82b865f12724b6407e/base/message_loop/message_loop_task_runner_perftest.cc
[modify] https://crrev.com/2af43e66d1952f61d0b4fc82b865f12724b6407e/base/synchronization/waitable_event_perftest.cc
Project Member

Comment 18 by bugdroid1@chromium.org, Nov 7 

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

commit 2bde9143df47464017b13df56cacf0c5828eb727
Author: Sami Kyostila <skyostil@chromium.org>
Date: Wed Nov 07 01:43:46 2018

sequence_manager: Speed up SequenceManagerPerfTests

In order to speed up performance waterfall iteration time, instead of
always running for 5 seconds in the SequenceManagerPerfTests, measure
the wall time cost of posting 100k tasks in various configurations. We
also only measure task queue scaling efficiency with specific types of
sequence manager configurations.

Bug: 897751
Change-Id: Iafd1d809bf5116912298b7cad10e00551939e601
Reviewed-on: https://chromium-review.googlesource.com/c/1315887
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605923}
[modify] https://crrev.com/2bde9143df47464017b13df56cacf0c5828eb727/base/task/sequence_manager/sequence_manager_perftest.cc
Project Member

Comment 20 by bugdroid1@chromium.org, Nov 13 

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

commit e66055b18d1f38f6b11fa1e0a2b177dedff26fef
Author: Sami Kyostila <skyostil@chromium.org>
Date: Tue Nov 13 12:49:48 2018

base_perftest: Add data dependency on perf test runner script

This perf test should depend on //testing:run_perf_test in order to have
the correct wrapper scripts present in the isolate.

TBR=fdoray@chromium.org

Bug: 897751
Change-Id: I77d454984f6e8c6550961a6ecb240d201d289476
Reviewed-on: https://chromium-review.googlesource.com/c/1333449
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Commit-Queue: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607569}
[modify] https://crrev.com/e66055b18d1f38f6b11fa1e0a2b177dedff26fef/base/BUILD.gn
Project Member

Comment 21 by bugdroid1@chromium.org, Nov 27 

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

commit f9c4d719618e1c1119edb63a42d9e32d28316dc8
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Nov 27 10:08:50 2018

Avoid pointless new/delete in SequenceManagerImpl::TakeTaskImpl

According to our perf test on linux this shaves off ~0.1 us/task or
about 10% of our current overhead.

Bug: 897751
Change-Id: I79d6cf6bacd2b62be68b7ee028bf6749bec1ff57
Reviewed-on: https://chromium-review.googlesource.com/c/1348097
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611059}
[modify] https://crrev.com/f9c4d719618e1c1119edb63a42d9e32d28316dc8/base/task/sequence_manager/sequence_manager_impl.cc
[modify] https://crrev.com/f9c4d719618e1c1119edb63a42d9e32d28316dc8/base/task/sequence_manager/sequence_manager_impl.h
Project Member

Comment 22 by bugdroid1@chromium.org, Jan 8 

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

commit 0c32a9a06d83f1eccc4e6f65bab9850e30689f53
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Jan 08 17:30:33 2019

Optimise TaskQueueSelector

This patch reimplements the priority selection logic by maintaining a
priority queue of task priorities.  I.e. to select a task we first
check the current highest priority priority with tasks and then select
the oldest queue from within that priority.

To account for task starvation we keep a count of the number of times
we have selected a task, and assign each priority a sort key based on
the current task count plus a priority specific bias.  After selecting
a queue, we re-evaluate the sort key for the associated priority.

This means if we run a lot of say high priority tasks, the sort key for
any waiting normal priority will eventually become lower than the
key for the high priority, resulting in us selecting a "starving"
normal priority queue.

Despite the overhead of maintaining this new queue, it's still makes a
4% improvement on SequenceManagerPerfTest.PostImmediateTasks_OneQueue.

Bug: 897751
Change-Id: I2e9add8e0266f269d98aee15a01e3d457931ea21
Reviewed-on: https://chromium-review.googlesource.com/c/1397612
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620778}
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/task_queue_selector.cc
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/task_queue_selector.h
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/task_queue_selector_unittest.cc
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/work_queue_sets.cc
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/work_queue_sets.h
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/work_queue_sets_unittest.cc
[modify] https://crrev.com/0c32a9a06d83f1eccc4e6f65bab9850e30689f53/base/task/sequence_manager/work_queue_unittest.cc
Project Member

Comment 23 by bugdroid1@chromium.org, Jan 8 

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

commit 1b11790cd20697e4d0616d794505752f644aeb7d
Author: François Doray <fdoray@chromium.org>
Date: Tue Jan 08 20:30:59 2019

Revert "Optimise TaskQueueSelector"

This reverts commit 0c32a9a06d83f1eccc4e6f65bab9850e30689f53.

Reason for revert: Tentative fix for  https://crbug.com/919965 

Original change's description:
> Optimise TaskQueueSelector
> 
> This patch reimplements the priority selection logic by maintaining a
> priority queue of task priorities.  I.e. to select a task we first
> check the current highest priority priority with tasks and then select
> the oldest queue from within that priority.
> 
> To account for task starvation we keep a count of the number of times
> we have selected a task, and assign each priority a sort key based on
> the current task count plus a priority specific bias.  After selecting
> a queue, we re-evaluate the sort key for the associated priority.
> 
> This means if we run a lot of say high priority tasks, the sort key for
> any waiting normal priority will eventually become lower than the
> key for the high priority, resulting in us selecting a "starving"
> normal priority queue.
> 
> Despite the overhead of maintaining this new queue, it's still makes a
> 4% improvement on SequenceManagerPerfTest.PostImmediateTasks_OneQueue.
> 
> Bug: 897751
> Change-Id: I2e9add8e0266f269d98aee15a01e3d457931ea21
> Reviewed-on: https://chromium-review.googlesource.com/c/1397612
> Commit-Queue: Alex Clarke <alexclarke@chromium.org>
> Reviewed-by: Alexander Timin <altimin@chromium.org>
> Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#620778}

TBR=skyostil@chromium.org,alexclarke@chromium.org,altimin@chromium.org

Change-Id: I49d7762a01985b651185ecbe885d51edf6f8ca79
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 897751
Reviewed-on: https://chromium-review.googlesource.com/c/1401190
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#620862}
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/task_queue_selector.cc
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/task_queue_selector.h
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/task_queue_selector_unittest.cc
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/work_queue_sets.cc
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/work_queue_sets.h
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/work_queue_sets_unittest.cc
[modify] https://crrev.com/1b11790cd20697e4d0616d794505752f644aeb7d/base/task/sequence_manager/work_queue_unittest.cc
Project Member

Comment 24 by bugdroid1@chromium.org, Jan 11 

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

commit 0c17bc4823f8506061a7f20935f95f0d71d4ad5c
Author: Alex Clarke <alexclarke@chromium.org>
Date: Fri Jan 11 11:39:05 2019

Reland [Optimize TaskQueueSelector]

The previous patch was starving low priorirty tasks more by
accident which was enough to make some layout tests "leak"
blink objects.

Previous patch: http://crrev.com/c/1397612

This patch reimplements the priority selection logic by maintaining a
priority queue of task priorities.  I.e. to select a task we first
check the current highest priority priority with tasks and then select
the oldest queue from within that priority.

To account for task starvation we keep a count of the number of times
we have selected a task, and assign each priority a sort key based on
the current task count plus a priority specific bias.  After selecting
a queue, we re-evaluate the sort key for the associated priority.

This means if we run a lot of say high priority tasks, the sort key for
any waiting normal priority will eventually become lower than the
key for the high priority, resulting in us selecting a "starving"
normal priority queue.

Despite the overhead of maintaining this new queue, it's still makes a
4% improvement on SequenceManagerPerfTest.PostImmediateTasks_OneQueue.

Bug: 897751
Change-Id: Ia7737343f1a82f190802e0da1ed16989cf10e14e
Reviewed-on: https://chromium-review.googlesource.com/c/1404091
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Cr-Commit-Position: refs/heads/master@{#621976}
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue.h
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue_impl.h
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue_selector.cc
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue_selector.h
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/task_queue_selector_unittest.cc
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/work_queue_sets.cc
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/work_queue_sets.h
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/work_queue_sets_unittest.cc
[modify] https://crrev.com/0c17bc4823f8506061a7f20935f95f0d71d4ad5c/base/task/sequence_manager/work_queue_unittest.cc
Project Member

Comment 26 by 42576172...@developer.gserviceaccount.com, Jan 14 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/113f625b940000
Project Member

Comment 28 by 42576172...@developer.gserviceaccount.com, Jan 14 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/16c41f37940000
Project Member

Comment 30 by 42576172...@developer.gserviceaccount.com, Jan 15 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/11555518540000
Project Member

Comment 31 by bugdroid1@chromium.org, Jan 15 

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

commit 3ffd1ecfc08c65e453c2891d298e662553757a9c
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Jan 15 12:05:52 2019

Only reset next_delayed_do_work if we've processed it.

This works around pathalogical behavior in some tests where there's
a pending delayed task and the thread gets imemdiate tasks from
another thread. Previoulsy it was continually re-posting the delayed
task which is expensive on Mac.  Now it no longer does this.

In addition I've refactored the continuation code for clarity.

Bug: 897751
Change-Id: Ie72a5ec8344d8b95316d7e3434dd1ace0fba6fdb
Reviewed-on: https://chromium-review.googlesource.com/c/1407079
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Sami Kyöstilä <skyostil@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#622821}
[modify] https://crrev.com/3ffd1ecfc08c65e453c2891d298e662553757a9c/base/task/sequence_manager/thread_controller_with_message_pump_impl.cc
[modify] https://crrev.com/3ffd1ecfc08c65e453c2891d298e662553757a9c/base/task/sequence_manager/thread_controller_with_message_pump_impl.h
[modify] https://crrev.com/3ffd1ecfc08c65e453c2891d298e662553757a9c/base/task/sequence_manager/thread_controller_with_message_pump_impl_unittest.cc
Project Member

Comment 32 by 42576172...@developer.gserviceaccount.com, Jan 16 (6 days ago) 

📍 Pinpoint job started.
https://pinpoint-dot-chromeperf.appspot.com/job/14a8ffe0540000
Project Member

Comment 33 by 42576172...@developer.gserviceaccount.com, Jan 16 (6 days ago) 

📍 Job complete. See results below.
https://pinpoint-dot-chromeperf.appspot.com/job/14a8ffe0540000
►

Sign in to add a comment