search

mikeseven / node-opencl

Low-level OpenCL 1.x and 2.x bindgings for node.js
156 stars 33 forks source link

Schedule AsyncQueueWorker to be called on the main thread #65

Open trxcllnt opened 6 years ago

trxcllnt commented 6 years ago

Hey @mikeseven if you don't mind looking this over, I think this is the fix for the failing uv_queue_done assertion I've been seeing. From what I gather, clSetEventCallback calls notifyCB off the main thread. If that's the case, we can't immediately call AsyncQueueWorker, because uv_queue_work isn't thread safe.

I followed the example of the CallbackBridge class in node-sass. C++ isn't my first language, so please feel free to clean this up.

It seems like there may still be an issue I haven't addressed: what's the behavior if someone calls clReleaseEvent() after setEventCallback, but before the callback has been invoked? Will we leak NoCLEventWorker instances?

mikeseven commented 6 years ago

Humm this is always a tricky one as they change the behavior and indeed nothing is guaranteed.

I'll have a look.

--mike

From: Paul Taylor notifications@github.com Sent: Saturday, May 19, 2018 9:55:34 AM To: mikeseven/node-opencl Cc: Bourges-sevenier, Mikael; Mention Subject: [mikeseven/node-opencl] Schedule AsyncQueueWorker to be called on the main thread (#65)

Hey @mikesevenhttps://github.com/mikeseven if you don't mind looking this over, I think this is the fix for the failing uv_queue_done assertion I've been seeing. From what I gather, clSetEventCallback calls notifyCB off the main thread. If that's the case, we can't immediately call AsyncQueueWorkerhttps://github.com/nodejs/nan/blob/946377f194e3aaf2aeb5141fa38768f5ca56734a/nan.h#L2163, because uv_queue_work isn't thread safe.

I followed the example of the CallbackBridgehttps://github.com/sass/node-sass/blob/94ce8529486643f350d8a658791a4e6204db0e70/src/callback_bridge.h class in node-sass. C++ isn't my first language, so please feel free to clean this up.

It seems like there may still be an issue I haven't addressed: what's the behavior if someone calls clReleaseEvent() after setEventCallback, but before the callback has been invoked? Will we leak NoCLEventWorker instances?

You can view, comment on, or merge this pull request online at:

https://github.com/mikeseven/node-opencl/pull/65

Commit Summary

File Changes

Patch Links:

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/65, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAxYLAzM_rSQYBv9dCT4hSV-mN3gegbuks5t0E6GgaJpZM4UFxr9.

trxcllnt commented 6 years ago

Thanks Mike! Here are links to a few of the discussions I found as I was digging in:

I looked into using AsyncProgressWorker, as it does handle waking the main event-loop thread via uv_async_send, but I couldn't figure out how to change NoCLEventWorker to extend it instead of AsyncWorker. Hope this helps!

mikeseven commented 6 years ago

This is the normal behavior. Good this hasn't changed.

The problem with CL events is that they come from a thread in the driver and it varies across implementations ie some use a unique thread to propagate events, others use multiple threads.

So when we receive such event, we build a js event and queue it into js world, wrapping the native event.

As long as js code remains in the same thread, this shouldn't be an issue. If on the other end the js code uses lots of async constructs then there is a chance that the native cl context is reused across threads and so are events; this can be an issue. However, this is really an issue of the cl driver that hasn't been tested enough for such async scenarios. In other words, some drivers are better than others.

This was an issue years ago but should be less and less the case today. Oh well, gotta test....

--mike

From: Paul Taylor Sent: Saturday, 19 May, 18:10 Subject: Re: [mikeseven/node-opencl] Schedule AsyncQueueWorker to be called on the main thread (#65) To: mikeseven/node-opencl Cc: Bourges-sevenier, Mikael, Mention

Thanks Mike! Here are links to a few of the discussions I found as I was digging in: nodejs/nan#526 (comment)https://github.com/nodejs/nan/issues/526#issuecomment-165757856 Nan::AsyncQueueWorker() is supposed to be called from the main thread. If you want to wake up the main thread from another thread, you'll have to jury-rig something using uv_async_send(). Note that the uv_async_t handle should be initialized on the main thread first. https://stackoverflow.com/questions/43220524/uv-queue-done-assertion#comment73652659_43220524 uv_queue_work is not thread-safe. You must call it from the loop thread. See docs.libuv.org/en/v1.x/design.html#the-i-o-loophttp://docs.libuv.org/en/v1.x/design.html#the-i-o-loop nodejs/nan#669 (comment)https://github.com/nodejs/nan/issues/669#issuecomment-300895519 uv_queue_work should only be called from the main thread, it will queue a job to run in one of uv's worker threads, and the job's callback will run in the main thread. uv_async_send , however, can be called in any thread and will queue a job to wake and run in the main thread. This is described in much better detail in the libuv documentationhttp://docs.libuv.org/. As for a wrapper for worker creation from a foreign thread, all of the advice that I have read so far recommends to use AsyncProgressWorker for this, since it uses uv_async_send for its async worker functionality. I looked into using AsyncProgressWorker, as it does handle waking the main event-loop thread via uv_async_send, but I couldn't figure out how to change NoCLEventWorker to extend it instead of AsyncWorker. Hope this helps! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/mikeseven/node-opencl/pull/65#issuecomment-390448012, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AAxYLC_JJ7RdCY3M24LSUgusUw1-Wc_aks5t0MJrgaJpZM4UFxr9.