Skip to content

Commit

Permalink
make Ref(x) always construct a RefValue(x) object (#21527)
Browse files Browse the repository at this point in the history
the old behavior is now directly a feature of `convert` only instead
  • Loading branch information
vtjnash authored Dec 21, 2017
1 parent 555264e commit b57a592
Show file tree
Hide file tree
Showing 4 changed files with 19 additions and 6 deletions.
2 changes: 2 additions & 0 deletions NEWS.md
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,8 @@ Language changes
* Prefix `&` for by-reference arguments to `ccall` has been deprecated in favor of
`Ref` argument types ([#6080]).

* The constructor `Ref(x::T)` now always returns a `Ref{T}` ([#21527]).

* All line numbers in ASTs are represented by `LineNumberNode`s; the `:line` expression
head is no longer used. `QuoteNode`s are also consistently used for quoted symbols instead
of the `:quote` expression head (though `:quote` `Expr`s are still used for quoted
Expand Down
5 changes: 5 additions & 0 deletions base/deprecated.jl
Original file line number Diff line number Diff line change
Expand Up @@ -3426,6 +3426,11 @@ workspace() = error("workspace() is discontinued, check out Revise.jl for an alt
# PR #25113
@deprecate_binding CartesianRange CartesianIndices

# PR 21527
@deprecate Ref(x::AbstractArray) Ref(x, 1)
@deprecate Ref(x::Ptr) Ref(x, 1)
@deprecate Ref(x::Ref) x # or perhaps, `convert(Ref, x)`

# END 0.7 deprecations

# BEGIN 1.0 deprecations
Expand Down
2 changes: 1 addition & 1 deletion base/libgit2/merge.jl
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ Return two outputs, `analysis` and `preference`. `analysis` has several possible
function merge_analysis(repo::GitRepo, anns::Vector{GitAnnotated})
analysis = Ref{Cint}(0)
preference = Ref{Cint}(0)
anns_ref = Ref(map(a->a.ptr, anns))
anns_ref = Ref(map(a->a.ptr, anns), 1)
anns_size = Csize_t(length(anns))
@check ccall((:git_merge_analysis, :libgit2), Cint,
(Ptr{Cint}, Ptr{Cint}, Ptr{Cvoid}, Ptr{Ptr{Cvoid}}, Csize_t),
Expand Down
16 changes: 11 additions & 5 deletions base/refpointer.jl
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,16 @@ An object that safely references data of type `T`. This type is guaranteed to po
valid, Julia-allocated memory of the correct type. The underlying data is protected from
freeing by the garbage collector as long as the `Ref` itself is referenced.
In Julia, `Ref` objects are dereferenced (loaded or stored) with `[]`.
Creation of a `Ref` to a value `x` of type `T` is usually written `Ref(x)`.
Additionally, for creating interior pointers to containers (such as Array or Ptr),
it can be written `Ref(a, i)` for creating a reference to the `i`-th element of `a`.
When passed as a `ccall` argument (either as a `Ptr` or `Ref` type), a `Ref` object will be
converted to a native pointer to the data it references.
There is no invalid (NULL) `Ref`.
There is no invalid (NULL) `Ref` in Julia, but a `C_NULL` instance of `Ptr` can be passed to a `ccall` Ref argument.
"""
Ref

Expand Down Expand Up @@ -44,14 +50,14 @@ end
RefValue(x::T) where {T} = RefValue{T}(x)
isassigned(x::RefValue) = isdefined(x, :x)

Ref(x::Ref) = x
Ref(x::Any) = RefValue(x)
Ref(x::Ptr{T}, i::Integer=1) where {T} = x + (i-1)*Core.sizeof(T)
Ref(x, i::Integer) = (i != 1 && error("Object only has one element"); Ref(x))
Ref{T}() where {T} = RefValue{T}() # Ref{T}()
Ref{T}(x) where {T} = RefValue{T}(x) # Ref{T}(x)
convert(::Type{Ref{T}}, x) where {T} = RefValue{T}(x)

Ref(x::Ref, i::Integer) = (i != 1 && error("Ref only has one element"); x)
Ref(x::Ptr{T}, i::Integer) where {T} = x + (i - 1) * Core.sizeof(T)

function unsafe_convert(P::Type{Ptr{T}}, b::RefValue{T}) where T
if isbits(T) || isbitsunion(T)
return convert(P, pointer_from_objref(b))
Expand Down Expand Up @@ -81,7 +87,7 @@ end
RefArray(x::AbstractArray{T}, i::Int, roots::Any) where {T} = RefArray{T,typeof(x),Any}(x, i, roots)
RefArray(x::AbstractArray{T}, i::Int=1, roots::Nothing=nothing) where {T} = RefArray{T,typeof(x),Nothing}(x, i, nothing)
convert(::Type{Ref{T}}, x::AbstractArray{T}) where {T} = RefArray(x, 1)
Ref(x::AbstractArray, i::Integer=1) = RefArray(x, i)
Ref(x::AbstractArray, i::Integer) = RefArray(x, i)

function unsafe_convert(P::Type{Ptr{T}}, b::RefArray{T}) where T
if isbits(T)
Expand Down

5 comments on commit b57a592

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @ararslan

@StefanKarpinski
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are some pretty gnarly regressions – @rfourquet, are these the ones from your recent PR? @vtjnash, is it possible any of these are caused by this change?

@Sacha0
Copy link
Member

@Sacha0 Sacha0 commented on b57a592 Dec 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least the regressions in

"linalg", "arithmetic", "("\\", "HermitianSparseWithNonZeroPivots", "Vector", 1024)"
"linalg", "arithmetic", "("\\", "HermitianSparseWithNonZeroPivots", "Vector", 256)"

appear real and unrelated to @rfourquet's recent work. Best!

@rfourquet
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are the ones from the "set-like operations" PR which have been analyzed and are not real regressions, but my guess is that the other regressions, concerning essentially BitSet, are the combination of the "BitSet stores any Int" and one "random" PR which changes the generated streams for ranges: indeed, the new BitSet would tend to allocate less initially, by using only as much space as necessary (using the offset field). But then, a symdiff! operation will force to allocate space as new elements are added, when earlier those element would fall in the arleady pre-allocated space. In other words, the performance depend substantially on the specific BitSet used for benchmarks, and those have been changed by the "random" PR.

Please sign in to comment.