Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
440893002:
RundownTaskCounter needs to use a non-nestable task to block on the completion of already scheduled… (Closed)
|
Created:
6 years, 4 months ago by gab Modified:
6 years, 4 months ago CC:
chromium-reviews Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
DescriptionRundownTaskCounter needs to use a non-nestable task to block on the completion of already scheduled
tasks on each SequencedTaskRunner.
By using a nestable task the RundownTaskCounter was guaranteed that the task would START after current
tasks had also STARTED but not necessarily COMPLETED..!
BUG=386126
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288128
Patch Set 1 #
Messages
Total messages: 15 (0 generated)
Siggi PTAL!!
@Scott for OWNERS Thanks, Gab
Sorry, I'm not familiar enough with the ramifications of this change. Could you find a better reviewer?
Looks like you and brettw were TBR'ed on the original impl during no meetings week: https://codereview.chromium.org/349263004
lgtm thanks. Scott/Brett, I suspect there's no practical difference to this, as I don't expect anything's spawning nested message loops on the blocking pool. For my edification - is is sane usage of the blocking pool to spin nested message loops, and if not, is this guarded somehow?
On 2014/08/06 09:51:18, Sigurður Ásgeirsson wrote: > lgtm thanks. > > Scott/Brett, I suspect there's no practical difference to this, as I don't > expect anything's spawning nested message loops on the blocking pool. I don't think this has to do with nested message loops, but rather with the SequencedTaskRunner dispatching tasks to multiple threads unless tasks are non-nestable (i.e. "nesting" here means "running in parallel" FWIU). > > For my edification - is is sane usage of the blocking pool to spin nested > message loops, and if not, is this guarded somehow?
On Wed, Aug 6, 2014 at 3:10 PM, <gab@chromium.org> wrote: > On 2014/08/06 09:51:18, Sigurður Ásgeirsson wrote: > >> lgtm thanks. >> > > Scott/Brett, I suspect there's no practical difference to this, as I don't >> expect anything's spawning nested message loops on the blocking pool. >> > > I don't think this has to do with nested message loops, but rather with the > SequencedTaskRunner dispatching tasks to multiple threads unless tasks are > non-nestable (i.e. "nesting" here means "running in parallel" FWIU). > > Interesting, I suppose that can be read into the interface, though "started" is a difficult time point to define for a task. It appears that my interpretation is implied by such comments as // - If T2 will start after T1 starts by the above guarantee, then // T2 will start after T1 finishes and is destroyed if: // // * T2 is non-nestable, or // * *T1 doesn't call any task-running methods.* In any case, this is moar, better correct as you've rewritten it. > > > For my edification - is is sane usage of the blocking pool to spin nested >> message loops, and if not, is this guarded somehow? >> > > > > https://codereview.chromium.org/440893002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/08/06 15:19:12, Sigurður Ásgeirsson wrote: > On Wed, Aug 6, 2014 at 3:10 PM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> wrote: > > > On 2014/08/06 09:51:18, Sigurður Ásgeirsson wrote: > > > >> lgtm thanks. > >> > > > > Scott/Brett, I suspect there's no practical difference to this, as I don't > >> expect anything's spawning nested message loops on the blocking pool. > >> > > > > I don't think this has to do with nested message loops, but rather with the > > SequencedTaskRunner dispatching tasks to multiple threads unless tasks are > > non-nestable (i.e. "nesting" here means "running in parallel" FWIU). > > > > > Interesting, I suppose that can be read into the interface, though > "started" is a difficult time point to define for a task. It appears that > my interpretation is implied by such comments as > // - If T2 will start after T1 starts by the above guarantee, then > // T2 will start after T1 finishes and is destroyed if: > // > // * T2 is non-nestable, or > // * *T1 doesn't call any task-running methods.* Aaah I see, I'd missed that subtlety... so probably won't help in the wild :-(. Either way, as you said, being explicit about it is moar correct I guess.. > > In any case, this is moar, better correct as you've rewritten it. > > > > > > > For my edification - is is sane usage of the blocking pool to spin nested > >> message loops, and if not, is this guarded somehow? > >> > > > > > > > > https://codereview.chromium.org/440893002/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium-reviews+unsubscri....
@brettw or sky, PTAL.
On Thu, Aug 7, 2014 at 1:06 PM, <gab@chromium.org> wrote: > On 2014/08/06 15:19:12, Sigurður Ásgeirsson wrote: > >> On Wed, Aug 6, 2014 at 3:10 PM, >> > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=gab@chromium.org> > wrote: > > > On 2014/08/06 09:51:18, Sigurður Ásgeirsson wrote: >> > >> >> lgtm thanks. >> >> >> > >> > Scott/Brett, I suspect there's no practical difference to this, as I >> don't >> >> expect anything's spawning nested message loops on the blocking pool. >> >> >> > >> > I don't think this has to do with nested message loops, but rather with >> the >> > SequencedTaskRunner dispatching tasks to multiple threads unless tasks >> are >> > non-nestable (i.e. "nesting" here means "running in parallel" FWIU). >> > >> > >> Interesting, I suppose that can be read into the interface, though >> "started" is a difficult time point to define for a task. It appears that >> my interpretation is implied by such comments as >> // - If T2 will start after T1 starts by the above guarantee, then >> // T2 will start after T1 finishes and is destroyed if: >> // >> // * T2 is non-nestable, or >> // * *T1 doesn't call any task-running methods.* >> > > > Aaah I see, I'd missed that subtlety... so probably won't help in the wild > :-(. > > Yeah, I find this bit of documentation pretty hard to parse, and I'm still not sure what's the truth of the matter. It's possible that things on the blocking pool are spinning nested message loops, I suppose... > Either way, as you said, being explicit about it is moar correct I guess.. > > > > > In any case, this is moar, better correct as you've rewritten it. >> > > > >> > >> > For my edification - is is sane usage of the blocking pool to spin >> nested >> >> message loops, and if not, is this guarded somehow? >> >> >> > >> > >> > >> > https://codereview.chromium.org/440893002/ >> > >> > > To unsubscribe from this group and stop receiving emails from it, send an >> > email > >> to >> > https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=chromium- > reviews+unsubscribe@chromium.org. > > > > https://codereview.chromium.org/440893002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
LGTM
The CQ bit was checked by gab@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/440893002/1
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: win8_chromium_rel on tryserver.chromium.win (http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
Message was sent while issue was closed.
Change committed as 288128 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
