use Ref, not preallocated arrays, for passing parameters by reference #44

musm · 2017-08-15T15:49:56Z

No description provided.

stevengj · 2017-08-15T16:12:01Z

See also the cy, ae, and wrk arrays later in the file.

As long as you're updating this, could you fix these to be thread-safe? Should be as simple as:

const ai = Vector{Float64}(2 * Threads.nthreads())

and then replace pointer(ai,1) and pointer(ai,2) with pointer(ai, threadoffset-1) and pointer(ai, threadoffset), respectively, where threadoffset = Threads.threadid()*2.

musm · 2017-08-15T16:13:45Z

Thanks @stevengj for the suggestion, will do.

stevengj · 2017-08-15T16:16:34Z

One thing that worries me is how my suggestion will interact with precompiling, however: we don't want it to use Threads.nthreads() from precompile time.

We may have to do const ai = Vector{Float64}(0) and then do resize!(ai, Threads.nthreads()*2) in the module __init__ function.

musm · 2017-08-16T15:42:37Z

@stevengj The arrays are constant, not sure why those need to be set based on the thread size? I updated the code to get rid of the & and have more appropriately used Ref (JuliaLang/julia#22734)

stevengj · 2017-08-16T18:25:21Z

The current code is not thread-safe. Imagine that two threads call _bessely in parallel. Because they are using the same arrays (wrk etc.) to pass arguments by reference, you will get a race condition where one thread could be stomping on the data of the other thread.

One solution is to make sure that each thread uses a separate memory location to pass variables by reference. To do this with a pre-allocated array, you have to allocate nthreads() copies of the array and use threadid() to determine which copy to use.

Another solution would be to not pre-allocate arrays, but rather to allocate new Ref variables for each call. (You already changed airy to do this.) I'm not sure what performance penalty this incurs.

(An even cleaner solution would be possible if Julia had language-level support for thread-local storage, or if becomes cheaper to allocate Ref variables as needed: JuliaLang/julia#8134.)

stevengj · 2017-08-16T18:26:32Z

src/bessel.jl

+           Ptr{Float64}, Ptr{Float64}, Ptr{Int32}, Ptr{Int32}),
+           rz1, rz2, rid, rkode,
+           ai1, ai2, ae1, ae2)
+    if ae2[] == 0 || ae2[] == 3 # ignore underflow and less than half machine accuracy loss


Hopefully, the performance penalty for allocating new Ref variables for each call, rather than using pre-allocated arrays, is negligible. Have you benchmarked it?

Yes on master they are equivalent (thanks to yuyichao's work) and this avoids the thread issues you have mentioned.

julia> @btime __airy(z,id,kode) # version in this PR 680.412 ns (1 allocation: 32 bytes) -2.197881420594205e-53 - 1.6155839776764676e-53im julia> @btime _airy(z,id,kode) 683.544 ns (1 allocation: 32 bytes) -2.197881420594205e-53 - 1.6155839776764676e-53im # Commit 952dc93489* (2017-08-02 23:54 UTC) (right before BenchmarkTools broke on master)

I will update the rest of the ccall's soon.

stevengj · 2017-08-16T20:04:55Z

src/bessel.jl

+
+    ccall((:zbiry_,openspecfun), Void,
+          (Ptr{Float64}, Ptr{Float64}, Ptr{Int32}, Ptr{Int32},
+           Ptr{Float64}, Ptr{Float64}, Ptr{Int32}),


These should be Ref, not Ptr. (Using Ptr here is probably a relic of the days before Ref.)

Once you change it to Ref, then you will no longer need to explicitly create Ref objects for parameters like kode that are purely inputs. You can just pass kode directly, and ccall will create the Ref for you. You only need to explicitly create Ref objects for output parameters like ai1.

Thank you. I have fixed them all up.

stevengj · 2017-08-16T20:06:20Z

src/bessel.jl

-    return z
+    rx = Ref(x)
+    rz = Ref(BigFloat())
+    ccall((:mpfr_ai, :libmpfr), Int32, (Ptr{BigFloat}, Ptr{BigFloat}, Int32), rz, rx, ROUNDING_MODE[])


Similarly, use Ref here and pass x, not rx.

stevengj · 2017-08-17T11:45:04Z

src/bessel.jl

+
+    ccall((:zairy_,openspecfun), Void,
+          (Ref{Float64}, Ref{Float64}, Ref{Int32}, Ref{Int32},
+           Ptr{Float64}, Ptr{Float64}, Ptr{Int32}, Ptr{Int32}),


The last four arguments should be Ref too.

There doesn't seem to be any difference in doing so, but will change since it seems more consistent

julia> f(x::Float64) = Base.unsafe_convert(Ref{Float64}, Base.cconvert(Ref{Float64},Ref{Float64}(x))) julia> g(x::Float64) = Base.unsafe_convert(Ptr{Float64}, Base.cconvert(Ptr{Float64},Ref{Float64}(x))) julia> @code_llvm f(2.) ; Function f ; Location: REPL[18] ; Function Attrs: uwtable define double* @julia_f_63230(double) #0 { top: %1 = alloca i64, align 8 %2 = bitcast i64* %1 to i8* %3 = bitcast i64* %1 to %jl_value_t* call void @llvm.lifetime.start(i64 8, i8* %2) ; Location: REPL[18]:1 %4 = bitcast %jl_value_t* %3 to double* ret double* %4 } julia> @code_llvm g(2.) ; Function g ; Location: REPL[19] ; Function Attrs: uwtable define double* @julia_g_63231(double) #0 { top: %1 = alloca i64, align 8 %2 = bitcast i64* %1 to i8* %3 = bitcast i64* %1 to %jl_value_t* call void @llvm.lifetime.start(i64 8, i8* %2) ; Location: REPL[19]:1 %4 = bitcast %jl_value_t* %3 to double* ret double* %4 }

I'm not sure If I should change this or not, in the example here @yuyichao uses a Ptr for arg type for the Ref in the example JuliaLang/julia#22684

Both Ptr and Ref work, but Ref is the recommended style for pass-by-reference arguments. I normally would only use Ptr{T} for passing pointers to arrays (or raw pointers, of course).

stevengj · 2017-08-17T11:49:09Z

src/bessel.jl

@@ -499,7 +509,7 @@ Bessel function of the first kind of order 0, ``J_0(x)``.
 """
 function besselj0(x::BigFloat)
    z = BigFloat()
-    ccall((:mpfr_j0, :libmpfr), Int32, (Ptr{BigFloat}, Ptr{BigFloat}, Int32), &z, &x, ROUNDING_MODE[])
+    ccall((:mpfr_j0, :libmpfr), Int32, (Ref{BigFloat}, Ref{BigFloat}, Int32), z, x, ROUNDING_MODE[])


Why doesn't z need to be a Ref here, since, since it is modified by the ccall?

I'm confused z is a Ref here?

z is being passed by reference, and is modified by mpfr_j0 . z is copied to a Ref(z) container by ccall when it is passed to the Ref{BigFloat} argument, and the mpfr_j0 actually modifies this copy. I think the reason is the original z is still modified is because a BigFloat object contains a pointer to the actual "limb" data, and this raw pointer was copied over to Ref(z), so z still sees the modified limbs.

Still, it seems like it would be clearer if we initialized z = Ref(BigFloat()) and then returned z[].

Agreed (yeah the logic is a bit confusing with all the cconverts and unsafe_converts that occur....)

So If I understand correctly the previous usage also had the same confusing behavior i.e. when passing &z .

z is being passed by reference, and is modified by mpfr_j0 .

Yes.

z is copied to a Ref(z) container by ccall when it is passed to the Ref{BigFloat} argument, and the mpfr_j0 actually modifies this copy.

No.

I think the reason is the original z is still modified is because a BigFloat object contains a pointer to the actual "limb" data, and this raw pointer was copied over to Ref(z), so z still sees the modified limbs.

So No.

Still, it seems like it would be clearer if we initialized z = Ref(BigFloat()) and then returned z[].

The two do the same and both mutate the origional BigFloat. Wrapping in Ref makes this less clear.

I see...so when you pass an instance z of a mutable type for a Ref argument then it actually passes a pointer to the original data in z?

Correct. It always pass an pointer to a memory with the same layout as the object being Refd (don't know how to describe this but hopefuly that's unambiguous). For !isbits objects this is the object itself and for a copy for isbits ones.

Use Vector for constant refs for airy and biry

74ab9e7

musm added 2 commits August 16, 2017 10:34

Update other constant vector arrays

03e5d89

Update _airy and _biry calls

a38a05d

stevengj reviewed Aug 16, 2017

View reviewed changes

Update all to use Ref

d5a2994

musm force-pushed the musm-patch-1 branch from f210e92 to d5a2994 Compare August 16, 2017 20:00

stevengj reviewed Aug 16, 2017

View reviewed changes

stevengj changed the title ~~Use Vector for constant refs for airy and biry~~ use Ref, not preallocated arrays, for passing parameters by reference Aug 16, 2017

musm force-pushed the musm-patch-1 branch from ca7fdc4 to c1679f4 Compare August 16, 2017 20:29

Address comment

61c57ec

musm force-pushed the musm-patch-1 branch from c1679f4 to 61c57ec Compare August 16, 2017 20:30

stevengj reviewed Aug 17, 2017

View reviewed changes

musm added 2 commits August 17, 2017 13:33

Address comments

53d011b

white space

ff213c6

stevengj approved these changes Aug 17, 2017

View reviewed changes

musm mentioned this pull request Aug 17, 2017

Misc remove &x ccall syntax JuliaLang/julia#23301

Merged

Change back

32f9002

musm merged commit aba8c8a into master Aug 17, 2017

musm deleted the musm-patch-1 branch August 17, 2017 22:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use Ref, not preallocated arrays, for passing parameters by reference #44

use Ref, not preallocated arrays, for passing parameters by reference #44

musm commented Aug 15, 2017

stevengj commented Aug 15, 2017 •

edited

Loading

musm commented Aug 15, 2017

stevengj commented Aug 15, 2017

musm commented Aug 16, 2017

stevengj commented Aug 16, 2017 •

edited

Loading

stevengj Aug 16, 2017

musm Aug 16, 2017 •

edited

Loading

musm Aug 16, 2017

stevengj Aug 16, 2017 •

edited

Loading

musm Aug 16, 2017

stevengj Aug 16, 2017

stevengj Aug 17, 2017

musm Aug 17, 2017 •

edited

Loading

musm Aug 17, 2017 •

edited

Loading

stevengj Aug 17, 2017

stevengj Aug 17, 2017

musm Aug 17, 2017

stevengj Aug 17, 2017 •

edited

Loading

musm Aug 17, 2017

musm Aug 17, 2017

yuyichao Aug 17, 2017

stevengj Aug 17, 2017

yuyichao Aug 17, 2017

use Ref, not preallocated arrays, for passing parameters by reference #44

use Ref, not preallocated arrays, for passing parameters by reference #44

Conversation

musm commented Aug 15, 2017

stevengj commented Aug 15, 2017 • edited Loading

musm commented Aug 15, 2017

stevengj commented Aug 15, 2017

musm commented Aug 16, 2017

stevengj commented Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

musm Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

musm Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

musm Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj Aug 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevengj commented Aug 15, 2017 •

edited

Loading

stevengj commented Aug 16, 2017 •

edited

Loading

musm Aug 16, 2017 •

edited

Loading

stevengj Aug 16, 2017 •

edited

Loading

musm Aug 17, 2017 •

edited

Loading

musm Aug 17, 2017 •

edited

Loading

stevengj Aug 17, 2017 •

edited

Loading