don't capture arrays/tuples in BoundsError
#43738
Conversation
aviatesk
force-pushed
the
avi/boundserror
branch
from
55fc1ea to
4929b0a
Compare
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.
aviatesk
force-pushed
the
avi/boundserror
branch
from
4929b0a to
20a3e85
Compare
| @@ -139,11 +139,19 @@ JL_DLLEXPORT void JL_NORETURN jl_atomic_error(char *str) // == jl_exceptionf(jl_ | |||
| jl_throw(jl_new_struct(jl_atomicerror_type, msg)); | |||
| } | |||
|
|
|||
| void JL_NORETURN jl_throw_bounds_error(jl_value_t *v, jl_value_t *t) | |||
There was a problem hiding this comment.
Does this code have a special meaning in the context of the other changes, or is this just a refactor?
|
LGTM, and iirc @JeffBezanson pitched something along these lines when we last discussed this topic. If this looks good to everyone, this should basically be the last requirement to merge #42465 (modulo some more tests / minor changes due to reviews) |
| # these `invoke_in_world`s here essentially prohibits users from changing | ||
| # the semantics of `BoundsError` by overriding `summary(a)` implementations | ||
| # (and potentially allowing `a` to be escaped to somewhere) | ||
| desc = Base.invoke_in_world(Base._BASE_DEFINED_AGE, Base.summary, a)::String |
There was a problem hiding this comment.
I am a bit worried about that. When I joked offline that we "supported" this, @JeffBezanson was "We do not!".
I think a better approach is to define a Core.throw_bounds_error that is defined in the inference world-age.
and have jl_throw_bounds_error invoke in that world-age.
There was a problem hiding this comment.
I don't think we really need to (or should) call summary here; saving the size and type should be enough.
To handle calling size, we could define constructors here only for Array and Tuple, and add a constructor in Base for other types so it can call Base.size. (I hope to have a better general solution for this one day, but for now this is how it is.)
|
From triage: let's change it to capture the type and axes. For |
|
This is on top of my list to sort out once I can focus on #42465 again |
|
Do we need to (or should we) think about TypeError (typeassert) also? |
|
@vtjnash Are you referring to potential escape through the The last I talked to Jeff, I think the idea for this PR was trying to mostly handle potential |
|
Yeah, but I suppose this is sufficient for now. I think UndefRefError is another candidate where we eventually want to capture something about the array itself too, to provide better context in the error message, but which conflicts with the intent of this PR? |
|
We could take a similar approach with I don't think bundling |
oscardssmith
added
triage
|
We should to get this rebased and merged. It will be very nice once we get #55913 |
|
Bump, but I don't think we need triage to talk about this. |
This commit changes the semantics of
BoundsErrorso that it doesn'tcapture arrays/tuples passed to its constructor.
The change wouldn't improve any performance by itself because the
BoundsErrorconstructor is mostly called on error paths and thusit is opaque to analyzers/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
ImmutableArrayconstruction and stack allocation ofArrays, byallowing us to assume the invariant that primitive indexing operations
into array/tuple don't escape it so that our escape analyses
can achieve further accuracy.
Specifically,
BoundsErrorconstructor will now compute the "summary"of its
Array/Tupleargument, rather than capturing it for the laterinspection. Assuming
Base.summary(x::Array)orBase.summary(x::Tuple)don't escape
x, we can assume thatBoundsErrordoesn't escapex.This change won't appear as breaking in most cases since
showerrorwillprint 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
BoundsErrorin most cases.The 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.