Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2807063007:
Remove SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback. (Closed)
DescriptionRemove SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback.
This was useful when we thought that sequences could contain
tasks with different traits and be enqueued in different priority
queues during their lifetime. This is no longer a supported use case.
BUG=553459
Patch Set 1 #
Depends on Patchset: Messages
Total messages: 17 (6 generated)
The CQ bit was checked by fdoray@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
fdoray@chromium.org changed reviewers: + gab@chromium.org, robliao@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
On 2017/04/11 17:54:22, fdoray wrote: > PTAL Hrm... it might be safer to work out the branched sequence work (funnels) and then do this. Thoughts?
On 2017/04/12 01:10:26, robliao wrote: > On 2017/04/11 17:54:22, fdoray wrote: > > PTAL > > Hrm... it might be safer to work out the branched sequence work (funnels) and > then do this. Thoughts? In my opinion, we should never allow sequences (including sequences of sequences) to move between pools. A sequence that contains background *and* foreground sequences should live in a foreground pool. Background sequences are last in a foreground PQ, so they have a high latency. However, they're scheduled on foreground threads so their execution time is normal. That means that the latency of foreground sequences is never affected by background tasks taking a long time to run on a background thread. Also, I secretly hope to get rid of blocking pools and instead rely on ScopedMayBlock. Since we don't have a clear plan for sequence funneling yet, we can delay the review of this CL.
On 2017/04/12 12:28:24, fdoray wrote: > On 2017/04/12 01:10:26, robliao wrote: > > On 2017/04/11 17:54:22, fdoray wrote: > > > PTAL > > > > Hrm... it might be safer to work out the branched sequence work (funnels) and > > then do this. Thoughts? > > In my opinion, we should never allow sequences (including sequences of > sequences) to move between pools. > > A sequence that contains background *and* foreground sequences should live in a > foreground pool. Background sequences are last in a foreground PQ, so they have > a high latency. However, they're scheduled on foreground threads so their > execution time is normal. That means that the latency of foreground sequences is > never affected by background tasks taking a long time to run on a background > thread. > > Also, I secretly hope to get rid of blocking pools and instead rely on > ScopedMayBlock. > > Since we don't have a clear plan for sequence funneling yet, we can delay the > review of this CL. Agreed that sequences moving between pools is unlikely to ever be a good idea (maybe blocking wise, never priority wise?). Let's not keep this code around just in case we need it IMO (we can revert later if need be). We're more likely to get rid of separate pools than we are to bounce sequences between pools? LGTM
On 2017/04/12 18:55:34, gab wrote: > On 2017/04/12 12:28:24, fdoray wrote: > > On 2017/04/12 01:10:26, robliao wrote: > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > PTAL > > > > > > Hrm... it might be safer to work out the branched sequence work (funnels) > and > > > then do this. Thoughts? > > > > In my opinion, we should never allow sequences (including sequences of > > sequences) to move between pools. > > > > A sequence that contains background *and* foreground sequences should live in > a > > foreground pool. Background sequences are last in a foreground PQ, so they > have > > a high latency. However, they're scheduled on foreground threads so their > > execution time is normal. That means that the latency of foreground sequences > is > > never affected by background tasks taking a long time to run on a background > > thread. > > > > Also, I secretly hope to get rid of blocking pools and instead rely on > > ScopedMayBlock. > > > > Since we don't have a clear plan for sequence funneling yet, we can delay the > > review of this CL. > > Agreed that sequences moving between pools is unlikely to ever be a good idea > (maybe blocking wise, never priority wise?). Let's not keep this code around > just in case we need it IMO (we can revert later if need be). We're more likely > to get rid of separate pools than we are to bounce sequences between pools? > > LGTM Reverting it later is costlier than just keeping this CL here and committing it later. It basically amounts to a hand re-merge of the code. Given that this plumbing isn't costing us too much and there's no urgent need, I'd rather do it then.
On 2017/04/12 19:01:21, robliao wrote: > On 2017/04/12 18:55:34, gab wrote: > > On 2017/04/12 12:28:24, fdoray wrote: > > > On 2017/04/12 01:10:26, robliao wrote: > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > PTAL > > > > > > > > Hrm... it might be safer to work out the branched sequence work (funnels) > > and > > > > then do this. Thoughts? > > > > > > In my opinion, we should never allow sequences (including sequences of > > > sequences) to move between pools. > > > > > > A sequence that contains background *and* foreground sequences should live > in > > a > > > foreground pool. Background sequences are last in a foreground PQ, so they > > have > > > a high latency. However, they're scheduled on foreground threads so their > > > execution time is normal. That means that the latency of foreground > sequences > > is > > > never affected by background tasks taking a long time to run on a background > > > thread. > > > > > > Also, I secretly hope to get rid of blocking pools and instead rely on > > > ScopedMayBlock. > > > > > > Since we don't have a clear plan for sequence funneling yet, we can delay > the > > > review of this CL. > > > > Agreed that sequences moving between pools is unlikely to ever be a good idea > > (maybe blocking wise, never priority wise?). Let's not keep this code around > > just in case we need it IMO (we can revert later if need be). We're more > likely > > to get rid of separate pools than we are to bounce sequences between pools? > > > > LGTM > > Reverting it later is costlier than just keeping this CL here and committing it > later. It basically amounts to a hand re-merge of the code. > Given that this plumbing isn't costing us too much and there's no urgent need, > I'd rather do it then. I disagree, dead code is always eventually detrimental. i.e. if the revert eventually conflicts it's likely because this dead code was unnecessarily moved around in a prior refactoring... When do you think we'd need to bounce pools?
On 2017/04/12 19:21:48, gab wrote: > On 2017/04/12 19:01:21, robliao wrote: > > On 2017/04/12 18:55:34, gab wrote: > > > On 2017/04/12 12:28:24, fdoray wrote: > > > > On 2017/04/12 01:10:26, robliao wrote: > > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > > PTAL > > > > > > > > > > Hrm... it might be safer to work out the branched sequence work > (funnels) > > > and > > > > > then do this. Thoughts? > > > > > > > > In my opinion, we should never allow sequences (including sequences of > > > > sequences) to move between pools. > > > > > > > > A sequence that contains background *and* foreground sequences should live > > in > > > a > > > > foreground pool. Background sequences are last in a foreground PQ, so they > > > have > > > > a high latency. However, they're scheduled on foreground threads so their > > > > execution time is normal. That means that the latency of foreground > > sequences > > > is > > > > never affected by background tasks taking a long time to run on a > background > > > > thread. > > > > > > > > Also, I secretly hope to get rid of blocking pools and instead rely on > > > > ScopedMayBlock. > > > > > > > > Since we don't have a clear plan for sequence funneling yet, we can delay > > the > > > > review of this CL. > > > > > > Agreed that sequences moving between pools is unlikely to ever be a good > idea > > > (maybe blocking wise, never priority wise?). Let's not keep this code around > > > just in case we need it IMO (we can revert later if need be). We're more > > likely > > > to get rid of separate pools than we are to bounce sequences between pools? > > > > > > LGTM > > > > Reverting it later is costlier than just keeping this CL here and committing > it > > later. It basically amounts to a hand re-merge of the code. > > Given that this plumbing isn't costing us too much and there's no urgent need, > > I'd rather do it then. > > I disagree, dead code is always eventually detrimental. i.e. if the revert > eventually conflicts it's likely because this dead code was unnecessarily moved > around in a prior refactoring... > > When do you think we'd need to bounce pools? With sequence branching, it's reasonable for a component to do both background and foreground work that feeds to the same sequence. If that sequence is tied to a SchedulerWorkerPoolImpl, it is no longer possible to reschedule that sequence for other priority work should we decide to do it. How we decide to handle sequence branching will determine if we need to keep this.
On 2017/04/12 19:27:49, robliao wrote: > On 2017/04/12 19:21:48, gab wrote: > > On 2017/04/12 19:01:21, robliao wrote: > > > On 2017/04/12 18:55:34, gab wrote: > > > > On 2017/04/12 12:28:24, fdoray wrote: > > > > > On 2017/04/12 01:10:26, robliao wrote: > > > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > > > PTAL > > > > > > > > > > > > Hrm... it might be safer to work out the branched sequence work > > (funnels) > > > > and > > > > > > then do this. Thoughts? > > > > > > > > > > In my opinion, we should never allow sequences (including sequences of > > > > > sequences) to move between pools. > > > > > > > > > > A sequence that contains background *and* foreground sequences should > live > > > in > > > > a > > > > > foreground pool. Background sequences are last in a foreground PQ, so > they > > > > have > > > > > a high latency. However, they're scheduled on foreground threads so > their > > > > > execution time is normal. That means that the latency of foreground > > > sequences > > > > is > > > > > never affected by background tasks taking a long time to run on a > > background > > > > > thread. > > > > > > > > > > Also, I secretly hope to get rid of blocking pools and instead rely on > > > > > ScopedMayBlock. > > > > > > > > > > Since we don't have a clear plan for sequence funneling yet, we can > delay > > > the > > > > > review of this CL. > > > > > > > > Agreed that sequences moving between pools is unlikely to ever be a good > > idea > > > > (maybe blocking wise, never priority wise?). Let's not keep this code > around > > > > just in case we need it IMO (we can revert later if need be). We're more > > > likely > > > > to get rid of separate pools than we are to bounce sequences between > pools? > > > > > > > > LGTM > > > > > > Reverting it later is costlier than just keeping this CL here and committing > > it > > > later. It basically amounts to a hand re-merge of the code. > > > Given that this plumbing isn't costing us too much and there's no urgent > need, > > > I'd rather do it then. > > > > I disagree, dead code is always eventually detrimental. i.e. if the revert > > eventually conflicts it's likely because this dead code was unnecessarily > moved > > around in a prior refactoring... > > > > When do you think we'd need to bounce pools? > > With sequence branching, it's reasonable for a component to do both background > and foreground work that feeds to the same sequence. If that sequence is tied to > a SchedulerWorkerPoolImpl, it is no longer possible to reschedule that sequence > for other priority work should we decide to do it. > > How we decide to handle sequence branching will determine if we need to keep > this. Background and foreground work will definitely be supported. But the background tasks shouldn't run in a pool with background thread priority. Otherwise, a single background task could delay the execution of foreground tasks by a lot.
On 2017/04/12 19:41:29, fdoray wrote: > On 2017/04/12 19:27:49, robliao wrote: > > On 2017/04/12 19:21:48, gab wrote: > > > On 2017/04/12 19:01:21, robliao wrote: > > > > On 2017/04/12 18:55:34, gab wrote: > > > > > On 2017/04/12 12:28:24, fdoray wrote: > > > > > > On 2017/04/12 01:10:26, robliao wrote: > > > > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > > > > PTAL > > > > > > > > > > > > > > Hrm... it might be safer to work out the branched sequence work > > > (funnels) > > > > > and > > > > > > > then do this. Thoughts? > > > > > > > > > > > > In my opinion, we should never allow sequences (including sequences of > > > > > > sequences) to move between pools. > > > > > > > > > > > > A sequence that contains background *and* foreground sequences should > > live > > > > in > > > > > a > > > > > > foreground pool. Background sequences are last in a foreground PQ, so > > they > > > > > have > > > > > > a high latency. However, they're scheduled on foreground threads so > > their > > > > > > execution time is normal. That means that the latency of foreground > > > > sequences > > > > > is > > > > > > never affected by background tasks taking a long time to run on a > > > background > > > > > > thread. > > > > > > > > > > > > Also, I secretly hope to get rid of blocking pools and instead rely on > > > > > > ScopedMayBlock. > > > > > > > > > > > > Since we don't have a clear plan for sequence funneling yet, we can > > delay > > > > the > > > > > > review of this CL. > > > > > > > > > > Agreed that sequences moving between pools is unlikely to ever be a good > > > idea > > > > > (maybe blocking wise, never priority wise?). Let's not keep this code > > around > > > > > just in case we need it IMO (we can revert later if need be). We're more > > > > likely > > > > > to get rid of separate pools than we are to bounce sequences between > > pools? > > > > > > > > > > LGTM > > > > > > > > Reverting it later is costlier than just keeping this CL here and > committing > > > it > > > > later. It basically amounts to a hand re-merge of the code. > > > > Given that this plumbing isn't costing us too much and there's no urgent > > need, > > > > I'd rather do it then. > > > > > > I disagree, dead code is always eventually detrimental. i.e. if the revert > > > eventually conflicts it's likely because this dead code was unnecessarily > > moved > > > around in a prior refactoring... > > > > > > When do you think we'd need to bounce pools? > > > > With sequence branching, it's reasonable for a component to do both background > > and foreground work that feeds to the same sequence. If that sequence is tied > to > > a SchedulerWorkerPoolImpl, it is no longer possible to reschedule that > sequence > > for other priority work should we decide to do it. > > > > How we decide to handle sequence branching will determine if we need to keep > > this. > > Background and foreground work will definitely be supported. But the background > tasks shouldn't run in a pool with background thread priority. Otherwise, a > single background task could delay the execution of foreground tasks by a lot. It would also be unexpected for a component that generated a lot of background work to run at user blocking priority :-). Hence, there's a lot of nuance with this.
Sequence funneling IMO would be driven towards a single thread or single pool. Anyways we don't need to figure that out now, the point is merely not to keep dead code around because it might be useful later, that's never the right choice IMO. Le mer. 12 avr. 2017 15:43, <robliao@chromium.org> a écrit : > On 2017/04/12 19:41:29, fdoray wrote: > > On 2017/04/12 19:27:49, robliao wrote: > > > On 2017/04/12 19:21:48, gab wrote: > > > > On 2017/04/12 19:01:21, robliao wrote: > > > > > On 2017/04/12 18:55:34, gab wrote: > > > > > > On 2017/04/12 12:28:24, fdoray wrote: > > > > > > > On 2017/04/12 01:10:26, robliao wrote: > > > > > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > > > > > PTAL > > > > > > > > > > > > > > > > Hrm... it might be safer to work out the branched sequence > work > > > > (funnels) > > > > > > and > > > > > > > > then do this. Thoughts? > > > > > > > > > > > > > > In my opinion, we should never allow sequences (including > sequences > of > > > > > > > sequences) to move between pools. > > > > > > > > > > > > > > A sequence that contains background *and* foreground sequences > should > > > live > > > > > in > > > > > > a > > > > > > > foreground pool. Background sequences are last in a foreground > PQ, > so > > > they > > > > > > have > > > > > > > a high latency. However, they're scheduled on foreground > threads so > > > their > > > > > > > execution time is normal. That means that the latency of > foreground > > > > > sequences > > > > > > is > > > > > > > never affected by background tasks taking a long time to run > on a > > > > background > > > > > > > thread. > > > > > > > > > > > > > > Also, I secretly hope to get rid of blocking pools and instead > rely > on > > > > > > > ScopedMayBlock. > > > > > > > > > > > > > > Since we don't have a clear plan for sequence funneling yet, > we can > > > delay > > > > > the > > > > > > > review of this CL. > > > > > > > > > > > > Agreed that sequences moving between pools is unlikely to ever > be a > good > > > > idea > > > > > > (maybe blocking wise, never priority wise?). Let's not keep this > code > > > around > > > > > > just in case we need it IMO (we can revert later if need be). > We're > more > > > > > likely > > > > > > to get rid of separate pools than we are to bounce sequences > between > > > pools? > > > > > > > > > > > > LGTM > > > > > > > > > > Reverting it later is costlier than just keeping this CL here and > > committing > > > > it > > > > > later. It basically amounts to a hand re-merge of the code. > > > > > Given that this plumbing isn't costing us too much and there's no > urgent > > > need, > > > > > I'd rather do it then. > > > > > > > > I disagree, dead code is always eventually detrimental. i.e. if the > revert > > > > eventually conflicts it's likely because this dead code was > unnecessarily > > > moved > > > > around in a prior refactoring... > > > > > > > > When do you think we'd need to bounce pools? > > > > > > With sequence branching, it's reasonable for a component to do both > background > > > and foreground work that feeds to the same sequence. If that sequence > is > tied > > to > > > a SchedulerWorkerPoolImpl, it is no longer possible to reschedule that > > sequence > > > for other priority work should we decide to do it. > > > > > > How we decide to handle sequence branching will determine if we need > to keep > > > this. > > > > Background and foreground work will definitely be supported. But the > background > > tasks shouldn't run in a pool with background thread priority. > Otherwise, a > > single background task could delay the execution of foreground tasks by > a lot. > > It would also be unexpected for a component that generated a lot of > background > work to run at user blocking priority :-). > Hence, there's a lot of nuance with this. > > https://codereview.chromium.org/2807063007/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/04/13 14:07:55, chromium-reviews wrote: > Sequence funneling IMO would be driven towards a single thread or single > pool. Anyways we don't need to figure that out now, the point is merely not > to keep dead code around because it might be useful later, that's never the > right choice IMO. > > Le mer. 12 avr. 2017 15:43, <mailto:robliao@chromium.org> a écrit : > > > On 2017/04/12 19:41:29, fdoray wrote: > > > On 2017/04/12 19:27:49, robliao wrote: > > > > On 2017/04/12 19:21:48, gab wrote: > > > > > On 2017/04/12 19:01:21, robliao wrote: > > > > > > On 2017/04/12 18:55:34, gab wrote: > > > > > > > On 2017/04/12 12:28:24, fdoray wrote: > > > > > > > > On 2017/04/12 01:10:26, robliao wrote: > > > > > > > > > On 2017/04/11 17:54:22, fdoray wrote: > > > > > > > > > > PTAL > > > > > > > > > > > > > > > > > > Hrm... it might be safer to work out the branched sequence > > work > > > > > (funnels) > > > > > > > and > > > > > > > > > then do this. Thoughts? > > > > > > > > > > > > > > > > In my opinion, we should never allow sequences (including > > sequences > > of > > > > > > > > sequences) to move between pools. > > > > > > > > > > > > > > > > A sequence that contains background *and* foreground sequences > > should > > > > live > > > > > > in > > > > > > > a > > > > > > > > foreground pool. Background sequences are last in a foreground > > PQ, > > so > > > > they > > > > > > > have > > > > > > > > a high latency. However, they're scheduled on foreground > > threads so > > > > their > > > > > > > > execution time is normal. That means that the latency of > > foreground > > > > > > sequences > > > > > > > is > > > > > > > > never affected by background tasks taking a long time to run > > on a > > > > > background > > > > > > > > thread. > > > > > > > > > > > > > > > > Also, I secretly hope to get rid of blocking pools and instead > > rely > > on > > > > > > > > ScopedMayBlock. > > > > > > > > > > > > > > > > Since we don't have a clear plan for sequence funneling yet, > > we can > > > > delay > > > > > > the > > > > > > > > review of this CL. > > > > > > > > > > > > > > Agreed that sequences moving between pools is unlikely to ever > > be a > > good > > > > > idea > > > > > > > (maybe blocking wise, never priority wise?). Let's not keep this > > code > > > > around > > > > > > > just in case we need it IMO (we can revert later if need be). > > We're > > more > > > > > > likely > > > > > > > to get rid of separate pools than we are to bounce sequences > > between > > > > pools? > > > > > > > > > > > > > > LGTM > > > > > > > > > > > > Reverting it later is costlier than just keeping this CL here and > > > committing > > > > > it > > > > > > later. It basically amounts to a hand re-merge of the code. > > > > > > Given that this plumbing isn't costing us too much and there's no > > urgent > > > > need, > > > > > > I'd rather do it then. > > > > > > > > > > I disagree, dead code is always eventually detrimental. i.e. if the > > revert > > > > > eventually conflicts it's likely because this dead code was > > unnecessarily > > > > moved > > > > > around in a prior refactoring... > > > > > > > > > > When do you think we'd need to bounce pools? > > > > > > > > With sequence branching, it's reasonable for a component to do both > > background > > > > and foreground work that feeds to the same sequence. If that sequence > > is > > tied > > > to > > > > a SchedulerWorkerPoolImpl, it is no longer possible to reschedule that > > > sequence > > > > for other priority work should we decide to do it. > > > > > > > > How we decide to handle sequence branching will determine if we need > > to keep > > > > this. > > > > > > Background and foreground work will definitely be supported. But the > > background > > > tasks shouldn't run in a pool with background thread priority. > > Otherwise, a > > > single background task could delay the execution of foreground tasks by > > a lot. > > > > It would also be unexpected for a component that generated a lot of > > background > > work to run at user blocking priority :-). > > Hence, there's a lot of nuance with this. > > > > https://codereview.chromium.org/2807063007/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. In principle I agree, but given that we're going to be figuring this out reasonably soon, the code is actively called today, we've lived with this for many months, one more month isn't going to adversely impact the code.
Description was changed from ========== Remove SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback. This was useful when we thought that sequences could contain tasks with different traits and be enqueued in different priority queues during their lifetime. This is no longer a supported use case. BUG=553459 ========== to ========== Remove SchedulerWorkerPoolImpl::ReEnqueueSequenceCallback. This was useful when we thought that sequences could contain tasks with different traits and be enqueued in different priority queues during their lifetime. This is no longer a supported use case. BUG=553459 ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
