1628204 - minor refactoring on dispatching SourceCompressionTask

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P2)

Description

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

The taskSpecs in HelperThread has a Handler function,
https://searchfox.org/mozilla-central/rev/a707541ff423ade0d81cef6488e6ecfa09273886/js/src/vm/HelperThreads.cpp#2466

And there are many similiar code in each handler, like
Assert(idle());
Assert(CanStart...())

currentTask.emplace(..);

// Handle the task

currentTask.reset();

notify(CONSUMER);

We could move those similar code out of the handlers.
The idea is in the case of Tasks are dispatched to XPCOM thread pool, we could reuse the handler.

Comment 1

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204: Refactor Handler in HelperThread.

Comment 2

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Hi, Jonco
This is the refactoring for HelperThread as I mentioned last week,
can you give it a look?

Also for the Wasm case, I have a WIP patch in bug 1569819.

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #2)
To be able to use the XPCOM thread pool, we need to split the work that needs to run on the helper thread from the work to select and dispatch tasks. Everything that needs to run on the other thread must be moved into an implementation of RunnableTask::runTask().

This change factors out the work to get the task to run, so this is a good first step. Following on from this, everything in the handleFooTask() methods needs to be either moved into runTask() or into the current thread scheduler. Eventually we should just run the task by calling runTask() in HelperThread::threadLoop.

I'm not sure whether a partial migration to the XPCOM thread pool is going to be possible. There does need to be some kind of scheduling still (as you've seen with the restriction on running as single Wasm job at once) and I don't see how you can have two schedulers at the same time.

Currently scheduling works by picking the next task to run in HelperThread::threadLoop, or waiting for a new task to be queued. That won't work for the XPCOM thread pool because we don't have constantly running threads. Instead we could have a function that checks whether a new task can be scheduled and if so starts it. This would need to be called when every helper thread task is queued and also when every helper thread task finishes. That would let us keep our current scheduling policy.

One direction to approach this from might be to build a simple thread pool for the shell that exposes the same interface as the XPCOM one, and then change the shell to use this first.

Comment 4

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - (WIP) Refactor with HelperThreadPool.

Comment 5

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Hi Jonco
This is the WIP patch with HelperThreadPool.
This adds a PriorityQueue with js::UniquePtr<RunnableTask>, so when we start an off thread task, like wasm tier2 generator in bug 1569819,
we insert that task into the queue, then the similar threadLoop will fetch the new task from the queue and call runTask() directly.
And PriorityQueue doesn't support move sematics (same for std:priority_queue), so I add a partial specialization for Priorty_Queue with js::UniquePtr.
I haven't run any test yet so it could be very bogus....

could you take a look?
Thanks

Comment 6

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - (WIP) Refactor HelperThreadState

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Hi Jonco
This version I added some check before dispatching the task, please check js::StartOffThreadWasmTier2Generator for detail.
It will check if we have avaialble thread and this task could be dispatched, and if so, we call HelperThreadTaskCallback to dispatch it.

Meanwhile there's a threadLoopNew() and canDispatch.*Task() functions, which are similar to original threadLoop() and canStart.*Task()
I'll try to merge them later.

So far the task dispatched to SM HelperThreads still uses the same way, we push to the corresponding task queue, and HelperThread checks getHighestPriorityTask() and run it. (see threadLoopNew())

For XPCOM thread we make sure there are still available threads and the task doesn't exceed its limit, then we dispatch the getHighestPriorityTask() to XPCOM thread pool, and when the RunnableTask is about to finished, we call a callback in GlobalHelperThreadState to see if there's another high priority task, and if there is, we dispatch the next high priority task again.

Could you check this?

Thanks

Comment 8

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 4: Check if gHelperThread still exists.

Comment 9

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - HelperThread inherits RunnableTask

Comment 10

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

After fixed bug 1634720 I went back to review my patches, and found one simpler way to implement this. So far this is still WIP, but at least the try on linux64(debug + opt) are all green. There're still some ongoing patches will be added.

Comment 11

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #10)
I'm not sure about this approach. If I understand correctly, this runs our current helper thread loop in an XPCOM runnable task. This won't play fair with other users of the thread pool that may be starved if we continually run tasks.

Comment 12

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

(In reply to Jon Coppeard (:jonco) from comment #11)

I'm not sure about this approach. If I understand correctly, this runs our current helper thread loop in an XPCOM runnable task. This won't play fair with other users of the thread pool that may be starved if we continually run tasks.
Yes, the HelperThread running on XPCOM thread will keep running the tasks until all the tasks are executed. Then the thread will return to HelperThreadPool.
I thought this was okay because normally SM dispatches more than 1 SourceComressionTasks to HelperThreads.

I'll update the patch to let it run only 1 task from queue, once the task is finished it will check if the queue is empty, if not empty it will schedule another RunnableTask to XPCOM thread pool.

Comment 13

[Jon Coppeard (:jonco)]

That would be better but I'm still not sure this is the right approach. Really we need to have a separate abstraction for a task than a thread.

Comment 14

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 3: Add a dispatch function for dispatching tasks.

Comment 15

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 5: Add mShutdown for HelperThreadPool.

Added a mShutdown to prevent thread race and leak when nsThreadPool is shutdown.
This is needed because the dispatch() will be called on a non-main
thread.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Jonco, can you check the patch and give me some comments?

Thanks

Comment 17

[Jon Coppeard (:jonco)]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #16)
Comments in phabricator.

Comment 18

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Jonco, I just update the code with your comments addressed, could you check it?
Thanks

Comment 19

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

ni? jonco again for the updated patches, thanks

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 1: Move append to compressionFinishedList inside runTask.

Comment 21

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

https://phabricator.services.mozilla.com/D78461 moves the code inside runTask,
however it still has some asan failures
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a4e213567f7c6a30e1efd16977d80e10d535052&selectedTaskRun=e1q_9sOOSxmLVLOnhbaIPQ-0

The reason is originally "append to compressionFinishList" and currentTask.reset() will be executed atomically, but now there's an extra unlock before currentTask.reset(), and other thread executes AttachToFinishList and deletes the pointer, and now 3rd thread calls CancelOffThreadCompressionTask and try to get helper.compressionTask(), but the pointer has been deleted.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Hi Jonco
Or does it make sense to call SourceCompressionTask->complete() after runTask on a helper thread?
Now I got a TSAN failure, one is a helper thread calling complete(), and the main thread is calling trace().

Comment 23

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

fixed to heap-use-after-free problem by delaying the timing when the SourceCompresionsTask is release from compressionFinishedList.

Comment 24

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

So far I found there are some tests on try will have leak, still figuring out where the leak is from.
I've tried to make all subclasses of RunnableTasks like SourceCompressionTask, ParseTask, wasm::Tier2Generator, IonCompileTask .... to inherit HelperTask, but IonCompileTasks seems to have more problems (I found many assertions related to IonScript, IonCompileTask in my try push), so in the end I've only made SourceComressionTask inherit HelperTask, other tasks remain the same (inherit RunnableTask).

Comment 25

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #21)

https://phabricator.services.mozilla.com/D78461 moves the code inside runTask,
however it still has some asan failures
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a4e213567f7c6a30e1efd16977d80e10d535052&selectedTaskRun=e1q_9sOOSxmLVLOnhbaIPQ-0

The reason is originally "append to compressionFinishList" and currentTask.reset() will be executed atomically, but now there's an extra unlock before currentTask.reset(), and other thread executes AttachToFinishList and deletes the pointer, and now 3rd thread calls CancelOffThreadCompressionTask and try to get helper.compressionTask(), but the pointer has been deleted.

Updated the patch with jonco's comment https://phabricator.services.mozilla.com/D78461#inline-456075
now runTaskImpl takes a lock can fix this.

(In reply to Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh] from comment #24)

So far I found there are some tests on try will have leak, still figuring out where the leak is from.
The leak is calling nsThreadPool::Dispatch while it's shutdown, filed the bug 1645153

Jonco, can you look the latest patch again? Thanks

Comment 26

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 2: Don't submit compression tasks after shutdown.

Comment 27

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 2: Add a HelperTask.

Comment 28

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Attachment: Bug 1628204 - Part 6: release mOffThreadTask.

revert https://phabricator.services.mozilla.com/D74736

In handleCompression the task will be moved to compressionFinishedList
and later main thread will use compressionFinishedList to do the
finishing.

So we need to release the ownership otherwise the task will be
released by HelperThreadTaskHandler.

Comment 29

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

Updated the part 3 patch, so we don't dispatch to XPCOM at the moment.

Comment 30

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO ~ Nov. 1]

We only want to land the refactoring parts in this bug atm.
https://phabricator.services.mozilla.com/D78461
https://phabricator.services.mozilla.com/D79420

try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1df47233ea678b9a087e21e553436c057767189f&selectedTaskRun=KJosgBWPSBS3Rm1QzmP22g.0

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7a81a85d9881
Part 1: Move append to compressionFinishedList inside runTask. r=jonco
https://hg.mozilla.org/integration/autoland/rev/8050fb033d92
Part 2: Don't submit compression tasks after shutdown. r=jonco

Comment 32

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/7a81a85d9881
https://hg.mozilla.org/mozilla-central/rev/8050fb033d92