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

Issue 857101 link

Starred by 2 users
Status: Available
Owner: ----
Cc:
eseckler@chromium.org
skyos...@chromium.org
altimin@chromium.org
█ alexclarke@chromium.org
█ kraynov@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Task



Sign in to add a comment

Refactor TimeDomain

Project Member Reported by kraynov@chromium.org, Jun 27 2018 
Key issues with TimeDomain:
1. Encompass too different responsibilities.
2. No clear distinction between TimeDomain's and SequenceManager's time which might lead to bugs/confusion.
3. Registration pattern should go away.

Comment 1 by kraynov@chromium.org, Jun 27 2018

Summary: Make TimeDomain clear (was: Mkae TimeDomain clear)

Comment 2 by altimin@chromium.org, Jun 27 2018

Components: Blink>Scheduling
Summary: Refactor TimeDomain (was: Make TimeDomain clear)

Comment 3 by altimin@chromium.org, Jun 27 2018 

I think that we should remove multiple TimeDomain classes and just have one TimeDomain per SequenceManager which will aggregate wake-ups and advance time/wake-up queues as needed.
Project Member

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

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

commit a5c89a76fabd623811c804e63df48d7bce18caef
Author: Greg Kraynov <kraynov@chromium.org>
Date: Thu Jun 28 15:27:11 2018

Make TimeDomain::NextScheduledRunTime protected.

It shouldn't be used as an API method, use DelayTillNextTask
method instead. The are intercahngable for RealTimeDomain but
NextScheduledRunTime disreards any additional logic which
might be defined in a DelayTillNextTask override.
So this change also makes it more semantically correct.

Bug: 857101
Change-Id: Ie6170312d7840a2acfe32e592fb2d799e4eaf0a7
Reviewed-on: https://chromium-review.googlesource.com/1118224
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571133}
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl_unittest.cc
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/scheduler/base/time_domain.h
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/scheduler/common/idle_helper.cc
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/scheduler/common/idle_helper_unittest.cc
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/testing/testing_platform_support_with_mock_scheduler.cc
[modify] https://crrev.com/a5c89a76fabd623811c804e63df48d7bce18caef/third_party/blink/renderer/platform/timer_test.cc
Project Member

Comment 5 by bugdroid1@chromium.org, Jun 28 2018 

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

commit a22e020d32453b260c5bb86eae9fcb530bc639db
Author: Greg Kraynov <kraynov@chromium.org>
Date: Thu Jun 28 15:52:37 2018

Remove TaskQueueManagerImpl::CreateLazyNow method.

Also avoid a hop to get own clock via the RealTimeDomain.

Bug: 857101
Change-Id: I0433f8442b061ff2b2f33f839eb08719fb76bc5d
Reviewed-on: https://chromium-review.googlesource.com/1118267
Reviewed-by: Alexander Timin <altimin@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571142}
[modify] https://crrev.com/a22e020d32453b260c5bb86eae9fcb530bc639db/third_party/blink/renderer/platform/scheduler/base/real_time_domain.cc
[modify] https://crrev.com/a22e020d32453b260c5bb86eae9fcb530bc639db/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.cc
[modify] https://crrev.com/a22e020d32453b260c5bb86eae9fcb530bc639db/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.h
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 28 2018 

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

commit f8240bdab80613fecf5f9d76730b4cdc938086bc
Author: Greg Kraynov <kraynov@chromium.org>
Date: Thu Jun 28 17:42:36 2018

Simplify TimeDomain.

The primary goal was to make ThrottledTimeDomain directly
inherit TimeDomain since RealTimeDomain gets moved to //base
and will be made internal. Since we can't use TaskQueueManagerImpl
outside of scheduler/base it was required to make the TimeDomain
deal with it, that caused API changes, and finally it makes sense
to update doc comments when API changes. It could be nice to break
this down into a smaller CLs, but most changes here are pretty
dependant on each other.

Bug: 857101
Change-Id: I5cd3352f714bf1b0f6e89255c5bc31cc540336c4
Reviewed-on: https://chromium-review.googlesource.com/1107977
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571191}
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/real_time_domain.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/real_time_domain.h
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/task_queue_impl.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_perftest.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/time_domain.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/time_domain.h
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/time_domain_unittest.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/virtual_time_domain.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/base/virtual_time_domain.h
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/common/throttling/throttled_time_domain.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/common/throttling/throttled_time_domain.h
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.cc
[modify] https://crrev.com/f8240bdab80613fecf5f9d76730b4cdc938086bc/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.h
Project Member

Comment 7 by bugdroid1@chromium.org, Jun 29 2018 

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

commit e28bfbcb9404393fbd872937a57f74a07f1fc446
Author: Greg Kraynov <kraynov@chromium.org>
Date: Fri Jun 29 14:53:37 2018

Remove redundant TaskQueueManagerImpl::CancelDelayedWork.

The way how ThreadController work isn't compatible with the
semantic of unscheduling wake-ups at a *specific* time, simply
because there's only one delayed wake-up can be set at any time.

ThreadControllerImpl::DoWork post a delayed continuation
if necessary, making TimeDomain's CancelWakeUpAt pointless,
so TimeDomain now simplified as well.

Bug: 857101
Change-Id: Ib542ee3eb13a481175aeed4d9072e0296a1d06e4
Reviewed-on: https://chromium-review.googlesource.com/1120245
Reviewed-by: Alex Clarke <alexclarke@chromium.org>
Commit-Queue: Greg Kraynov <kraynov@chromium.org>
Cr-Commit-Position: refs/heads/master@{#571473}
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/base/task/sequence_manager/time_domain.h
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_impl.h
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/task_queue_manager_perftest.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/time_domain.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/time_domain_unittest.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/virtual_time_domain.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/base/virtual_time_domain.h
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/common/throttling/throttled_time_domain.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/common/throttling/throttled_time_domain.h
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.cc
[modify] https://crrev.com/e28bfbcb9404393fbd872937a57f74a07f1fc446/third_party/blink/renderer/platform/scheduler/main_thread/auto_advancing_virtual_time_domain.h

Comment 8 by kraynov@chromium.org, Jul 18

Cc: eseckler@chromium.org

Comment 9 by kraynov@chromium.org, Oct 3

Owner: ----
Status: Available (was: Assigned)
►

Sign in to add a comment