-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Schedule AsyncQueueWorker to be called on the main thread #65
base: feature/upgrade_to_NAN_2.8.0
Are you sure you want to change the base?
Schedule AsyncQueueWorker to be called on the main thread #65
Conversation
2 fixes for test_leak1.js on Win10 x64 Nvidia Drivers
…eseven/node-opencl into feature/upgrade_to_NAN_2.8.0
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 @mikeseven<https://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 AsyncQueueWorker<https://github.com/nodejs/nan/blob/946377f194e3aaf2aeb5141fa38768f5ca56734a/nan.h#L2163>, because uv_queue_work isn't thread safe.
I followed the example of the CallbackBridge<https://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:
#65
Commit Summary
* 2 fixes for test_leak1.js on Win10 x64 Nvidia Drivers
* Merge pull request #61 from jeffallen6767/master
* move some things from dependencies to devDependencies
* upgraded to latest NAN 2.8.0, checked tests on MacBook Pro's crappy OpenCL 1.2
* updated with CL 2.1 headers (methods not implemented yet)
* updated with CL 2.2 header (no method implementation yet)
* cleaned up deprecated warnings with v8 and nan
* updated examples to support OCL 1.2 and 2. Tested on Mac (opencl 1.2).
* blacklisted some tests not working, buggy driver, on Mac OS X.
* Merge branch 'feature/upgrade_to_NAN_2.8.0' of https://github.com/mikeseven/node-opencl into feature/upgrade_to_NAN_2.8.0
* schedule AsyncQueueWorker to be called on the main thread
File Changes
* A package-lock.json<https://github.com/mikeseven/node-opencl/pull/65/files#diff-0> (787)
* M package.json<https://github.com/mikeseven/node-opencl/pull/65/files#diff-1> (13)
* M src/event.cpp<https://github.com/mikeseven/node-opencl/pull/65/files#diff-2> (34)
* M test/test_leak1.js<https://github.com/mikeseven/node-opencl/pull/65/files#diff-3> (4)
Patch Links:
* https://github.com/mikeseven/node-opencl/pull/65.patch
* https://github.com/mikeseven/node-opencl/pull/65.diff
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#65>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAxYLAzM_rSQYBv9dCT4hSV-mN3gegbuks5t0E6GgaJpZM4UFxr9>.
|
Thanks Mike! Here are links to a few of the discussions I found as I was digging in:
I looked into using |
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)<nodejs/nan#526 (comment)>
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-loop<http://docs.libuv.org/en/v1.x/design.html#the-i-o-loop>
nodejs/nan#669 (comment)<nodejs/nan#669 (comment)>
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 documentation<http://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 GitHub<#65 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AAxYLC_JJ7RdCY3M24LSUgusUw1-Wc_aks5t0MJrgaJpZM4UFxr9>.
|
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
callsnotifyCB
off the main thread. If that's the case, we can't immediately call AsyncQueueWorker, becauseuv_queue_work
isn't thread safe.I followed the example of the
CallbackBridge
class innode-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()
aftersetEventCallback
, but before the callback has been invoked? Will we leakNoCLEventWorker
instances?