New issue
Advanced search Search tips

Issue 839525 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: May 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug


Show other hotlists

Hotlists containing this issue:
Hotlist-1
Hotlist-14


Sign in to add a comment

[Feature] Label TaskScheduler worker thread stacks with a custom frame

Project Member Reported by gab@chromium.org, May 3 2018

Issue description

BrowserThread::IO has a specific IOThreadRun() stack frame.

The TaskScheduler should also have named frames on its threads so they can be more easily identified (currently they're all just ThreadMain() -> Thread::Run() -> ...).

For example in issue 728235, it would have been nice if the stack had explicitly labeled the reported thread as the service thread.
 
To maximize the usefulness of this, particularly when background pool is merged with foreground pool, we should have a stack frame for each possible TaskTraits.


NOINLINE void TaskTracker::RunUserBlockingPriority(Task task, Sequence* sequence, bool can_run_task) {
  RunOrSkipTask(Task task, sequence, can_run_task)
}

NOINLINE void TaskTracker::RunUserVisiblePriority(Task task, Sequence* sequence, bool can_run_task) {
  RunOrSkipTask(Task task, sequence, can_run_task)
}

NOINLINE void TaskTracker::RunBackgroundPriority(Task task, Sequence* sequence, bool can_run_task) {
  RunOrSkipTask(Task task, sequence, can_run_task)
}

NOINLINE void TaskTracker::RunMayBlock(Task task, Sequence* sequence, bool can_run_task) {
  switch (task.traits.priority()) {
    case TaskPriority::USER_BLOCKING:
      RunUserBlockingPriority(...);
      break;
    ...
  }
}

...

Comment 2 by gab@chromium.org, May 3 2018

I want it to be at the absolute lowest frame, i.e. just after ThreadMain() (so we can even identify threads which are sleeping -- e.g. to double-check thread counts make sense when bugs are dumped on us #causescheduling).

I have a CL coming up shortly.
Project Member

Comment 4 by bugdroid1@chromium.org, May 4 2018

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

commit 6084a13301141c53ed3b21cc9c45310a9b2e3799
Author: Gabriel Charette <gab@chromium.org>
Date: Fri May 04 19:05:59 2018

[TaskScheduler] Label service thread's stack with an identifying frame.

The approach taken here is identical to IOThreadRun() in
browser_process_sub_thread.cc

R=fdoray@chromium.org

Bug:  839525 
Test: Locally inspect stack in debugger in a Release build.
Change-Id: I409623f98dacef908cc6c1685fa2508041c78042
Reviewed-on: https://chromium-review.googlesource.com/1044496
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556139}
[modify] https://crrev.com/6084a13301141c53ed3b21cc9c45310a9b2e3799/base/task_scheduler/task_scheduler_impl.cc
[modify] https://crrev.com/6084a13301141c53ed3b21cc9c45310a9b2e3799/base/task_scheduler/task_scheduler_impl.h

Project Member

Comment 5 by bugdroid1@chromium.org, May 4 2018

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

commit 5b0cb536a073784425e1ba4e1fa594aefb58bebe
Author: Gabriel Charette <gab@chromium.org>
Date: Fri May 04 20:16:16 2018

[TaskScheduler] Label SchedulerWorker threads stacks with an identifying frame

I intentionally didn't add "Foreground" to non "Background" workers as
they may be used for background tasks in some configurations and I don't
want it to be confusing for developers.

R=fdoray@chromium.org

Bug:  839525 
Change-Id: I8c1e928914aa6d2e35fedc1db4e8a639d39116b2
Reviewed-on: https://chromium-review.googlesource.com/1044501
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556169}
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_single_thread_task_runner_manager.h
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_worker.cc
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_worker.h
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/5b0cb536a073784425e1ba4e1fa594aefb58bebe/base/task_scheduler/task_scheduler_impl_unittest.cc

Comment 6 by gab@chromium.org, May 4 2018

Status: Fixed (was: Started)
Woohoo! User-friendly stacks everywhere :)
Project Member

Comment 7 by bugdroid1@chromium.org, May 5 2018

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

commit 4b3ad4224ed672d11138d24e5d7313d2496b9a1d
Author: Gabriel Charette <gab@chromium.org>
Date: Sat May 05 05:26:38 2018

Revert "[TaskScheduler] Label SchedulerWorker threads stacks with an identifying frame"

This reverts commit 5b0cb536a073784425e1ba4e1fa594aefb58bebe.

Reason for revert: find it identified flakes, I'll investigate on Monday

Original change's description:
> [TaskScheduler] Label SchedulerWorker threads stacks with an identifying frame
> 
> I intentionally didn't add "Foreground" to non "Background" workers as
> they may be used for background tasks in some configurations and I don't
> want it to be confusing for developers.
> 
> R=​fdoray@chromium.org
> 
> Bug:  839525 
> Change-Id: I8c1e928914aa6d2e35fedc1db4e8a639d39116b2
> Reviewed-on: https://chromium-review.googlesource.com/1044501
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556169}

TBR=gab@chromium.org,fdoray@chromium.org

Change-Id: I660f43ba88eade2ac3e8fedf4d60495eab708054
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  839525 
Reviewed-on: https://chromium-review.googlesource.com/1045747
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556306}
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_single_thread_task_runner_manager.h
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_worker.cc
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_worker.h
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/4b3ad4224ed672d11138d24e5d7313d2496b9a1d/base/task_scheduler/task_scheduler_impl_unittest.cc

Project Member

Comment 8 by bugdroid1@chromium.org, May 7 2018

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

commit be2419c13b0a7a1c43d9aeef8804266c923c0191
Author: Gabriel Charette <gab@chromium.org>
Date: Mon May 07 18:32:11 2018

Reland "[TaskScheduler] Label SchedulerWorker threads stacks with an identifying frame"

This is a reland of 5b0cb536a073784425e1ba4e1fa594aefb58bebe

It was reverted @ r556169 because the tests were flaky on POSIX.
I believe the instability is in base::debug::StackTrace().Print().

This reland excludes the tests on POSIX. The logic is cross-platform
and the tests on Windows prove it works. I will investigate POSIX
separately in crbug.com/840429


Original change's description:
> [TaskScheduler] Label SchedulerWorker threads stacks with an identifying frame
>
> I intentionally didn't add "Foreground" to non "Background" workers as
> they may be used for background tasks in some configurations and I don't
> want it to be confusing for developers.
>
> R=fdoray@chromium.org
>
> Bug:  839525 
> Change-Id: I8c1e928914aa6d2e35fedc1db4e8a639d39116b2
> Reviewed-on: https://chromium-review.googlesource.com/1044501
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Reviewed-by: François Doray <fdoray@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#556169}

TBR=fdoray@chromium.org

Bug:  839525 ,  840050 , 840429
Change-Id: Id1ee52a52483717990336920487ea1eee790358b
Reviewed-on: https://chromium-review.googlesource.com/1047846
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Cr-Commit-Position: refs/heads/master@{#556506}
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_single_thread_task_runner_manager.h
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_worker.cc
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_worker.h
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_worker_pool_impl.cc
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_worker_stack_unittest.cc
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/scheduler_worker_unittest.cc
[modify] https://crrev.com/be2419c13b0a7a1c43d9aeef8804266c923c0191/base/task_scheduler/task_scheduler_impl_unittest.cc

Sign in to add a comment