Implements event_notify using safe operations #87

vchuravy · 2015-11-07T04:39:06Z

Fixes #86.

julia> using OpenCL

julia> code_llvm(OpenCL.event_notify, (OpenCL.CL_event, OpenCL.CL_int,Ptr{Ptr{Void}}))

define void @julia_event_notify_21517(i8*, i32, i8**) {
top:
  %3 = load i8** %2, align 1
  %4 = getelementptr i8** %2, i64 1
  %5 = load i8** %4, align 1
  %6 = bitcast i8* %5 to i8**
  %7 = getelementptr i8** %2, i64 2
  %8 = load i8** %7, align 1
  %9 = bitcast i8* %8 to i32*
  store i8* %0, i8** %6, align 1
  store i32 %1, i32* %9, align 1
  call void inttoptr (i64 140256579765648 to void (i8*)*)(i8* inreg %3)
  ret void
}

vchuravy · 2015-11-07T04:40:32Z

src/event.jl

+    evt_id = Ref{CL_event}(0)
+    status = Ref{CL_int}(0)
+    cb = Base.SingleAsyncWork(data -> callback(evt_id[], status[]))
+    ptrs = [cb.handle, Base.unsafe_convert(Ptr, evt_id), Base.unsafe_convert(Ptr, status)]


We need to make sure that this doesn't get GC'd so I am contemplating adding an ObjectIdDict to CLEvent. @jakebolewski Any better Ideas?

vchuravy · 2015-11-07T04:44:44Z

A problem that I see with this implementation is that each callback has only one set of return values, but it is entirely possible that event_notify is called multiple times before or during data -> callback(evt_id[], status[]) is run, thus overwriting the return values.

vchuravy · 2015-11-07T06:43:52Z

This fix only works on Julia v0.4. @jakebolewski Any ideas how to achieve the same with 0.3?

I currently don't have a computer with OpenCL around so I am not quite sure why the tests are hanging on the events. Will see if I can try it out during work.

vchuravy · 2015-11-08T01:44:20Z

I am running into a weird seqfault,

julia -i -L test_ev.jl

signal (11): Segmentation fault
jl_field_index at /usr/bin/../lib/julia/libjulia.so (unknown line)
jl_f_get_field at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab466459)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab46554c)
unknown function (ip: 0x7f62ab464480)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab463d31)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab45dd49)
unknown function (ip: 0x7f62ab4603e0)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab45a51a)
unknown function (ip: 0x7f62ab45ab46)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62ab45a0bb)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62aeaf6e28)
unknown function (ip: 0x7f62aeaf7724)
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
julia_notify_21330 at  (unknown line)
jlcall_notify_21330 at  (unknown line)
task_done_hook at task.jl:145
jl_apply_generic at /usr/bin/../lib/julia/libjulia.so (unknown line)
unknown function (ip: 0x7f62aeb57756)
unknown function (ip: 0x7f62aeb5836c)
unknown function (ip: (nil))
[1]    9941 segmentation fault (core dumped)  julia -i -L test_ev.jl

Where:

using OpenCL
const cl = OpenCL

ctx = cl.create_some_context()
q = cl.CmdQueue(ctx)

callback_called = false
function test_callback(evt, status)
  callback_called = true
  println("Test Callback $evt, $status")
end

usr_evt = cl.UserEvent(ctx)
cl.enqueue_wait_for_events(q, usr_evt)
mkr_evt = cl.enqueue_marker(q)

cl.add_callback(mkr_evt, test_callback)

cl.complete(usr_evt)

julia> code_llvm(OpenCL.event_notify, (OpenCL.CL_event, OpenCL.CL_int,Ptr{Void}))

define void @julia_event_notify_21517(i8*, i32, i8*) {
top:
  %3 = bitcast i8* %2 to i8**
  %4 = load i8** %3, align 1
  %5 = getelementptr i8* %2, i64 8
  %6 = bitcast i8* %5 to i8**
  %7 = load i8** %6, align 1
  %8 = bitcast i8* %7 to i8**
  %9 = getelementptr i8* %2, i64 16
  %10 = bitcast i8* %9 to i8**
  %11 = load i8** %10, align 1
  %12 = bitcast i8* %11 to i32*
  store i8* %0, i8** %8, align 1
  store i32 %1, i32* %12, align 1
  call void inttoptr (i64 140438188430736 to void (i8*)*)(i8* inreg %4)
  ret void
}

@vtjnash Do you have any insight in what I might be doing wrong/or if this is the right direction/
The issue I am facing is to pass data from the callback back to the original Julia process.

vtjnash · 2015-11-08T20:02:31Z

src/event.jl

+    p_status = Base.unsafe_convert(Ptr{CL_int}, r_status)
+
+    cb = Base.SingleAsyncWork(data -> begin
+		println("Received callback")


println isn't really allowed here. the SingleAsyncWork callback is being run directly from libuv's callback, so anything that may try to block (such as the flush at the end of printing) could result in a recursive call to the libuv event loop. a better pattern is for the SingleAsyncWork callback to notify a Condition variable, and do the real work on a thread that monitors that variable (a PR to Base to replace the callback with a Condition variable would be a useful improvement to this API).

So instead I would have:

cond = Condition() cb = Base.SingleAsyncWork(data -> notify(cond)) @async begin wait(cond) callback(r_evt_id[], r_status[]) end

Could it be that the main issue I am running into is that I am trying to write memory in a protected address space?

vchuravy · 2015-11-09T16:03:22Z

@vtjnash Could you take another look?

This is my most promising attempt yet and the callback compiles to:

julia> code_llvm(OpenCL.event_notify, (OpenCL.CL_event, OpenCL.CL_int,Ptr{Void}))

define void @julia_event_notify_21521(i8*, i32, i8*) {
top:
  %3 = bitcast i8* %2 to %_EventCB*
  %4 = bitcast i8* %2 to i8**
  %5 = load i8** %4, align 8
  %6 = insertvalue %_EventCB undef, i8* %5, 0
  %7 = insertvalue %_EventCB %6, i8* %0, 1
  %8 = insertvalue %_EventCB %7, i32 %1, 2
  store %_EventCB %8, %_EventCB* %3, align 1
  call void inttoptr (i64 139700456842640 to void (i8*)*)(i8* inreg %5)
  ret void
}

jakebolewski · 2015-11-09T22:44:08Z

I believe this solution should work, although should there be a corresponding unpreserve_callback? otherwise all registered callbacks would never get GC'd.

vchuravy · 2015-11-10T00:28:52Z

@jakebolewski Thanks. Before this goes in we would need to bump the minimal version to 0.4 so I will start a PR for that.

jakebolewski · 2015-11-10T02:00:35Z

I think dropping support for 0.4 is reasonable. Some parts of the API can be simplified with call overloading.

vchuravy · 2015-11-10T02:33:39Z

I hope you meant dropping support for 0.3 ;)

vchuravy · 2015-11-10T02:45:50Z

https://github.com/JuliaGPU/OpenCL.jl/blob/master/src/context.jl#L32-L38 Should probably receive the same treatment?

vchuravy · 2015-11-10T05:16:05Z

So rebased and squashed and I will start to fix the callback in context.jl, because baased on the docs [1] the callback can/will happen asynchronously.

[1] https://www.khronos.org/registry/cl/sdk/1.0/docs/man/xhtml/clCreateContext.html

The callback itself should only use basic and thread-safe operations and should also not block. The result from the callback is stored in a reference to an immutable and a waiting task notify to get the result value and dispatch the user-supplied callback.

vchuravy · 2015-11-10T15:08:16Z

@jakebolewski Take a look at this and if you agree we can merge, after fixing the oom travis error we can go ahead and tag a new release.

Implements event_notify using safe operations

vchuravy reviewed Nov 7, 2015
View reviewed changes

vchuravy force-pushed the vc/fix_callback branch from 6c26bb8 to a6ce169 Compare November 7, 2015 06:19

vchuravy mentioned this pull request Nov 7, 2015

fix syntax #88

Merged

vchuravy mentioned this pull request Nov 8, 2015

Base structure for Native Operators dmlc/MXNet.jl#16

Closed

2 tasks

vtjnash reviewed Nov 8, 2015
View reviewed changes

vchuravy force-pushed the vc/fix_callback branch from ae570b9 to 6eead50 Compare November 9, 2015 16:11

vchuravy mentioned this pull request Nov 10, 2015

drop support for v0.3 and remove Compat #89

Merged

vchuravy force-pushed the vc/fix_callback branch from 05fb9a6 to c1e0421 Compare November 10, 2015 05:13

vchuravy added 2 commits November 10, 2015 14:21

fix event_notify callback

559c583

The callback itself should only use basic and thread-safe operations and should also not block. The result from the callback is stored in a reference to an immutable and a waiting task notify to get the result value and dispatch the user-supplied callback.

update context_err callback to be truely async

267266e

vchuravy force-pushed the vc/fix_callback branch from c1e0421 to 267266e Compare November 10, 2015 05:38

vchuravy mentioned this pull request Nov 17, 2015

Current Event callback uses undefined behaviour #86

Closed

vchuravy added a commit that referenced this pull request Nov 17, 2015

Merge pull request #87 from JuliaGPU/vc/fix_callback

712ba87

Implements event_notify using safe operations

vchuravy merged commit 712ba87 into master Nov 17, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implements event_notify using safe operations #87

Implements event_notify using safe operations #87

vchuravy commented Nov 7, 2015

vchuravy Nov 7, 2015

vchuravy commented Nov 7, 2015

vchuravy commented Nov 7, 2015

vchuravy commented Nov 8, 2015

vtjnash Nov 8, 2015

vchuravy Nov 9, 2015

vchuravy Nov 9, 2015

vchuravy commented Nov 9, 2015

jakebolewski commented Nov 9, 2015

vchuravy commented Nov 10, 2015

jakebolewski commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015

Implements event_notify using safe operations #87

Implements event_notify using safe operations #87

Conversation

vchuravy commented Nov 7, 2015

vchuravy Nov 7, 2015

Choose a reason for hiding this comment

vchuravy commented Nov 7, 2015

vchuravy commented Nov 7, 2015

vchuravy commented Nov 8, 2015

vtjnash Nov 8, 2015

Choose a reason for hiding this comment

vchuravy Nov 9, 2015

Choose a reason for hiding this comment

vchuravy Nov 9, 2015

Choose a reason for hiding this comment

vchuravy commented Nov 9, 2015

jakebolewski commented Nov 9, 2015

vchuravy commented Nov 10, 2015

jakebolewski commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015

vchuravy commented Nov 10, 2015