-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Reduce size of InterpErrorInfo to 8 bytes #82116
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -356,7 +356,7 @@ where | |
// an allocation, which we should avoid. When that happens, | ||
// dedicated error variants should be introduced instead. | ||
assert!( | ||
!error.kind.allocates(), | ||
!error.kind().allocates(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Doesn't this stop making any sense now? (Btw I'd appreciate a Cc for changes like this that affect the Miri/CTFE engine.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unboxing of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If that means it didn't make any sense before, that's okay and we should still remove it. ;) But I feel like the difference between 0 and 1 allocation is much bigger than between 1 and 2, so I am not sure this check is worth keeping. OTOH, maybe the expensive bit we should protect against isn't the allocation itself but the string formatting in the |
||
"interning encountered allocating error: {}", | ||
error | ||
); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,5 @@ | ||
#![feature(llvm_asm)] | ||
|
||
// compile-flags: -Ccodegen-units=1 | ||
// build-fail | ||
// only-x86_64 | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is already in a
Box
do we need a secondBox
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this will be
None
in most cases (unless debugging the const evaluator), I'd leave this box in in order to not increase the regular size of theInterpErrorInfoInner
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But if
kind
is used won'tbacktrace
be used as well? Do we need a second indirection?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
backtrace
is only used ifRUSTC_CTFE_BACKTRACE
is set. In all other cases there will be noBox
allocated. I'm not sure how large a
Backtracestruct is, but if it's larger than
8bytes, for all normal cases we don't waste space, as no
Boxis allocated. It's fine to have arbitrary costs if
RUSTC_CTFE_BACKTRACE` is used, as that is already expensive by itself