Try using "native" boundschecking for Array

[mbauman]

Just a trial balloon. The LLVM looks pretty good, but it's semantically different as it uses & instead of && for each dimension. Let's try this. @nanosoldier runbenchmarks(ALL, vs = ":master")

Fixes #20469.

[nanosoldier]

Something went wrong when running your job:

NanosoldierError: failed to run benchmarks against primary commit: ErrorException("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

[andreasnoack]

@jrevels Do you know why the soldier is still giving the blas_set_num_threads error?

[jrevels]

Ref here, I'm not deploying the changes/doing all the test runs until Nanosoldier supports a serialization compatibility layer to prevent JLD breakage with the current BenchmarkTools.

[vchuravy]

Trial by fire it is ;) JuliaCI/Nanosoldier.jl#28 (comment) @nanosoldier runbenchmarks(ALL, vs = ":master")

[nanosoldier]

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

[nalimilan]

Just curious, why is the 1-argument definition needed?

[mbauman]

It's historical. Win32 had trouble with this at some point. It can probably be simplified, but I'm not inclined to mess with it unless there's a good reason to change it.

[jrevels]

ref my comment here:

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, vs = ":master")

[nanosoldier]

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

[jrevels]

Argh, forgot to update FileIO on one of the nodes. Just did so. Third time's a charm?

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

[nanosoldier]

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

[mbauman]

Thanks @jrevels! Let's try short-circuiting.

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

[nanosoldier]

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

[mbauman]

Bummer. That's better, but still terrible. The first few benchmarks I looked at were slower because getindex now contains more Julia code… which can push an enclosing function over the inlining boundary. The generated LLVM of getindex itself, though, is nearly identical. And micro benchmarks of individual calls are performing identically.

[mbauman]

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

[nanosoldier]

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

[vtjnash]

This seems like a good plan: it should make switching to a Julia-based implementation of multi-dimensional arrays easier. I might request keeping the code around for doing codegen-based bounds checking, as it can come in useful when debugging inference / codegen errors. But it's OK if that's a slower, coarser memory range validation.

[mbauman]

Any advice or help on the remaining regressions here? I've gotten this as close as I can for now.

[mbauman]

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

[mbauman]

Hm, Nanosoldier seems to have missed the prod. @nanosoldier runbenchmarks(ALL, vs = ":master")

[nanosoldier]

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

[vtjnash]

I expect (most of?) those regressions should be fixed now that #22826 is merged.

[ararslan]

Only one way to find out. :) @nanosoldier runbenchmarks(ALL, vs=":master")

Edit: Except there are merge conflicts so this won't be informative. Sorry! Need conflicts resolved before Nanosoldier can use the merge commit.

[nanosoldier]

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

[StefanKarpinski]

Regressions are real, but they're for the old version – this needs to have conflicts resolved or be merged and then have nanosoldier retriggered before we can find out if it's ok now.

[mbauman]

This isn't nearly as important as it was before Jameson's #22826. That PR solved the real problem in the disparity between the Julia and C bounds check removal. See #20469 (comment) for an example; #23516 adds that as a test.

Moving more things from C to Julia is nice, and dogfooding boundscheck/checkbounds in Array is also good, but now that the bug is fixed it would be unobservable. So this PR will go to the bottom of my list now.

[vtjnash]

Closing, since we fixed the problem this was attempting to address