Taal: Should we use @inbounds + @boundscheck to increase standard performance?

[sloede]

I think we should have a discussion about our way of boundschecking in general. There are at least two possible ways:

1. Use more @boundscheck blocks and @inbounds afterwards to make the assumptions on bounds explicit and the code fast even if the users start Julia with default arguments. While debugging some code, we can start Julia with the flag --check-bounds=yes to override @inbounds.
2. Do not use @boundscheck or @inbounds at all. By default, Julia will check bounds on every access. Hence, users need to start Julia with --check-bounds=no (or --check-bounds=0 in @sloede's custom build 😉) - our status quo.

Personally, I favor the first option.

Originally posted by @ranocha in #203 (comment)

[ranocha]

Maybe we can use a hybrid way for now: If we want to introduce additional bounds checks, we hide them inside @boundscheck to eliminate them when using --check-bounds=no. What I have in mind is something like

Trixi.jl/src/amr/amr.jl

Lines 22 to 23 in 2c670e2

	@assert length(lambda) == length(leaf_cell_ids) ("Indicator and leaf cell arrays have " *
	"different length")

Trixi.jl/src/solvers/dg/2d/amr.jl

Lines 59 to 62 in 2c670e2

	if isperiodic(mesh.tree) && n_l2mortars == 0 && n_ecmortars == 0
	@assert n_interfaces == 2*n_elements ("For 2D and periodic domains and conforming elements, "
	* "n_surf must be the same as 2*n_elem")
	end

We don't use @inbounds for now, though.

@boundscheck tells Julia that the following code performs some bounds checks for security, see https://docs.julialang.org/en/v1/devdocs/boundscheck/. It doesn't perform a check like @assert. It's purpose is solely to mark some code as to be removed when bounds checking is disabled, e.g. by using --check-bounds=no. Hence, I don't want to convert @assert stuff to @boundscheck stuff but to @boundscheck @assert stuff.

[ranocha]

Result: Use @boundscheck for things like

Trixi.jl/src/amr/amr.jl

Lines 22 to 23 in 2c670e2

	@assert length(lambda) == length(leaf_cell_ids) ("Indicator and leaf cell arrays have " *
	"different length")

but not for sanity checks such as

Trixi.jl/src/solvers/dg/2d/amr.jl

Lines 59 to 62 in 2c670e2

	if isperiodic(mesh.tree) && n_l2mortars == 0 && n_ecmortars == 0
	@assert n_interfaces == 2*n_elements ("For 2D and periodic domains and conforming elements, "
	* "n_surf must be the same as 2*n_elem")
	end

Keep the status quo of requiring julia --check-bounds=no for optimal performance.

[sloede]

From my perspective, the first example is just on the edge between being a bounds check and a logic check. However, an open-and-shut example for @inbounds to me is the following:

Trixi.jl/src/callbacks/amr_dg2d.jl

Lines 97 to 108 in 5120d7b

	@boundscheck begin
	@assert old_element_id >= 1
	@assert size(old_u, 1) == nvariables(equations)
	@assert size(old_u, 2) == nnodes(dg)
	@assert size(old_u, 3) == nnodes(dg)
	@assert size(old_u, 4) >= old_element_id
	@assert element_id >= 1
	@assert size( u, 1) == nvariables(equations)
	@assert size( u, 2) == nnodes(dg)
	@assert size( u, 3) == nnodes(dg)
	@assert size( u, 4) >= element_id + 3
	end

[ranocha]

Just in case we decide to change the default behavior in the future: Stuff like

@inline function multiply_add_to_node_vars!(...)

should be

Base.@propagate_inbounds function multiply_add_to_node_vars!(...)

[ranocha]

We might need to revisit this based on the outcome of JuliaLang/julia#48245