Skip to content

Commit

Permalink
Merge pull request #20317 from pabloferz/pz/unique
Browse files Browse the repository at this point in the history
Improve inferability of unique
  • Loading branch information
pabloferz authored Feb 8, 2017
2 parents b8c21cf + 432478d commit e2cceb6
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 5 deletions.
39 changes: 34 additions & 5 deletions base/set.jl
Original file line number Diff line number Diff line change
Expand Up @@ -123,16 +123,45 @@ julia> unique([1; 2; 2; 6])
6
```
"""
function unique(C)
out = Vector{eltype(C)}()
seen = Set{eltype(C)}()
for x in C
function unique(itr)
T = _default_eltype(typeof(itr))
out = Vector{T}()
seen = Set{T}()
i = start(itr)
if done(itr, i)
return out
end
x, i = next(itr, i)
if !isleaftype(T)
S = typeof(x)
return _unique_from(itr, S[x], Set{S}((x,)), i)
end
push!(seen, x)
push!(out, x)
return unique_from(itr, out, seen, i)
end

_unique_from(itr, out, seen, i) = unique_from(itr, out, seen, i)
@inline function unique_from{T}(itr, out::Vector{T}, seen, i)
while !done(itr, i)
x, i = next(itr, i)
S = typeof(x)
if !(S === T || S <: T)
R = typejoin(S, T)
seenR = convert(Set{R}, seen)
outR = convert(Vector{R}, out)
if !in(x, seenR)
push!(seenR, x)
push!(outR, x)
end
return _unique_from(itr, outR, seenR, i)
end
if !in(x, seen)
push!(seen, x)
push!(out, x)
end
end
out
return out
end

"""
Expand Down
3 changes: 3 additions & 0 deletions test/sets.jl
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,9 @@ u = unique([1,1,2])
@test length(u) == 2
@test unique(iseven, [5,1,8,9,3,4,10,7,2,6]) == [5,8]
@test unique(n->n % 3, [5,1,8,9,3,4,10,7,2,6]) == [5,1,9]
# issue 20105
@test @inferred(unique(x for x in 1:1)) == [1]
@test unique(x for x in Any[1,1.0])::Vector{Real} == [1]

# allunique
@test allunique([])
Expand Down

24 comments on commit e2cceb6

@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.

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@tkelman
Copy link
Contributor

@tkelman tkelman commented on e2cceb6 Feb 8, 2017

Choose a reason for hiding this comment

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

Hm. JLD type mismatch?

@jrevels
Copy link
Member

@jrevels jrevels commented on e2cceb6 Feb 8, 2017

Choose a reason for hiding this comment

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

Silly me, I forgot to push all the BaseBenchmarks updates to the nanosoldier branch (i.e. the branch that Nanosoldier pulls down before running the benchmarks).

I just did so, let's see if it works now:

@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 @jrevels

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

And this is what happens when Nanosoldier goes down for a few days..

@tkelman
Copy link
Contributor

@tkelman tkelman commented on e2cceb6 Feb 8, 2017

Choose a reason for hiding this comment

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

should trigger it on several intervening commits comparing against the same baseline, see if we can narrow it down. should open an issue to track this, assuming it isn't caused by deprecations.

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, vs = "@78448fa8cbba354ea7786bfb5ff19f8e17630430")

@jrevels
Copy link
Member

@jrevels jrevels commented on e2cceb6 Feb 8, 2017

Choose a reason for hiding this comment

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

Note that today's daily build report is comparing against a previous build which used a different version of BenchmarkTools, and so should not be taken as necessarily representative of changes in Julia itself (at least without further benchmarking).

For example the new BenchmarkTools includes JuliaCI/BenchmarkTools.jl#30, which could've "slowed down" some benchmarks compared to the old BenchmarkTools.

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against comparison commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Bleh:

ERROR: LoadError: UndefVarError: isabstract not defined
Stacktrace:
 [1] include_from_node1(::String) at ./loading.jl:539
 [2] include(::String) at ./sysimg.jl:14
 [3] anonymous at ./<missing>:2
while loading /home/nanosoldier/.julia/v0.6/Compat/src/Compat.jl, in expression starting on line 1779
ERROR: LoadError: LoadError: LoadError: Failed to precompile Compat to /home/nanosoldier/.julia/lib/v0.6/Compat.ji.

@tkelman
Copy link
Contributor

@tkelman tkelman commented on e2cceb6 Feb 9, 2017

Choose a reason for hiding this comment

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

that isdefined should be checking for isabstract rather than UnionAll then

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

Which one? Is it in compat?

@tkelman
Copy link
Contributor

@tkelman tkelman commented on e2cceb6 Feb 9, 2017

Choose a reason for hiding this comment

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

@tkelman
Copy link
Contributor

@tkelman tkelman commented on e2cceb6 Feb 9, 2017

Choose a reason for hiding this comment

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

try that again with latest compat deployed now @nanosoldier runbenchmarks(ALL, vs = "@78448fa8cbba354ea7786bfb5ff19f8e17630430")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against comparison commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@martinholters
Copy link
Member

Choose a reason for hiding this comment

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

ERROR: LoadError: UndefVarError: TypeConstructor not defined
Stacktrace:
 [1] include_from_node1(::String) at ./loading.jl:539
 [2] include(::String) at ./sysimg.jl:14
 [3] anonymous at ./<missing>:2
while loading /home/nanosoldier/.julia/v0.6/Compat/src/Compat.jl, in expression starting on line 1804
ERROR: LoadError: LoadError: LoadError: Failed to precompile Compat to /home/nanosoldier/.julia/lib/v0.6/Compat.ji.

Not quite there yet...

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

looks like the TypeConstructor line also needs an isdefined check?

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

not sure if latest compat is on nanosoldier but lets try

@nanosoldier runbenchmarks(ALL, vs = "@78448fa8cbba354ea7786bfb5ff19f8e17630430")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against comparison commit: failed process: Process(`sudo cset shield -e su nanosoldier -- -c ./benchscript.sh`, ProcessExited(1)) [1]

Logs and partial data can be found here
cc @jrevels

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

apparently not

@jrevels
Copy link
Member

Choose a reason for hiding this comment

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

Just did a Pkg.update.

@nanosoldier runbenchmarks(ALL, vs = "@78448fa8cbba354ea7786bfb5ff19f8e17630430")

@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 @jrevels

@KristofferC
Copy link
Member

Choose a reason for hiding this comment

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

So that is quite nice. However, the conclusion is then that some SIMD benchmarks got 60 times slower due to the changes in BenchmarkTools? Perhaps they got elided or something in the previous version. I guess running the SIMD benchmarks locally with the two versions of BenchmarkTools would be fruitful.

Please sign in to comment.