Skip to content
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

Avoid unnecessary conversions of nullables. #12424

Merged
merged 1 commit into from
Aug 12, 2015

Conversation

dcjones
Copy link
Contributor

@dcjones dcjones commented Aug 1, 2015

There's currently an unnecessary conversion that happens between nullables of the same type. Adding this def sped up some code I was working by 2x.

Here's a contrived benchmark

type Thing
    x::Nullable{Int}
end

function f()
    a = Thing(1)
    b = Thing(2)
    for _ in 1:100000000
        a.x = b.x
    end
end

f()
@time f()

Before this PR: 0.409873 seconds (155 allocations: 10.543 KB)
After this PR: 0.070780 seconds (155 allocations: 10.543 KB)

@nalimilan
Copy link
Member

Looks like a good idea to me. CC: @johnmyleswhite

@IainNZ
Copy link
Member

IainNZ commented Aug 1, 2015

I'd have thought that

convert{T}(::T, x::T) = x

Was always true, and would have been defined somewhere earlier for some reason. I guess this is actually more like

convert{T,S}(::T{S}, x::T{S}) = x

or something though? 😕

@yuyichao
Copy link
Contributor

yuyichao commented Aug 1, 2015

I think there should be an ambiguity warning for this

julia> f{T}(::Type{T}, x::T) = 1
f (generic function with 1 method)

julia> f{T}(::Type{Nullable{T}}, x::Nullable) = 2
f (generic function with 2 methods)

Only the first one can match f(Int, 1) and only the second one can match f(Nullable{Int}, Nullable{Float64}())

@dcjones
Copy link
Contributor Author

dcjones commented Aug 1, 2015

Yikes, Nullable certainly isn't the only type that affected by this. Rational for example also does a very expensive and unnecessary conversion when copied. I found a few others that likely have the same problem just greping through.

Should I add more convert functions or is there some catchall way to fix this

Edit: Ok so it's not that bad. Non-exhaustively, these types all have this issue: Complex, Rational LDLt, LU, QRPackedQ, QRCompactWYQ, SymTridiagonal,

@johnmyleswhite
Copy link
Member

LGTM

@johnmyleswhite
Copy link
Member

Just to clarify for myself: is the reason we can't have convert{T}(T, x::T) = x that it would be less specific than convert{S, T}(Nullable{S}, x::Nullable{T})? It's actually not obvious to me how our specificity rules work in that case. Nullable{T} is clearly more specific than T, but it seems one could defensibly say that convert{T}(T, x::T) is more specific than convert{S, T}(Nullable{S}, x::Nullable{T}).

@yuyichao
Copy link
Contributor

yuyichao commented Aug 3, 2015

@johnmyleswhite My understanding of the specific/ambigious is that if it is possible to match A without matching B, then A is not more specific than B. If both A and B match something but none of them is more specific than the other, then the two are ambigious.

With this definition, I think there's a missing ambiguity warning here #12424 (comment)

@StefanKarpinski
Copy link
Sponsor Member

There's a bigger problem here, but this certainly seems correct in any case.

StefanKarpinski added a commit that referenced this pull request Aug 12, 2015
Avoid unnecessary conversions of nullables.
@StefanKarpinski StefanKarpinski merged commit 963ab22 into master Aug 12, 2015
@StefanKarpinski StefanKarpinski deleted the dcj/nullableconvert branch August 12, 2015 03:04
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants