Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Non-capturing BoundsError API #44439

Closed
wants to merge 1 commit into from

Conversation

ianatol
Copy link
Member

@ianatol ianatol commented Mar 4, 2022

This PR rebases #43738 and changes the API for BoundsError a bit.

We export throw_boundserror from Base, which by default routes to the same BoundsError constructor that we know and love. However, we also implement a new BoundsError constructor that accepts axes (or whatever equivalent notion of size) and a type rather than an object.

Therefore, by defining throw_boundserror(t::Tuple,...), throw_boundserror(s::AbstractString,...) to give just the size and type info to the new BoundsError constructor, we avoid capture of these objects, opening us up to be a bit less conservative about ThrownEscape in #43800. Package developers can also add their own implementation of throw_boundserror to prevent their objects from being captured by BoundsError and give more accurate size/type in error messages for their custom types.

We also define a new function, jl_bounds_error_summarized, which replaces jl_bounds_error in jl_get_nth_field and similarly avoids the capture of structs via getfield/getproperty.

Note: for Array, we call Base.axes from inside of the normal BoundsError constructor, instead of defining a throw_boundserror(a::AbstractArray,...), per #43738 (comment).

Co-authored by Shuhei Kadowaki (@aviatesk)

@ianatol ianatol marked this pull request as draft March 4, 2022 08:03
This commit changes the semantics of `BoundsError` so that it doesn't
capture arrays/tuples passed to its constructor.

The change wouldn't improve any performance by itself because the
`BoundsError` constructor is mostly called on error paths and thus
it is opaque to analyses/optimizers when compiling a caller context.
Rather it's supposed to be a building block for broadening the possibility
of certain memory optimizations in the future such as copy elision for
`ImmutableArray` construction and stack allocation of `Array`s, by
allowing us to assume the invariant that primitive indexing operations
into array/tuple indexing don't escape it so that our escape analyses
can achieve further accuracy.

Specifically, when `BoundsError` constructor will now compute the "summary"
of its `Array`/`Tuple` argument, rather than capturing it for the later
inspection. Assuming `Base.summary(x::Array)` or `Base.summary(x::Tuple)`
don't escape `x`, we can assume that `BoundsError` doesn't escape `x`.
This change won't appear as breaking in most cases since `showerror` will
print the exact same error message as before, but obviously this is
technically breaking since we can no longer access to the original
arrays/tuples by catching `BoundsError`.
I'd say this breaking semantic change would be still acceptable, since
I think it's enough if we can know size/type of arrays/tuples from
`BoundsError` in most cases.

As a last note, I didn't change the semantics for arbitrary user objects,
since we still don't have a good infrastructure to tell the compiler some
primitive assumptions/rules about user type objects anyway.
@vchuravy
Copy link
Member

vchuravy commented Mar 4, 2022

A great package to test this out on as well is StaticArrays.jl in particular MArray

@ianatol
Copy link
Member Author

ianatol commented Mar 4, 2022

Hmm yeah. It seems to work fine with MArray, but has some problems with SArray. What I don't like about this approach right now is it's a bit brittle in that I basically recreate parts of summary in showerror. It works fine for Array and Tuple, but certainly would have problems with some of the various AbstractArrays. Maybe a more general approach would be to just call summary at the thrower and pass a string to the BoundsError? Since we already know summary works nicely for a variety of types.

@vchuravy
Copy link
Member

vchuravy commented Mar 4, 2022

That wouldn't be great. Part of the goal (as I understand it) is to avoid having every throw site depend on the string handling code. As an example we can produce bounds errors on the GPU just find, but creating a string would not work.

@ianatol
Copy link
Member Author

ianatol commented Mar 4, 2022

Gotcha. I suppose for now we can limit the scope of this to Array and Tuple, like the original #43738, and just have other types use the normal BoundsError behavior, but that doesn't give us much in terms of what we can assume in EA 😵‍💫

@vtjnash
Copy link
Member

vtjnash commented Mar 4, 2022

The summary code also might throw a bounds error, and throw the analysis into an even worse state than before this change

@ianatol
Copy link
Member Author

ianatol commented Mar 4, 2022

Right, so I guess we do this "kinda summary" in showerror just for some select types, which we can then assume no ThrownEscapes for, and let types we don't support that way use the normal escaping constructor that actually calls summary (basically this PR as is). It feels pretty brittle, but I guess it's a start?

@vtjnash vtjnash closed this Jan 30, 2024
@vtjnash
Copy link
Member

vtjnash commented Jan 30, 2024

Just doing some old PR cleanup

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants