Don't spawn if: only one iteration or no threads or threading disabled

[IanButterworth]

A single pass of ObjectDetector.jl creates 150 new tasks, all of which were within for loops with length-1 iterators.. i.e. the new task can be entirely avoided.

With this there are 0 spawned tasks.

Avoiding constructing the iterators entirely should be possible too, but I thought I'd open a PR with these changes before looking into that.

PR Checklist

• Tests are added
• Documentation, if applicable

[IanButterworth]

I'm also thinking it would be good to be able to opt out of threading.

[ToucheSir]

An opt-out would be good, but IMO new ones should be done with ScopedValues instead of global toggles. Is https://github.com/vchuravy/ScopedValues.jl a good enough compat package for those?

[IanButterworth]

Sounds good. I think that can be a follow-on PR I did it here

[IanButterworth]

Just noting that I tried simplifying this into a macro

macro maybe_thread(ex)
    ex.head == :for || throw(ArgumentError("Expected a for loop"))
    it, iter = ex.args[1].args
    loop_body = ex.args[2]
    quote
        if should_use_spawn() && length($(esc(iter))) > 1
            Threads.@sync for $(esc(it)) in $(esc(iter))
                Threads.@spawn $(esc(loop_body))
            end
        else
            $(esc(ex))
        end
    end
end

i.e. simply

@maybe_thread for (xc, wc) in zip(x_cs, w_cs)
    x = @view in1[ntuple(i -> i == 4 ? xc : Colon(), 5)...]
    w = @view in2[ntuple(i -> i == 5 ? wc : Colon(), 5)...]
    y = @view out[ntuple(i -> i == 4 ? wc : Colon(), 5)...]
    $(Symbol("$(front_name)_$(backend)!"))(y, x, w, cdims2; kwargs...)
end

but hit some macro hygiene issue with @sync

ERROR: LoadError: syntax: invalid let syntax around task.jl:497
Stacktrace:
 [1] top-level scope
   @ none:1
 [2] eval(m::Module, e::Any)
   @ Core ./boot.jl:430
 [3] top-level scope
   @ ~/Documents/GitHub/NNlib.jl/src/conv.jl:183
 [4] include(mod::Module, _path::String)
   @ Base ./Base.jl:557
 [5] include(x::String)
   @ NNlib ~/Documents/GitHub/NNlib.jl/src/NNlib.jl:1
 [6] top-level scope
   @ ~/Documents/GitHub/NNlib.jl/src/NNlib.jl:80

[ToucheSir]

Would it make sense to have this as a function instead? At first I thought the macro would be better because it doesn't introduce new scope, but on second glance the callback to with is.

My worry would be that users run into closure boxing problems because they're not aware of this scope. At least with a function disallow_spawns() do ... end, that closure piece is explicit?

[IanButterworth]

Good point. I moved to using @with

[ToucheSir]

Another thought: would it make sense to have this be an integer param that controls the max degree of spawn parallelism NNlib can use? Setting that param <= 1 (or to false) would disable spawning as before.

[IanButterworth]

That could make sense. I'll comment that this is internal, so it can be changed when that design is figured out.

[ToucheSir]

Yes, I should've elaborated on this thought. The idea would be to have a way to control the degree of parallelism via a context manager/macro like disallow_spawns, but using a numeric param instead of an on/off flag.

But this design can be fleshed out later. Then there are more QoL things (not for this PR) we can consider around threading that would complement it. For example, deciding the number of tasks to spawn based on # of threads / # of BLAS threads.

[IanButterworth]

Sounds good. Tbh the who julia threading story needs more work on the composability of threading like this. It's not defined how best to orchestrate it from the Base.Threads API. The ecosystem might be better, but adoption isn't strong.

[IanButterworth]

Hard for me to tell if any of these failures are related?

[IanButterworth]

bump @ToucheSir is there anything further to do here?

[ToucheSir]

No, I just hadn't the time to make sure CI was all good and to do another read-through.

Can you bump the version, and then we'll merge + release?