-
Notifications
You must be signed in to change notification settings - Fork 12.5k
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
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
⌛ Trying commit c7393c762a516a2151cfbe27f7096f01cbb19933 with merge e868f5cd6ef13410be12eacb2cdf41f68cb1c226... |
☀️ Try build successful - checks-actions |
Queued e868f5cd6ef13410be12eacb2cdf41f68cb1c226 with parent 5fa22fe, future comparison URL. |
Finished benchmarking try commit (e868f5cd6ef13410be12eacb2cdf41f68cb1c226): comparison url. Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up. @bors rollup=never |
r? @oli-obk |
|
||
#[derive(Debug)] | ||
struct InterpErrorInfoInner<'tcx> { | ||
kind: InterpError<'tcx>, | ||
backtrace: Option<Box<Backtrace>>, |
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 second Box
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 the InterpErrorInfoInner
.
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't backtrace
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 if RUSTC_CTFE_BACKTRACE
is set. In all other cases there will be no Box
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
Amazing! Really good find. @bors r+ |
📌 Commit c7393c762a516a2151cfbe27f7096f01cbb19933 has been approved by |
⌛ Testing commit c7393c762a516a2151cfbe27f7096f01cbb19933 with merge d8ac472db5025de359865f5779c9d6b15b893692... |
This comment has been minimized.
This comment has been minimized.
💔 Test failed - checks-actions |
The test case fails with either one error or two errors. Use a single code generation unit to avoid nondeterminism.
Used |
@bors r+ |
📌 Commit 614b0cc has been approved by |
☀️ Test successful - checks-actions |
A job failed! Check out the build log: (web) (plain) Click to see the possible cause of the failure (guessed by this bot)
|
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this stop making any sense now?
The point of this allocates
thing was to make sure we don't allocate an error and then "catch" it (for perf reasons we wanted to avoid those allocations). But with this change we now allocate for all errors, so (a) it seems silly to also separately Box
some error kinds again, and (b) this check here makes no sense since we always allocate. So shouldn't we get rid of the extra nested Box
and the allocates
stuff?
(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 comment
The reason will be displayed to describe this comment to others. Learn more.
Unboxing of UndefinedBehaviorInfo::InvalidUninitBytes
content would be one thing to consider, but generally this check makes as much sense now as it made before. Heap allocation inside & outside cannot be easily merged together.
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.
this check makes as much sense now as it made before
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 format!
-using errors. In that case we should probably rename allocates
to pre_formatted
or so (and unbox InvalidUninitBytes
as you said).
all InterpError allocate now, so adjust alloc-error-check Cc rust-lang#82116 (comment) r? `@oli-obk`
r? @ghost