1364841 - Remove the default AbstractThread wrapper created by MessageChannel and require manual management in actors

Status: NEW

(Core :: IPC, enhancement, P3)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

After discuss with JW we agreed that using AbstractThread::GetCurrent() directly is often unclear which thread will be used for dispatch. He plans to remove AbstractThread::GetCurrent() and IPC actor code are encouraged to maintain the wrapper for their thread.

I think the explicitness is good that caller can be more aware that there are more options for task scheduling.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Uh, removing the concept of "current thread" seems kind of disruptive.  Why not just require threads participating in IPC to initialize their AbstractThread wrapper explicitly?  We already do this for main thread.  Worker threads and the PBackground worker thread could also explicitly create a wrapper at the top of their run loop.  No need to dynamically create this stuff in the IPC actor layer.

Comment 2

[JW Wang [:jwwang] [:jw_wang]]

Consider the following code:

  nsCOMPtr<nsIThread> mainThread;
  NS_GetMainThread(getter_AddRefs(mainThread));

  RefPtr<AbstractThread> at1 = new EventTargetWrapper(mainThread.get());
  RefPtr<AbstractThread> at2 = new EventTargetWrapper(mainThread.get());
  RefPtr<AbstractThread> at3 = new EventTargetWrapper(mainThread.get());

For a task running on the main thread, what do we expect AbstractThread::GetCurrent() to return? Should it be at1, at2 or at3?

Unlink NS_GetMainThread() which always returns the same instance, AbstractThread::GetCurrent() is ambiguous in this case. See bug 1364821 for more discussion about the ambiguity.

Comment 3

[Kan-Ru Chen [:kanru] (UTC+9)]

(In reply to Ben Kelly [reviewing, but slowly][:bkelly] from comment #1)
> Uh, removing the concept of "current thread" seems kind of disruptive.  Why
> not just require threads participating in IPC to initialize their
> AbstractThread wrapper explicitly?  We already do this for main thread. 
> Worker threads and the PBackground worker thread could also explicitly
> create a wrapper at the top of their run loop.  No need to dynamically
> create this stuff in the IPC actor layer.

The concept of "current thread" is actually not clear when TaskQueue or nsIThreadPool are involved. It's even more complicated that Quantum DOM can have multiple queues.

Like the example in comment 2, if a thread has a default wrapper then AbstractThread::GetCurrent() can mean different things depending on which wrapper was used to dispatch the current task. For example on main thread, GetCurrent() can return either sMainThread or one of the abstract main threads.

Maybe we can add a new method AbstractThread::CurrentNativeThread() that always return the wrapper of the worker threads to make the intent clearer and still maintain the ease of use for simple cases.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Well, in theory a queue is not a thread.  I think this is part of the reason we have nsIEventTarget as an interface, but NS_GetCurrentThread() gets a more specific thing than an event target.  I think the fact AbstractThread conflates these distinctions is a design flaw.

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

I don't quite get it. In which sense an AbstractThread combine a queue and a thread?

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I was trying to say TaskQueue should probably not have an "is-a" relationship with AbstractThread because its not a thread.  The fact that your "get a current thread" thing doesn't make sense for TaskQueue probably stems from that design choice.

But anyway, I will stop commenting here and let you move forward.

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.