Fix performance of broadcast and collect with Union{T, Missing} #30480

nalimilan · 2018-12-21T12:58:30Z

Use the same pattern as in collect_to! (which is used when size is known). Follows what was done by #25828.

Before:

julia> using BenchmarkTools

julia> v = fill(2.0, 50000);

julia> v2 = vcat(v, missing);

julia> v3 = vcat(missing, v);

julia> @btime $v .* 3.0;
  46.964 μs (2 allocations: 390.70 KiB)

julia> @btime $v2 .* 3.0;
  1.838 ms (99498 allocations: 2.33 MiB)

julia> @btime $v3 .* 3.0;
  3.569 ms (7 allocations: 439.77 KiB)

julia> @btime collect($(x for x in v if true));
  336.170 μs (17 allocations: 1.00 MiB)

julia> @btime collect($(x for x in v2 if true));
  569.995 μs (21 allocations: 2.29 MiB)

julia> @btime collect($(x for x in v3 if true));
  4.041 ms (20 allocations: 1.13 MiB)

After:

julia> @btime $v .* 3.0;
  46.908 μs (2 allocations: 390.70 KiB)

julia> @btime $v2 .* 3.0;
  1.351 ms (99498 allocations: 2.33 MiB)

julia> @btime $v3 .* 3.0;
  102.446 μs (7 allocations: 439.77 KiB)

julia> @btime collect($(x for x in v if true));
  332.648 μs (17 allocations: 1.00 MiB)

julia> @btime collect($(x for x in v2 if true));
  527.674 μs (21 allocations: 2.29 MiB)

julia> @btime collect($(x for x in v3 if true));
  625.985 μs (19 allocations: 1.13 MiB)

Use the same pattern as in collect_to_with_first! (which is used when size is known).

nalimilan · 2018-12-21T13:02:49Z

base/broadcast.jl

@@ -926,13 +926,12 @@ function copyto_nonleaf!(dest, bc::Broadcasted, iter, state, count)
        y === nothing && break
        I, state = y
        @inbounds val = bc[I]
-        S = typeof(val)
-        if S <: T
+        if val isa T || typeof(val) === T


Whether typeof(val) === T is needed is not clear (#30125), but I've added it for consistency with collect_to!. If that's redundant, we should remove all uses at the same time.

Yes, we should remove that from everywhere

nalimilan · 2018-12-21T14:19:16Z

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

nanosoldier · 2018-12-21T18:24:38Z

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

nalimilan · 2018-12-21T18:31:03Z

Nanosoldier confirms this improves performance dramatically with Union{T, Missing}. Most regressions are clearly spurious, there's only one regarding array comprehensions which could be real (but that would be surprising since there's no type instability): is it known to be noisy?

ararslan · 2018-12-21T19:42:30Z

@nanosoldier runbenchmarks("array" && "comprehension", vs=":master")

nanosoldier · 2018-12-21T20:53:39Z

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

ararslan · 2018-12-21T21:15:00Z

Looks like you're all clear.

mbauman · 2018-12-21T21:55:39Z

Am I understanding the magic here correctly? Is the key that we should not assign a type that may be a small discriminated union to a variable?

nalimilan · 2018-12-21T22:04:06Z

Am I understanding the magic here correctly? Is the key that we should not assign a type that may be a small discriminated union to a variable?

That's my conclusion as well, but I have no idea why that's the case.

vtjnash · 2018-12-21T22:46:02Z

<: has almost no optimizations implemented (there aren’t many worth doing) while isa is heavily optimized and usually gets replaced by something very simple

ararslan · 2018-12-23T05:36:47Z

Good to merge?

StefanKarpinski · 2019-01-02T21:46:32Z

Bump. Who's making the decision here?

Most of these conditions were introduced in #25828 and #30480 for some performance reasons atm, but now they seem just unnecessary or even harmful in terms of inferrability. There doesn't seem to be any performance difference in the benchmark used at #25828: ```julia using BenchmarkTools x = rand(Int, 100_000); y = convert(Vector{Union{Int,Missing}}, x); z = copy(y); z[2] = missing; ``` > master: ```julia julia> @Btime map(identity, x); 57.814 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, y); 94.040 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, z); 127.554 μs (5 allocations: 1.62 MiB) julia> @Btime broadcast(x->x, x); 59.248 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, y); 74.693 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, z); 126.262 μs (4 allocations: 1.62 MiB) ``` > this commit: ``` julia> @Btime map(identity, x); 58.668 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, y); 94.013 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, z); 126.600 μs (5 allocations: 1.62 MiB) julia> @Btime broadcast(x->x, x); 57.531 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, y); 69.561 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, z); 125.578 μs (4 allocations: 1.62 MiB) ```

Most of these conditions were introduced in #25828 and #30480 for some performance reasons atm, but now they seem just unnecessary or even harmful in terms of inferrability. There doesn't seem to be any performance difference in the benchmark used at #25828: ```julia using BenchmarkTools x = rand(Int, 100_000); y = convert(Vector{Union{Int,Missing}}, x); z = copy(y); z[2] = missing; ``` > master: ```julia julia> @Btime map(identity, x); 57.814 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, y); 94.040 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, z); 127.554 μs (5 allocations: 1.62 MiB) julia> @Btime broadcast(x->x, x); 59.248 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, y); 74.693 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, z); 126.262 μs (4 allocations: 1.62 MiB) ``` > this commit: ```julia julia> @Btime map(identity, x); 58.668 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, y); 94.013 μs (3 allocations: 781.31 KiB) julia> @Btime map(identity, z); 126.600 μs (5 allocations: 1.62 MiB) julia> @Btime broadcast(x->x, x); 57.531 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, y); 69.561 μs (2 allocations: 781.30 KiB) julia> @Btime broadcast(x->x, z); 125.578 μs (4 allocations: 1.62 MiB) ```

Fix performance of broadcast and collect with Union{T, Missing}

184fbc4

Use the same pattern as in collect_to_with_first! (which is used when size is known).

nalimilan added performance Must go faster missing data Base.missing and related functionality backport pending 1.0 labels Dec 21, 2018

nalimilan mentioned this pull request Dec 21, 2018

Broadcasting with missings is slow #30455

Closed

nalimilan commented Dec 21, 2018

View reviewed changes

KristofferC added Backport pending 1.1.1 and removed backport pending 1.1 labels Dec 23, 2018

nalimilan mentioned this pull request Dec 23, 2018

Add benchmark for collect with Union eltype JuliaCI/BaseBenchmarks.jl#245

Merged

KristofferC mentioned this pull request Dec 30, 2018

Backports for 1.0.4 #30536

Closed

53 tasks

mbauman merged commit 5a8ae94 into master Jan 2, 2019

mbauman deleted the nl/isa branch January 2, 2019 21:48

StefanKarpinski added triage This should be discussed on a triage call backport 1.1 and removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019

StefanKarpinski added triage This should be discussed on a triage call backport 1.0 and removed triage This should be discussed on a triage call backport 1.0 labels Jan 31, 2019

JeffBezanson removed backport 1.0 triage This should be discussed on a triage call labels Jan 31, 2019

cstjean mentioned this pull request Feb 25, 2019

Make broadcast eltype widening type-stable #31174

Merged

aviatesk mentioned this pull request Apr 18, 2022

eliminate uninferrable typeof(x) === T conditions #45018

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix performance of broadcast and collect with Union{T, Missing} #30480

Fix performance of broadcast and collect with Union{T, Missing} #30480

nalimilan commented Dec 21, 2018 •

edited

Loading

nalimilan Dec 21, 2018 •

edited

Loading

vtjnash Dec 21, 2018

nalimilan commented Dec 21, 2018

nanosoldier commented Dec 21, 2018

nalimilan commented Dec 21, 2018

ararslan commented Dec 21, 2018

nanosoldier commented Dec 21, 2018

ararslan commented Dec 21, 2018

mbauman commented Dec 21, 2018

nalimilan commented Dec 21, 2018

vtjnash commented Dec 21, 2018

ararslan commented Dec 23, 2018

StefanKarpinski commented Jan 2, 2019

Fix performance of broadcast and collect with Union{T, Missing} #30480

Fix performance of broadcast and collect with Union{T, Missing} #30480

Conversation

nalimilan commented Dec 21, 2018 • edited Loading

nalimilan Dec 21, 2018 • edited Loading

Choose a reason for hiding this comment

vtjnash Dec 21, 2018

Choose a reason for hiding this comment

nalimilan commented Dec 21, 2018

nanosoldier commented Dec 21, 2018

nalimilan commented Dec 21, 2018

ararslan commented Dec 21, 2018

nanosoldier commented Dec 21, 2018

ararslan commented Dec 21, 2018

mbauman commented Dec 21, 2018

nalimilan commented Dec 21, 2018

vtjnash commented Dec 21, 2018

ararslan commented Dec 23, 2018

StefanKarpinski commented Jan 2, 2019

nalimilan commented Dec 21, 2018 •

edited

Loading

nalimilan Dec 21, 2018 •

edited

Loading