Question on base::CurrentThread with base::ThreadPool

[Alex Chau]

Hi scheduler-dev,

I'm developing code that uses a ThreadPool TaskRunner, and need to access some API that depends base::CurrentThread. I find that base::CurrentThread::IsSet() always return false when called in ThreadPool TaskRunner (base::ThreadPool::PostTask or base::ThreadPool::CreateSingleThreadTaskRunner), but returns true when called in base::ThreadTaskRunnerHandle::Get or base::SequencedTaskRunnerHandle::Get.

Is there anyway to use base::CurrentThread in a ThreadPool, or should I use ThreadTaskRunnerHandle/SequencedTaskRunnerHandle instead when base::CurrentThread is needed?

Best Regards,

Alex Chau | Software Engineer | alex...@google.com | 44-20-7346-2379

[François Doray]

What functionality is needed by your use case? To post a task to the current sequence, it is sufficient to use SequencedTaskRunnerHandle. base::CurrentThread exposes some advanced functionality which isn't available in the ThreadPool.

--
You received this message because you are subscribed to the Google Groups "scheduler-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to scheduler-de...@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CACv-AFKV%2BoZpofys2NLc8QEHRf4fkR5LSVOw-PA-iEDYYy3syQ%40mail.gmail.com.

[Alex Chau]

We need to use it for CurrentThread::AddDestructionObserver, called by here through Jingle library. The function that needs to call CurrentThread::AddDestructionObserve runs in a task runner created by base::ThreadPool::CreateTaskRunner, and we're getting an exception that CurrentThread is not set.

I understand that posting the CurrentThread::AddDestructionObserve call to SequencedTaskRunnerHandle would solve the problem. I'm curious on why CurrentThread API doesn't work in a ThreadPool, is it by design or is possible with some setup.

Alex Chau | Software Engineer | alex...@google.com | 44-20-7346-2379

[François Doray]

base::ThreadPool runs tasks in a dynamical pool of OS threads. Since these OS threads are shared by many components and can be teared down and recreated on demand, they're not exposed. This explains why base::CurrentThread::AddDestructionObserver is not supported in base::ThreadPool.

Fortunately, I think there is a way to support your use case. base::ThreadPool::CreateSequencedTaskRunner() allows you to create a "sequence" -- a queue of tasks which run one at a time in posting order. You can use SequenceLocalStorageSlot to attach an object to a sequence. The object is automatically deleted when the SequencedTaskRunner is released and all tasks posted to it have finished running. Does it work to store the JingleThreadWrapper in a SequenceLocalStorageSlot?

[Alex Chau]

Thanks François.

Attaching the Jingle object to SequenceLocalStorageSlot should work instead of attaching the object to CurrentThread, but that requires changing the Jingle library that our team doesn't own. We've taken the approach of using base::ThreadTaskRunnerHandle::Get instead of base::ThreadPool::CreateSingleThreadTaskRunner, and that solves our problem.

Alex Chau | Software Engineer | alex...@google.com | 44-20-7346-2379

[François Doray]

base::ThreadTaskRunnerHandle::Get returns a TaskRunner that schedules tasks on the current thread. If the current thread is on the critical path of navigation or input handling (example: main thread or IO thread) and tasks take more than a few milliseconds to execute, this can cause jank. base::ThreadPool::CreateSingleThreadTaskRunner returns a TaskRunner that schedules tasks on a thread that is not on the critical path of navigation or input handling. Don't hesitate to add me on a code review for more guidance.

[Wez]

IIUC the AddDestructionObserver() call-site is:

https://source.chromium.org/chromium/chromium/src/+/master:jingle/glue/thread_wrapper.cc;l=45

which is in the Chromium tree, not in libjingle.

The AddDestructionObserver() call could be replaced with use of SequenceLocalStorageSlot, as was previously suggested, to ensure that the JingleThreadWrapper is properly cleaned-up if/when the Sequence is torn-down.

Are you sure that the WebRTC code that you're trying to run on the ThreadPool is actually ThreadPool-compatible, though?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CAGD3t5Hu%3D33vTuB4M7EndzFHxUehgsWR6kPxO6oBQF-L%2B9oeEw%40mail.gmail.com.

[Alex Chau]

Right, using base::ThreadTaskRunnerHandle::Get doesn't seem the best solution as it has the main thread / IO thread problem that François mentioned.

I find that JingleThreadWrapper has a WrapTaskRunner method that can be used directly, it returns a unique_ptr and does not rely on AddDestructionObserver to maintain the lifetime of the JingleThreadWrapper. Using WrapTaskRunner with a dedicated base::ThreadPool::CreateSingleThreadTaskRunner should be a better solution. +Himanshu Jaju Can we try if this works?

(cc James in case we need a follow-up from you)

Alex Chau | Software Engineer | alex...@google.com | 44-20-7346-2379

[Wez]

On Wed, 2 Sep 2020 at 13:57, Alex Chau <alex...@google.com> wrote:

Right, using base::ThreadTaskRunnerHandle::Get doesn't seem the best solution as it has the main thread / IO thread problem that François mentioned.

I don't think it's a solution at all, actually - it just gets the TaskRunner for the thread on which the caller is running, whereas in this case there isn't a thread, but a ThreadPool sequence.

JingleThreadWrapper::EnsureForCurrentMessageLoop() checks whether there is a JingleThreadWrapper for the current thread and creates one if not - it is that thread-affinity that is the problem for your use-case, I think; the AddDestructionObserver() call is just a symptom of the mismatch.

I find that JingleThreadWrapper has a WrapTaskRunner method that can be used directly, it returns a unique_ptr and does not rely on AddDestructionObserver to maintain the lifetime of the JingleThreadWrapper. Using WrapTaskRunner with a dedicated base::ThreadPool::CreateSingleThreadTaskRunner should be a better solution. +Himanshu Jaju Can we try if this works?

I don't think that that will help - the JingleThreadWrapper::WrapTaskRunner() function must only be called from the same _thread_ on which the supplied TaskRunner runs tasks.  The call sets a TLS slot so that JingleThreadWrapper::current() will return that instance, and clears it when the wrapper is destroyed.

If you are somehow able to use libjingle (which I suspect has thread-affinity baked-in in other places) on ThreadPool sequences then you will need to update the JingleThreadWrapper to have it be sequence-affine rather than thread-affine.

HTH,

Wez

[Wez]

... or alternatively, should this code actually be running on a dedicated thread, e.g. via CreateSingleThreadTaskRunner(), rather than on a ThreadPool sequence?

[Alex Chau]

Yes, we'll try to use CreateSingleThreadTaskRunner which runs on a dedicated thread. Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1125628 to follow-up.

Alex Chau | Software Engineer | alex...@google.com | 44-20-7346-2379

[Gabriel Charette]

We want to avoid dedicated threads as much as possible (that's what CreateSingleThreadTaskRunner gives you). Very few use cases are thread-affine, most just need thread-safety which SequencedTaskRunners provide. Why doesn't using SequenceLocalStorageSlot as proposed by fdoray@ earlier work?

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/scheduler-dev/CACv-AF%2BM783bigrm9D3P0_swdTMZYzUCCyXC_eoTvgiC1kLVXw%40mail.gmail.com.

[Wez]

Updating the JingleThreadWrapper to use SequenceLocalStorageSlot would require that JingleThreadWrapper / libjingle / WebRTC are able to run correctly on a ThreadPool sequence.  Since libjingle/WebRTC interact with network resources, it's not immediately obvious (to me) whether they need a dedicated IO-capable thread, or not.

Responded in more detail on the bug.