Thread pool's event APIs shouldn't be used to distribute work

[davisjam]

I'm trying to learn this module, and it's giving me some trouble. The thread pool doesn't seem to behave as I expect.

I'm working on a tweaked version of examples/quickIntro_multiThreadEvented.js as follows:

//Emit a bunch of 'giveMeTheFibo' requests
for (var i = 0; i < 100; i++)
  threadPool.any.emit('giveMeTheFibo', 30);

//Listener for the 'theFiboIs' events emitted by the child/background thread.
threadPool.on('theFiboIs', function cb (data) {
  process.stdout.write(" ["+ this.id+ "]"+ data);
});

There's still a threadpool of size 10.

When I run this, there's a noticeable pause, and then all of the requests are completed at once. In addition, they all complete on the same thread.

$ node quickIntro_multiThreadEvented_DAVISJAM.js
 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269

I tweaked the spinForever function to print statistics every so often.

$ node quickIntro_multiThreadEvented_DAVISJAM.js
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269 [0]1346269spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10
spinForever: totalThreads 10 pendingJobs 0 idleThreads 10

This seems to suggest the following sequence when I run my program:

1. I queue 100 expensive events
2. They all get emitted to thread 0
3. Each time thread 0 completes an event, it emits an event to itself (going at the end of its event queue)
4. Once the 100 expensive events finish, thread 0 handles all of the pending cleanup events

Step 3 is indeed suggested by the code of quickInto_evented_childThreadCode.js:

thread.on('giveMeTheFibo', function onGiveMeTheFibo (data) {
  this.emit('theFiboIs', fibo(+data)); //Emits 'theFiboIs' in the parent/main thread.
  ^^^^                                 ^^^^^^^^^^^^ Is this comment accurate? ^^^^^^^
});

So, here are my questions:

1. Why isn't the work being evenly distributed across the threadpool?
2. If I have more work pending in my threadpool than I have worker threads, and if I use the event scheme to submit requests and get responses, will my response events be artificially delayed by other pending work?

An additional, unrelated question:
3. I'm not sure how to get code from LiveScript to JavaScript to the .js.c files, so I can't tweak src/createPool.js and experiment a bit. Any hints or helpful links? I can't find anything about this online.

Regards,

Jamie

[davisjam]

OK, I've identified the cause of the problem.

Each time I call threadPool.any.emit (-> emitAny), it pushes the event onto the queue and then nextJob's it on a popped idleThreads VICTIM.

nextJob goes to case 2, where it calls VICTIM.emit() and then recurses into nextJob again with VICTIM.
The second time through nextJob, there's no work, and VICTIM is push()'d onto idleThreads. VICTIM will then be the next thread popped from idleThreads.

So, when I queue 100 events one after another, they are all delivered to the same thread VICTIM who has to handle all of them.

This answers question 1 and implies that once question 1 is fixed, question 2 should "just work".

I think a fix would be to unshift rather than push VICTIM into idleThreads in case 2 of nextJob. This should have no performance impact, since either way VICTIM ends up in idleThreads ready for more work.

I suspect (but haven't attempted to prove) that using threadPool.any.eval rather than threadPool.any.emit should result in an even distribution of work, just as the caller intends.

If I had an answer to question 3, I'd be happy to submit a pull request.

[davisjam]

The answer to question 3 seems to be

npm run-script js; npm install

While my proposal to unshift rather than push VICTIM onto idleThreads helps concurrency (all the threads take a turn), it doesn't address the underlying problem of the "done" events being executed on the threads rather than the threadPool. The current design means that if we use unshift(), we queue up N_EVENTS / N_THREADS_IN_POOL items per thread, and only call the associated "done" events once a thread has finished all of its pending "work" events.

This suggests that, as-is, trying to submit work via events is not a good idea.
Using threadPool.any.eval() instead works better.

I think that to do "work" and "done" via events, you would need to either:

1. Re-architect to have threads emit events onto the threadPool, and have the threadPool execute the "done" callbacks. This is analogous to the design in libuv.
2. Introduce a wrapper around the threadPool that only submits the next piece of work once the previous piece of work has completed. This is how examples/quickIntro_multiThreadEvented.js works, but it's not obvious from that (arguably misleading?) example or from the README that the approach I was attempting is actually unwise/unsupported.

I'm happy to extend the README to discuss this or to attempt the wrapper I described.
Let me know how I can help.

[brody4hire]

Unfortunately I do not think I understand this 100%. I ran a similar test in my work area and it seems to run 100 events in one thread, then 100 events in another thread, and so on. But I think it is clear that the thread pool is not 100% correct. I had a couple questions:

By "wrapper" do you mean making an external module, adding a better sample, adding more JavaScript to this module, or something else?

By "re-architect" did you mean fixing the JavaScript for threadPool, fixing the native code with some refactoring, a more extensive rewrite, or something else?

I think it would help the community if you can the README to describe the problem along with the proper approach and maybe a better sample.

I have mixed feelings about how much we should try to fix here. A part of me would favor keeping this project as lean and mean as possible, moving some of the higher-level features to a pure-JavaScript module. But I also wouldn't mind seeing the thread pool fixed here since this can be considered basic functionality.

Any comments from @audreyt (owner) or anyone else?

[davisjam]

wrapper I meant either an external module or a wrapping JavaScript class within this module to make the pool more useful without the re-architecting discussed next.

re-architect Suppose you're trying to use an 'emit events for work, and emit events for done' approach, as I did above. As I understand it, the problem is that each thread handles all of its pending 'work' events, and after completing each 'work' event it emits a 'done' event to itself, so the 'done' events won't be handled until all of the pending 'work' events are addressed. I think the threads need to be able to deliver events to the threadPool object instead. I don't think this is possible at the moment.

I don't think there's a problem with the sample per se. It demonstrates what seems to be the only way to use the evented approach with the threadPool. It would help, however, if the sample made clear that the other approach won't work.

Summary I think the thread pool is "core" functionality for this module, so I think either putting the event-friendly wrapper inside the module or performing the re-architecting I discussed is the right approach. I don't think a new module is a good answer.

[brody4hire]

@davisjam both your "wrapper" and "re-architect" ideas sound good to me. Unfortunately I am not so familiar with the event emitting code so if you contribute the "re-architect" idea I would definitely learn from it.

[jhecking]

+1 for the re-architecture: Same as @davisjam, I was also under the assumption that thread.emit() from the worker thread would send the event back to the thread pool. I was hoping to use the same approach to use an 'emit events for work, and emit events for done' for a proof-of-concept piece of work. I ended up using threadPool.any.eval() instead.