RFC: Make inbounds macros expression-like more often

[mbauman]

With #11169 fixed, we can now make @inbounds return its value edit: in common cases.

[KristofferC]

Seems like it could be worth to run the benchmarks? If Nanosoldier is feeling better now that is.

[eschnett]

I would wait for #15548 to ensure that this behaviour doesn't go away.

[mbauman]

ensure that this behaviour doesn't go away.

That's part of the plan. If this gets merged, I don't think there's any going back. :)

Sure, let's do benchmarks… touching these basic utilities can sometimes do surprising things. What are the categories these days? Is SIMD its own category?

[mbauman]

@nanosoldier runbenchmarks("array")

[jrevels]

The error on nanosoldier is

ERROR: LoadError: UndefVarError: A_ldiv_B not defined

@andreasnoack I believe the relevant benchmark is just testing the \ operator...did something change there recently? I searched all of Base on the merge commit the benchmarks ran on for a typo (i.e. A_ldiv_B instead of A_ldiv_B!) but didn't see anything.

[andreasnoack]

I think the error is here. I'm surprised that we don't have coverage for those methods. I'll take a look.

[timholy]

What needs to be done here?

[mbauman]

Is nanosoldier feeling any better these days? @nanosoldier runbenchmarks("array")

Or we can just press merge. I don't foresee this causing trouble.

[nanosoldier]

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

[yuyichao]

Maybe you missed the vs part?

[mbauman]

Doh. I saw it was optional but I figured the default was against master. Let's try that again: @nanosoldier runbenchmarks("array", vs = ":master")

[nanosoldier]

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

[timholy]

Surprising number of performance regressions. Might be fixed by a simple rebase?

[mbauman]

Marking this as request-for-help. This change is very simple and would be very nice to merge, but I'm not well-equipped to debug this sort of regression.

[mbauman]

Aha, I now see #16894 — I bet that's what's going on here.

[mbauman]

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

[tkelman]

I think nanosoldier is still being updated to work properly in a world where master is 0.6-dev. Too bad we lost track of this one, should've given it another go after #16897 was merged.

[jrevels]

Alright, our dear Nanosoldier should be back on its feet:

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

[nanosoldier]

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

[vchuravy]

Should we re-run nanosoldier one last time and merge this, if it comes back OK?

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

[jrevels]

Nanosoldier stopped working through it's job queue for some reason while I was away. Restarted the server:

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

[nanosoldier]

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

[mbauman]

Hm, those regressions look real. I would bet that this is pushing the inlining heuristic over the edge in some cases.

[vchuravy]

Interestingly the daily benchmark showed a bunch of improvements in the bitarray benchmark https://github.com/JuliaCI/BaseBenchmarkReports/blob/4c71955ce4d30d4938b34d5a74abfa64caf74a89/daily_2016_10_12/report.md

I am wondering if it was inline heuristics, then this might have helped.
@nanosoldier runbenchmarks("array", vs = ":master")

[nanosoldier]

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

[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 @ararslan

[mbauman]

Ok, well, I've identified a place where I'm still seeing an extra allocation with the expression-like behavior. See 0f2b7fd for the workaround. I'm having a hard time reducing it down.

[mbauman]

Alright, let's see where we are again here. I may still need to do some work on the inlining cost calculation.

@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 @ararslan

[ararslan]

Looks like there was some internal server error. I restarted the server, so let's try again: @nanosoldier runbenchmarks(ALL, vs=":master")

[ararslan]

Yeah okay something is wrong with Nanosoldier.

[ararslan]

I think Nanosoldier is feeling better now, so @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 @ararslan

[ararslan]

Sorry, still a few more things to fix in Nanosoldier.

[ararslan]

@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 @ararslan

[ararslan]

Sorry for the noise here. Dates moving to the stdlib broke basically the entire Nanosoldier stack and there have still been a couple of things lurking. I think I've finally gotten them all, but we'll see.

@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

[blakejohnson]

Looks like a few real memory regressions?

[mbauman]

Yes, thanks for fixing up Nanosoldier for this, Alex!

It looks like we're still at the same place that stymied me last time — I worked around it before by changing

julia/base/subarray.jl

Line 162 in 690f4bf

@inbounds idxs = to_indices(V.parent, reindex(V, V.indexes, I))

to:

idxs = @inbounds to_indices(V.parent, reindex(V, V.indexes, I))

It looks like the use of an intermediary variable here causes a tuple to be allocated.

[StefanKarpinski]

Let's go ahead and merge this with the workaround now and then once #24113 is merged, verify that the workaround is no longer needed.

[StefanKarpinski]

This seems to be good to go and passes CI. Shall we go ahead and merge this?

[JeffBezanson]

Already merged as #25033