RFC: A path forward on --check-bounds

[Keno]

In 1.9, --check-bounds=no has started causing significant performance regressions (e.g. #50110). This is because we switched a number of functions that used to be @pure to new effects-based infrastructure, which very closely tracks the the legality conditions for concrete evaluation. Unfortunately, disabling bounds checking completely invalidates all prior legality analysis, so the only realistic path we have is to completely disable it.

In general, we are learning that these kinds of global make-things-faster-but-unsafe flags are highly problematic for a language for several reasons:

• Code is written with the assumption of a particular mode being chosen, so it is in general not possible or unsafe to compose libraries (which in a language like julia is a huge problem).

• Unsafe semantics are often harder for the compiler to reason about, causing unexpected performance issues (although the 1.9 --check-bounds=no issues are worse than just disabling concrete eval for things that use bounds checking)

In general, I'd like to remove the --check-bounds= option entirely (#48245), but that proposal has encountered major opposition.

This PR implements an alternative proposal: We introduce a new function Core.should_check_bounds(boundscheck::Bool) = boundscheck. This function is passed the result of Expr(:boundscheck) (which is now purely determined by the inliner based on @inbounds, without regard for the command line flag).

In this proposal, what the command line flag does is simply redefine this function to either true or false (unconditionally) depending on the value of the flag.

Of course, this causes massive amounts of recompilation, but I think this can be addressed by adding logic to loading that loads a pkgimage with appropriate definitions to cure the invalidations. The special logic we have now now to take account of the --check-bounds flag in .ji selection, would be replaced by automatically injecting the special pkgimage as a dependency to every loaded image. This part isn't implemented in this PR, but I think it's reasonable to do.

I think with that, the --check-bounds flag remains functional, while having much more well defined behavior, as it relies on the standard world age mechanisms.

A major benefit of this approach is that it can be scoped appropriately using overlay tables. For exmaple:

julia> using CassetteOverlay

julia> @MethodTable AssumeInboundsTable;

julia> @overlay AssumeInboundsTable Core.should_check_bounds(b::Bool) = false;

julia> assume_inbounds = @overlaypass AssumeInboundsTable

julia> assume_inbounds(f, args...) # f(args...) with bounds checking disabled dynamically

Similar logic applies to GPUCompiler, which already supports overlay tables.

[gbaraldi]

This allows the compiler to run with checkbounds on when --check-bounds=no is set?

[gbaraldi]

Or is it still bad with the global flag but initiates the machinery for a cassete overlay like package?

[Keno]

The compiler no longer has a notion of checkbounds, only the explicit bounds check arguments to the intrinsic, which gets overriden by the new function in Core, so you can mix and match using overlays or world age.

[gbaraldi]

Right, but what if I start julia with --check-bounds = no and it attemps concrete eval? Or does it check the argument to the intrinsic and doesn't do the concrete eval?

[oscardssmith]

This doesn't make --checkbounds=no better, but it allows for users of --checkbounds=no to hopefully move to setting this in their program (which will mean it doesn't affect things like typejoin).

[staticfloat]

I think this can be addressed by adding logic to loading that loads a pkgimage with appropriate definitions to cure the invalidations.

What if we used compile-time preferences for this? E.g. we set a "compile-time" preference for some package (potentially.... Core.Compiler?) that causes Julia to reject the previously-built pkgimages (maybe even the sysimg itself?) because the new set of preferences does not match those baked into the header of the pkgimages.

So concretely, what I'm proposing is that we map a command-line flag to setting a special compile-time preference that is then read from by should_check_bounds(). This automatically takes care of invalidating package images that depend on this compile-time preference, and leaves alone packages that don't dabble with this.

[vchuravy]

What if we used compile-time preferences for this? E.g. we set a "compile-time" preference for some package (potentially.... Core.Compiler?) that causes Julia to reject the previously-built pkgimages (maybe even the sysimg itself?) because the new set of preferences does not match those baked into the header of the pkgimages.

That would generally be a good way forward for these kind of options, but I think independent of this?

I am a bit worried about the caching semantics, the current way of embedding it into the selection flags is a bit of a crutch and compile-time preference may be better, but ideally we would use something akin to multi-versioning to prevent the proliferation of cache files.

To that extend (and for the FMA&co) I have been thinking about the notion of Rusty's Target-Feature flags (https://rust-lang.github.io/rfcs/2045-target-feature.html).
This would be something like conditional dispatch/overlay methods with filters.

A method table would be instantiated with a set of target flags and methods would be made available if they match the set of target flags. If you then could do something like "many set inference" you could create cache files that match a set of combination of Cache-Files. This is of course vastly more complicated than this proposal, but it would open up the potential for Cross-Compilation and dealing with these pesky thinks like has_fma, preferred vector-width,...

[Keno]

Right, but what if I start julia with --check-bounds = no and it attemps concrete eval? Or does it check the argument to the intrinsic and doesn't do the concrete eval?

Yes, having anything other than Const(true) on the boundscheck argument of the intrinsic taints :consistent.

[vchuravy]

Instead of doing this we could have a lib like:

module BaseInbounds

using PrecompileTools

@recompile_invalidations begin
    Core.should_check_bounds(boundscheck::Bool) = true
end

end

and load that here?

That definition does take a while to compile. Loading it is 329 ms and on-disk size is 111M.

Right now this doesn't quite seem to work due to:

┌ BaseInbounds [e8e3d1da-c89d-4456-81ea-eb3240fbf100]
│  WARNING: Method definition should_check_bounds(Bool) in module Core at boot.jl:855 overwritten in module BaseInbounds at /home/vchuravy/src/julia/stdlib/BaseInbounds/src/BaseInbounds.jl:6.
│    ** incremental compilation may be fatally broken for this module **
└ 

julia> using BaseInbounds

julia> f(A) = A[100]
f (generic function with 1 method)

julia> @code_typed f([1])
CodeInfo(
1 ─ %1 = Base.arrayref(true, A, 100)::Int64
└──      return %1
) => Int64

[Keno]

Instead of doing this we could have a lib like:

Yes, I mentioned that in the original post, but it needs some special loading logic

[vchuravy]

So I quite like this as the first step towards a full solution. In particular it would also allow us to naturally implement @cuda inline=no using overlaying-overlay tables.

I think my biggest worry is the caching implication.

Right-now we don't provide a sysimg that was compiled with --check-bounds=no and ignore that request for any code in the sysimage (I guess that's a bug, but who is counting). So the redefinition would invalidate large swaths of Base right now. I guess the compiler etc. are guarded from that invalidation.

Using this branch as-is just starting the REPL takes quite a bit longer.

Right now we are in a situation where users of --check-bounds=no pay for it themselves using disk-space (since it splits the pkg cache fully), so would we ship "BaseInbounds" as part of Julia?

[JeffBezanson]

Triage thinks this is definitely moving in the right direction. The caching situation still needs to be decided on though.

[Keno]

Caching now implemented. Before caching:
Before caching:

keno@deepsea3:~/julia$ time ./julia --check-bounds=no -e 'exit()'

real	0m2.617s
user	0m2.967s
sys	0m1.589s

After caching:

time ./julia --check-bounds=no -e 'exit()'

real	0m0.635s
user	0m0.647s
sys	0m0.670s

REPL feels snappy also.

[JeffBezanson]

Suggested change

	JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype, int nowarn_overwrite)
	JL_DLLEXPORT void jl_method_table_insert(jl_methtable_t *mt, jl_method_t *method, jl_tupletype_t *simpletype, int warn_overwrite)

[vchuravy]

Suggested change

	by users in creating "Startup" packages to ensure that the code compiled by package authors is not invalidated.
	by users in creating "Startup" packages to ensure that the code compiled by package authors is not invalidated.
	!!! note
	Users should use `PrecompileTools.jl`. This macro is solely for internal usage in the Julia bootstrapping process.

[vchuravy]

Is this needed since the --check-bounds=no stdlib is a "virtual dependency"?
Could we avoid duplicating this logic by having the load happen in Julia proper?

I am not fully sure why the __precompile__(:mandatory) is needed, so expanding comments throughout the code would be helpful.

[Keno]

I don't understand the question.

[vchuravy]

Suggested change

	# OOICCDDP - see jl_cache_flags
	# OOIOCDDP - see jl_cache_flags

[vchuravy]

Suggested change

	# Check whether this is in the current list of mandatory packages
	for (mandatory_modkey, mandatory_build_id) in _mandatory_dependencies
	if mandatory_modkey == modkey && mandatory_build_id == build_id
	@goto mandatory_ok
	end
	end
	@debug "Rejecting cache file $cachefile for $modkey since it is a mandatory package that we have not loaded"
	return true
	@label mandatory_ok
	end
	# Check whether this is in the current list of mandatory packages
	found = false
	for (mandatory_modkey, mandatory_build_id) in _mandatory_dependencies
	if mandatory_modkey == modkey && mandatory_build_id == build_id
	found = true
	break
	end
	end
	if !found
	@debug "Rejecting cache file $cachefile for $modkey since it is a mandatory package that we have not loaded"
	return true
	end
	end

Since no other piece of this code uses gotos

[vchuravy]

Suggested change

	// OOICCDDP
	// OOIOCDDP

[vchuravy]

The name is almost to cute. Maybe simply: CheckBoundsNo?

One other thought I just had is that it is kinda weird to use "distinct" packages for this kind of options settings.
Could we instead use Preferences? That already interacts with the caching subsystem.

cc: @staticfloat

[Keno]

There needs to be a cache file that stores the curing definitions. Preferences don't (currently) have any mechanism that has identity for a cache file.

[vchuravy]

So I was thinking that we would have one stdlib instead of two.

module CheckBounds

const my_uuid = #
Base.record_compiletime_preference(my_uuid, "check-bounds")
let d = Base.get_preferences(my_uuid)
    global const check_bounds = d["check-bounds"]
end

@Base.recompile_invalidations begin
    Core.should_check_bounds(boundscheck::Bool) = check_bounds
end

Compiletime preferences split the cache and you get the same curing effect.

[Keno]

I think that would be fine, but does seem somewhat more complicated. Are there any other advantage of using Preferences that you're imagining that I'm not seeing. Also the precompile(:mandatory) is still required even with this setup.

[vchuravy]

Elliot and I were discussing that we might want to add Preferences usage for Julia instead of pushing more and more stuff into command line flags.

So while the user could use julia --check-bounds=no, they could also use their local LocalPreferences.toml to set

[CheckBounds]
check-bounds=no

I kinda like that from a user interface perspective.

Can you explain why the precompile(:mandatory) a bit more? I am not sure what problem it solves.

[Keno]

Can you explain why the precompile(:mandatory) a bit more? I am not sure what problem it solves.

Overwriting methods is disallowed in packages because we don't semantically impose an order on them. The :mandatory annotation requires that all future package loads implicitly gain the package as a dependency, so that there is a well-defined ordering.

[vchuravy]

So what #50578 turned into an explicit error. Gotcha

[vchuravy]

Suggested change

	# OOICCDDP
	# OOIOCDDP

[Keno]

I would propose sitting down at the hackathon tomorrow and talking this through. I think we can just knock this out and get this all behind us.

[Keno]

As discussed during the hackathon.

1. We are abandoning this PR as is for --check-bounds purposes. The method overwriting implementation will still be used for checked arithmetic, which may re-use pieces of this PR, but probably not the PR as is.
2. We will be accelerating work on RFC: Effect Preconditions - or - the future of @inbounds #50641 and using the effect assumptions infrastructure to implement --check-bounds.
3. We are fine with using different mechanisms for bounds checking and checked arithmetic. We have some partial justification for this from out of bounds indexing being undefined behavior, but checked arithmetic violations not being undefined behavior.
4. Checked arithmetic could potentially also use effect assumptions bits, but we don't think it's worth it.
5. For system images and pkgimages, we will be caching both --check-bounds=yes and default options to address the issue we have right now where --check-bounds=yes requires a full recompilation.
6. We did not reach a conclusion for caching --check-bounds=no, but it's possible to build a custom system image to do this.