speed up first and only for various types

[matthias314]

This PR is the result of this Discourse thread. It speeds up first for Array and String and only for Dict, IdDict and Set. For only the speed up comes from using length and first instead of calling iterate twice (once to get the first element, once to check that there are no more). I have also deleted dedicated only methods for Tuple, Int and the like because they are not faster than the standard method.

Here are some benchmarks. They were generated by the function

function runtest(v)
    y = only(v[1])
    @show eltype(v)
    @btime count(x -> first(x) == $y, $v)
    @btime count(x -> @inbounds(first(x)) == $y, $v)
    @btime count(x -> only(x) == $y, $v)
    @btime count(x -> @inbounds(only(x)) == $y, $v)
end

applied to a Vector v with n = 1000 randomly generated containers of the given type having one element each. Below I only show the relevant output.

first

type	master	new	@inbounds master	@inbounds new
String	3.145 μs	1.241 μs	3.147 μs	1.368 μs
Vector{Char}	985.571 ns	982.688 ns	961.227 ns	748.953 ns
Matrix{Char}	1.275 μs	1.276 μs	1.277 μs	866.526 ns
Array{Char, 3}	1.273 μs	1.268 μs	1.302 μs	872.368 ns

only

type	master	new	@inbounds master	@inbounds new
Dict{Char, Int64}	16.931 μs	5.940 μs	6.167 μs	5.830 μs
IdDict{Char, Int64}	28.419 μs	18.514 μs	22.029 μs	15.923 μs
Set{Char}	14.810 μs	3.742 μs	5.113 μs	3.561 μs
Tuple{Int64}	71.966 ns	70.243 ns	71.962 ns	70.251 ns
Int64	71.974 ns	71.031 ns	71.961 ns	71.061 ns
Char	102.729 ns	102.744 ns	100.216 ns	100.071 ns

There are a couple of types where the new method only(x, Val(:first)) would be faster the current one (now accessible as only(x, Val(:iterate)), but only when using @inbounds. However, if one adds @inbounds, then one can also switch to first instead of only to get a larger speed-up.

Note that only for IdDict is defined in iterators.jl since it is included later than iddict.jl into Base.jl. Maybe there is a more elegant way of doing it.

[mkitti]

I would rename the versions of only that take a Val to _only. They should not be part of the public only API. This is an implementation detail.

It is unclear to me if removing the methods for Number, Char, Tuple, etc is actually necessary at this stage. Could you justify that further?

I suggest moving the Iterator module methods into iterators.jl. I do not think there is precedent for those existing outside of that file.

[mkitti]

I would move this to iterators.jl

[matthias314]

Dict is defined in dict.jl, which is read after iterators.jl. So it can't be moved there.

[mkitti]

Is it possible to change the order?

[matthias314]

I don't think so, at least not without major changes to Base.jl. There are 24 include statements between iterators.jl on the one hand and dict.jl & set.jl on the other. In between is for example hashing.jl, where hash is defined. Shall we leave the definitions for Dict and Set (whichever form they take for your objective 1) where they are?

[mkitti]

I would rename these versions of only to _only. They should not be part of the public only API. This is an implementation detail.

[matthias314]

I'm asking myself whether these versions should be (semi)public (under the current or a different name). Currently the method is chosen on a case-by-case basis. Package developers and other users may want to do the same.

This also reflects that currently there is no general principle to choose the method. It's tempting to use haslength, but that doesn't always do the right thing:

typeof(y) = String
Base.haslength(typeof(y)) = true
  2.652 μs (0 allocations: 0 bytes)   # runtest with only(y, Val(:iterate))
  3.185 μs (0 allocations: 0 bytes)   # runtest with only(y, Val(:first))
typeof(y) = Vector{Char}
Base.haslength(typeof(y)) = true
  913.744 ns (0 allocations: 0 bytes)
  961.190 ns (0 allocations: 0 bytes)
typeof(y) = BitSet
Base.haslength(typeof(y)) = true
  4.046 μs (0 allocations: 0 bytes)
  5.834 μs (0 allocations: 0 bytes)

Maybe a function hasfastlength in analogy with hasfastin would help. On the other hand, only is probably not important enough to justify major additions.

Any thoughts?

[matthias314]

The new method should also be used for WeakKeyDict and maybe also for PersistentDict and Pairs. For ImmutableDict there is a mysterious allocation that slows it down:

typeof(y) = WeakKeyDict{Vector{Any}, Int64}
Base.haslength(typeof(y)) = true
  425.247 μs (9000 allocations: 203.12 KiB)   # runtest with only(y, Val(:iterate))
  253.297 μs (6000 allocations: 156.25 KiB)   # runtest with only(y, Val(:first))
typeof(y) = Base.PersistentDict{Char, Int64}
Base.haslength(typeof(y)) = true
  1.455 ms (18000 allocations: 562.50 KiB)
  1.203 ms (16000 allocations: 500.00 KiB)
typeof(y) = Base.ImmutableDict{Char, Int64}
Base.haslength(typeof(y)) = true
  1.594 μs (0 allocations: 0 bytes)
  2.715 μs (1 allocation: 32 bytes)
typeof(y) = Base.Pairs{Symbol, Int64, Tuple{Symbol}, @NamedTuple{x::Int64}}
Base.haslength(typeof(y)) = true
  9.510 μs (0 allocations: 0 bytes)
  9.261 μs (0 allocations: 0 bytes)

[mkitti]

As new public API, there are a few issues to work out here and this would require significantly more technical review since we would be committing to this specific interface for quite some time.

Additionally, rather than using Val the typical pattern here for the API would be to create trait types such as Base.HasLength.

Maybe something like as follows:

abstract type OnlyApproach end
struct OnlyIterate <: OnlyApproach end
struct OnlyFirst <: OnlyApproach end

[mkitti]

Rather than using Val what if we used the functions themselves as the second argument.

function only(x, ::typeof(iterate))
   # ...
end

function only(x, ::typeof(first)
    # ...
end

One can then invoke the functions as only(x, iterate) or only(x, first).

[mkitti]

I would rename these versions of only to _only. They should not be part of the public only API. This is an implementation detail.

[mkitti]

Move to iterators.jl

[matthias314]

Again, Set is defined in set.jl, which comes after iterators.jl.

[mkitti]

It is unclear to me if we need to remove all of these. Is there an issue with leaving these here?

[matthias314]

I've removed them to keep the code simple. As far as I can tell, these methods don't add anything because the default method (which is not new) already produces optimal code in these cases. Example:

julia> @code_llvm only(1)   # this PR
; Function Signature: only(Int64)
;  @ iterators.jl:1550 within `only`
define i64 @julia_only_2628(i64 signext %"x::Int64") #0 {
top:
  ret i64 %"x::Int64"
}

[mkitti]

While I recognize that you are trying to simplify the code, performing this simplification while you are trying to advance the other technical points makes the diff complicated. I think it might be good to focus on the additions first, and then make a second pull request for the simplification. This also would aide future debugging if there is an issue.

[mkitti]

While I think you are technically correct, there is too much going on here at once.

At the moment, you are suggesting three distinguishable changes.

1. Acceralate the current only API for particular types.
2. Introduce a new public only API using Val.
3. Simplify and refactor old methods of only.

I suggest focusing this initial pull reqest on objective 1, which might be straightforward enough to advance quickly. If I understand correctly, this part of the pull request should just make existing API faster without a change in behavior. Therefore, it could advance as a pure performance pull request.

The Val usage needs a refactor for public API, but might work well enough for private methods for the moment. This is part probably needs to go to a triage review.

The simplification is fine but I think is fully dividable from this pull request since that should be effectively a no-op.

[LilithHafner]

Adding triage label exclusively for the additions to the public API.

[matthias314]

I have made the following changes:

• rebased the PR on top of master
• renamed the new two-argument version of only to _only
• the second argument to _only now is first or iterate
• added WeakKeyDict and PersistentDict to the types to which the new method applies. The other types are Dict, IdDict and Set.
• change to first for AbstractArray partly undone. It now only adds @propagate_inbounds. (The previous version led to hickups in testing.)

The new methods for only are still defined where the types are defined except for IdDict (which is defined before iterators.jl). Doing it otherwise seems like a massive change to Base.jl for me.

[LilithHafner]

Removing triage label as this PR is now an uncontroversial improvement.

LGTM

[mkitti]

I concur. Can we get a @nanosoldier run?

[LilithHafner]

The string only( does not occur in the BaseBenchmarks repo so this won't be super informative, but first( does occur at least a couple of times

@nanosoldier runbenchmarks("array" || "collection")

[nanosoldier]

Your benchmark job has completed - successfully executed benchmarks. A full report can be found here.

[LilithHafner]

@maleadt, I thought the vs keyword was optional?

[maleadt]

@maleadt, I thought the vs keyword was optional?

Correct, but only for runtests invocations, as the benchmark bot hasn't been updated in ages (i.e. it needs to be updated to Nanosoldier.jl#master).

[LilithHafner]

@nanosoldier runbenchmarks("array" || "collection", vs="master")

[LilithHafner]

@nanosoldier runbenchmarks("array" || "collection", vs=:master)

[LilithHafner]

@nanosoldier runbenchmarks("array" || "collection", vs=":master")

[LilithHafner]

@nanosoldier runbenchmarks("array" || "collection", vs=":master")

lol

[nanosoldier]

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

[mkitti]

["array", "index", ("sumcartesian", "1:100000")] requires some investigation.

[LilithHafner]

That benchmark runs in O(1) time with a low constant factor so I'm guessing it's benchmarking noise.

julia> function perf_sumcartesian(A)
           s = zero(eltype(A))
           for I in CartesianIndices(size(A))
               val = Base.unsafe_getindex(A, I)
               s += val
           end
           return s
       end
perf_sumcartesian (generic function with 1 method)

julia> using Chairmarks

julia> @b 1:100000 perf_sumcartesian
0.466 ns

[LilithHafner]

Thank you for your contribution, @matthias314!