Warn @inbounds @threads for pattern etc. during macro expansion

[tkf]

I've seen users trying @inbounds @threads for and @inbounds @spawn for their attempt to improve performance [1]. Since @inbounds does not work across function boundaries, these annotations are useless and people can be confused by the performance of the code. So, it is rather tempting to just warn the users while expanding @inbounds which is much more efficient than keep paying attention in discourse/slack/zulip. What do people think about this kind of approach? Would it be too noisy?

Here's an example output:

(with https://github.com/JuliaLogging/TerminalLoggers.jl)

[1] Examples:

• https://discourse.julialang.org/t/what-is-the-cause-of-this-performance-difference-between-julia-and-cython/58886/4
• https://discourse.julialang.org/t/i-just-decided-to-migrate-from-python-fortran-to-julia-as-julia-was-faster-in-my-test/61188/11
• https://discourse.julialang.org/t/julia-beginner-from-python-numba-outperforms-julia-in-rewrite-any-tips-to-improve-performance/66414/2

[vchuravy]

I like this approach. But maybe triage should have a chat about it, since I don't think we do something like this anywhere else

[oscardssmith]

Given that we are able to detect this specific scenario, could we instead make the macro do what the user intended and propagate the @inbounds? To me it seems kind of mean for the macro to be smart enough to see the problem, but not smart enough to fix it for you.

[tkf]

The implementation in this PR is not enough to reliably support @inbounds propagation. For example, this PR doesn't handle cases like

@inbounds begin @threads for x in xs; end; end

@inbounds begin
    @threads for x in xs; end
    @threads for x in xs; end
end

@inbounds for y in ys
    @threads for x in xs; end
end

Warning can have false-negative and I think it's OK as long as it helps people understand how @inbounds interact with closures.

I think tweaking @inbounds ... @threads to propagate @inbounds is not super hard. We can capture Expr(:boundscheck) while creating the closure and "reload" it inside the closure:

macro inbounds_capturing_thunk(ex)
    quote
        checkbounds = true
        @boundscheck checkbounds = false
        if checkbounds
            thunk_checkbounds() = $(esc(ex))
        else
            thunk_nocheckbounds() = @inbounds $(esc(ex))
        end
    end
end

(It can be simplified if we can put Expr(:inbounds, _) inside a conditional. But I don't know if that's the case.)

Having said that, I'm against such a change. I think Julia programmers need to know how @inbounds interacts with closures. It's hard to expect all macros built on top of closure to support inbounds-capturing. Besides, I believe it's a really bad idea to put @inbounds around the code you don't write. The scope of @inbounds should be as minimal as possible.

[JeffBezanson]

I really do think inbounds would be lexically-scoped in a perfect world, so these cases should just work. But changing that can cause more things to become declared inbounds than before, which could introduce nasty errors. Perhaps those cases are either (1) rare, or (2) the programmer intended to declare everything inbounds anyway?

[tkf]

I think it'd be ideal if @inbounds only applies to what user "sees" in the sense that it is not applied to the code generated by the macros. This is stricter than lexical scoping.

(Perhaps @inbounds information can be provided as a dynamically scoped flag during macro expansion time and so each macro can cooperatively opt-in @inbounds application. The propagation is automated by dynamic scoping. But it has subtle problems since a macro can call macros with the expression that the user didn't provide.)

That said, looking at the current documentation

julia/base/essentials.jl

Lines 541 to 543 in c5f3487

	@inbounds(blk)
	Eliminates array bounds checking within expressions.

it's probably most natural to interpret that @inbounds is lexically scoped. So, maybe changing @inbounds to cross closure boundaries can even be considered technically a "bug fix"?

If we consider that @inbounds should be lexically scoped, I think it'd be nice to warn it in the docstring that the current limitation of @inbounds may not apply in the future and so misuse of @inbounds can break a program when updating Julia.

[codecov]

Codecov Report

Merging #41893 (c5f3487) into master (8cad32b) will increase coverage by 11.12%.
The diff coverage is 89.07%.

❗ Current head c5f3487 differs from pull request most recent head 5d6565a. Consider uploading reports for the commit 5d6565a to get more accurate results

@@             Coverage Diff             @@
##           master   #41893       +/-   ##
===========================================
+ Coverage   78.04%   89.17%   +11.12%     
===========================================
  Files         351      343        -8     
  Lines       74091    79363     +5272     
===========================================
+ Hits        57824    70771    +12947     
+ Misses      16267     8592     -7675     

Impacted Files	Coverage Δ
base/atomics.jl	84.61% <ø> (+0.61%)	⬆️
base/broadcast.jl	86.91% <ø> (+8.23%)	⬆️
base/channels.jl	95.85% <0.00%> (-0.47%)	⬇️
base/compiler/compiler.jl	25.00% <0.00%> (ø)
base/compiler/types.jl	66.66% <ø> (+4.59%)	⬆️
base/deprecated.jl	72.94% <ø> (+10.74%)	⬆️
base/dict.jl	97.94% <0.00%> (+0.40%)	⬆️
base/experimental.jl	82.27% <0.00%> (+19.61%)	⬆️
base/file.jl	81.86% <ø> (+16.60%)	⬆️
base/gmp.jl	86.28% <0.00%> (+2.24%)	⬆️
... and 415 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8cad32b...5d6565a. Read the comment docs.

[tkf]

We want to @inbounds to be recursive. So closing this PR.