Minor Array constructor tweaks

[mbauman]

This removes an unnecessary tuple allocation when constructing 1-3 dimensional arrays with a single tuple argument. Notably, this is the form that ends up getting called by similar. Before:

julia> @benchmark Array{Int}(1,1,1)
================ Benchmark Results ========================
     Time per evaluation: 67.50 ns [66.48 ns, 68.52 ns]
Proportion of time in GC: 2.00% [1.45%, 2.56%]
        Memory allocated: 80.00 bytes
   Number of allocations: 1 allocations

julia> @benchmark Array{Int}((1,1,1))
================ Benchmark Results ========================
     Time per evaluation: 132.95 ns [129.50 ns, 136.39 ns]
Proportion of time in GC: 1.96% [1.35%, 2.58%]
        Memory allocated: 112.00 bytes
   Number of allocations: 2 allocations

After:

julia> @benchmark Array{Int}(1,1,1)
================ Benchmark Results ========================
     Time per evaluation: 62.19 ns [61.18 ns, 63.20 ns]
Proportion of time in GC: 2.20% [1.62%, 2.78%]
        Memory allocated: 80.00 bytes
   Number of allocations: 1 allocations

julia> @benchmark Array{Int}((1,1,1))
================ Benchmark Results ========================
     Time per evaluation: 62.44 ns [61.45 ns, 63.43 ns]
Proportion of time in GC: 2.23% [1.65%, 2.82%]
        Memory allocated: 80.00 bytes
   Number of allocations: 1 allocations

While I was there, I also cleaned up the constructor functions themselves a bit, such that the fully-parametrized Array{T,N} was the final endpoint. It makes things simpler and a bit more consistent; before constructs like Array{Int,3}(1,2,3) and Matrix{Int}((1,2)) were missing method errors… but you might expect them to work.

[yuyichao]

Ref #14201

[mbauman]

Hah, thanks, I hadn't seen that. I'd prefer to not implement the 0-argument forms… in fact I almost deprecated them. But I think that should be done separately.

[yuyichao]

I don't really like the 0-argument form either but it seems that this PR basically implements what I mentioned in #14201 (comment) ?

[mbauman]

Yes, the second commit here enables all permutations of (Array{T}, Array{T,N}) with (::Integer..., and ::Tuple{Vararg{Integer}}) so long as dimensionalities match.

There are still two flavors of constructor that are only supported for Vector and Matrix:

• Vector{T}() and Matrix(): forms a 0 or 0x0 array.
• Vector(1) and Matrix(1,1): are Any arrays. These are only really callable with a typealias, but they could be generalized as Array(1,1). Not sure about the utility there, though...

And then there's the old Array(::Type, dims) constructors that predate callable types.

[JeffBezanson]

Seems ok to me, but I don't fully understand the efficiency issue. Where was the extra tuple getting allocated? The new definitions seem to always need tuples. One hopes they'll usually be inlined away, but this seems to increase the chances a tuple will be allocated.

[quinnj]

@mbauman, @jrevels, good PR candidate to test the new Base benchmarking capabilities?

[jrevels]

runbenchmarks("arrays", vs = "JuliaLang/julia:master")

[mbauman]

Whoa, amazing work @jrevels! 😲 💯

https://github.com/JuliaCI/BaseBenchmarkReports/blob/master/f369166/f369166_vs_01df9b1.md

It looks like it's comparing my 18-day old branch to the current master tip. Is that the case? Would it re-run if I rebase? Or is there a way to run the benchmarks between master and the result of merging this branch?

[tkelman]

Yeah PR's should probably be fetching the merge commit rather than the branch. See the first few lines of what Travis and AppVeyor do.

[timholy]

In the meantime, you could also compare against the parent commit (b33c4bd).

[KristofferC]

That's some seriously cool stuff @jrevels!

[jrevels]

Would it re-run if I rebase?

You'd have to manually submit a new job via a new comment. It would be cool to be able to re-trigger jobs without adding noisey comments, though I'd still want it to be a manual resubmission...

In the meantime, you could also compare against the parent commit (b33c4bd).

runbenchmarks("arrays", vs = "b33c4bd182ba897bdf3fffd68bc855e5b7c8229e")

PR's should probably be fetching the merge commit rather than the branch. See the first few lines of what Travis and AppVeyor do.

This sounds like a good idea! Can you point me to the script(s) you're mentioning here? I'm assuming it just does a merge in the local clone and builds from that?

[tkelman]

Just the first couple lines of the build log:

git fetch -q origin +refs/pull/14441/merge:
git checkout -qf FETCH_HEAD

[jrevels]

Ah, that makes sense, thanks!

[jrevels]

Just deployed the change that @tkelman suggested; let's try this again, shall we?

runbenchmarks("arrays", vs = "JuliaLang/julia:master")

We don't yet have constructor benchmarks in BaseBenchmarks.jl, so in theory the report should show that this PR's performance matches master's.

[mbauman]

Looks like this approach won't work with the array constructors now appearing so early in bootstrap (getindex isn't defined yet, so we can't reference tuple elements). I'm going to close this; I don't particularly feel like fighting my way through the bootstrap issues to update this.