Use broadcast for array operators

[martinholters]

Ref #17254: Motivation is to have the machinery for figuring out the result type available. For the dot-operators, this can be seen as temporary until they are lowered to op.() syntax anyway (if that is still the plan).

Note that this does not fix the issue for other AbstractArray classes providing their own operator implementations.

[nalimilan]

Cool. How is performance? I think that's the main (only?) concern here.

[nalimilan]

Doesn't broadcast handle that check?

[martinholters]

Not quite, broadcast is more liberal: broadcast(+, rand(3,3), rand(3)) works while rand(3,3) + rand(3) throws a DimensionMismatch.

[stevengj]

I'm not sure this PR makes sense at this juncture. I'm worried that it is too late for this to be worthwhile in 0.5, whereas for 0.6 there will be a completely different mechanism: dot operators will be converted by the parser to broadcast calls, so no method should be defined at all.

[martinholters]

Cool. How is performance? I think that's the main (only?) concern here.

I didn't do any thorough benchmarks, just checked adding a 1000×1000 to itself, and that was even slightly faster. Maybe some greater power could ask nanosoldier. Anyway, if this presents a performance penalty, we probably should improve broadcast.

@stevengj I was hoping this could make it into 0.5 (and, yes, be obsoleted in 0.6) to not have people complaining as in #17254 for a whole release cycle. And the change to un-dotted + and - would still make sense past 0.5.

[timholy]

If you fix the conflicts, we can run nanosoldier on this. https://github.com/JuliaCI/Nanosoldier.jl. NVM, saw your comment above.

[martinholters]

Rebased.

[timholy]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[stevengj]

Fair enough, let's see how the performance looks.

[nanosoldier]

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

[martinholters]

Bummer. Interestingly, the regressions are less severe for the larger problem sizes, so probably some constant overhead kicking in. But ["shootout","nbody_vec"] makes me really, really sad...

[timholy]

If you're interested in fixing the performance challenges, https://github.com/JuliaCI/BaseBenchmarks.jl#recipe-for-testing-a-julia-pr-locally has some good tips. I often start out lazily and eventually (e.g., #17137) discover I have to run locally. I find that having two julia builds on the same machine, one master and one PR, helps make this a lot more efficient.

[martinholters]

Changed the non-dot operators to use the old implementation if promote_op gives non-Any and fall back to broadcast otherwise which (at least locally) solves the performance problems.

I've also tried to short-circuit into Broadcast._broadcast! directly using

Broadcast._broadcast!(op, F, (map(i->true, size(A)), map(i->true, size(B))), (A, B), Val{2})

instead of the for-loop. But that had still one more allocation and still showed severe impact on ["shootout","nbody_vec"], which performs many operation on small (3-element) vectors where such an overhead makes all the difference.

Considering the original intention - having a fix for #17254 ready for 0.5 - I guess the current solution is reasonable: Only if promote_op falls back to returning Any, the algorithm is changed to call broadcast to address exactly that issue.

[martinholters]

I'll be ignored if I ask
@nanosoldier runbenchmarks(ALL, vs = ":master")
right? Would someone else be so kind?

[tkelman]

@nanosoldier runbenchmarks(ALL, vs = ":master")

[martinholters]

Err... is there a way to see more of the error message? Is this my (this PR's) fault or nanosoldier's? cc @jrevels

[timholy]

Removal of UTF16String broke JLD which broke BaseBenchmarks which broke nanosoldier. As of a few hours ago everything is fixed in the source code, but I bet something needs to do a Pkg.update().

[jrevels]

Removal of UTF16String broke JLD which broke BaseBenchmarks which broke nanosoldier. As of a few hours ago everything is fixed in the source code, but I bet something needs to do a Pkg.update()

^ This. ref JuliaCI/BenchmarkTools.jl#15. I've just updated JLD on nanosoldier. Let's see if it worked:

@nanosoldier runbenchmarks(ALL, vs = ":master")

[nanosoldier]

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

[timholy]

LGTM. Any objections to merging?

[tkelman]

This is causing test failures on the buildbots (edit: locally too), likely due to an interaction with something else that was merged since travis last ran here: https://build.julialang.org/builders/build_centos7.1-x64/builds/1435/steps/shell_2/logs/stdio

edit2: bandaid in #17394