Miri: avoid tracking current location three times #72879
Conversation
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
I did this for a few reasons:
I'd prefer working on 2. than doing this PR |
|
I very strongly dislike having redundant mutable state. For 1, I think a better solution is to not provide the query methods on For 4, the version with a span is not actually used that often. This PR overall makes code nicer as we stop having a lot of random |
|
Awaiting bors try build completion |
|
⌛ Trying commit b5bd4af967a2489941fe4ee9a622c02ee762fcd7 with merge 2b703712231b2c5d8bf99c98ff8596b3a46d539d... |
oh I like that. I'm not really worried about perf, but could you look into the regressed spans? |
| | | ||
| LL | n = if n % 2 == 0 { n/2 } else { 3*n + 1 }; | ||
| | ^^^^^^^^^^ exceeded interpreter step limit (see `#[const_eval_limit]`) | ||
| | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ exceeded interpreter step limit (see `#[const_eval_limit]`) |
There was a problem hiding this comment.
This one's ok, the previous span didn't really give the user any additional info.
| | |__- | ||
| LL | / const X: usize = { | ||
| LL | | let mut x = 0; | ||
| LL | | while x != 1000 { |
There was a problem hiding this comment.
Although when looking at the multi-line variant, I guess we should figure out how to make things point at the while instead of the entire while expression
Any hints for where to start? I am not even sure if these errors are better or worse than before. My best guess is... maybe const_prop is doing something weird? Does it even have a stack frame to fetch the
:) It's a big change though to do this for all queries, I don't think I'll be able to push this through. I could try doing it for a few queries that Miri needs, though. And anyway I figure this requires MCP so I'd first have to read up on the bureaucracy involved in that.^^ |
Oh I wasn't suggesting you do this, or it being done fast. Don't worry about it. I just want a path forward to exist, and you gave me one. |
|
☀️ Try build successful - checks-azure |
|
Queued 2b703712231b2c5d8bf99c98ff8596b3a46d539d with parent 680a4b2, future comparison URL. |
|
Finished benchmarking try commit 2b703712231b2c5d8bf99c98ff8596b3a46d539d, comparison URL. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Perf shows up to 3.5% regression in ctfe-stress-4. That's concerning. |
|
The problem is that the span may now be computed many times, since we need the span before invoking a query instead of just handling it at query failure time. Maybe we should just use the base span for queries? The little bit of help we get from having the query use the current statement's span is likely not worth it. |
Sure, makes sense. |
|
Okay, this should fix most of the perf impact. It doesn't actually change output for any of our tests... @bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit b34637db75c75bf22d201a78d1c16ebc04495f82 with merge 90b1db045a8b817d21633edf1974a53db4622000... |
|
@oli-obk I eliminated those two calls to |
|
Awaiting bors try build completion |
|
⌛ Trying commit c6512fd with merge 731149c25b195069cebc2629cea9bb0023470cee... |
|
☀️ Try build successful - checks-azure |
|
Queued 731149c25b195069cebc2629cea9bb0023470cee with parent 1fb612b, future comparison URL. |
|
Finished benchmarking try commit (731149c25b195069cebc2629cea9bb0023470cee): comparison url. |
|
I don't get it, how can it still be slowing down anything when we only do extra work in case of errors... but at least only the stress test is >1% this time (and 1.1% on a stress test is not a lot). |
|
FWIW here are min/max instruction counts, across 3 runs on the 3 consecutive parent commits of the try build plus the try build itself, for the ctfe-stress-4 benchmark on that last try build. To me, it looks like the increase in instruction counts isn't noise -- at least on the check-full benchmark, the previous results never went above 35328476078 whereas the try build came back at 35463018519.
For mostly my own future reference, generated this using: select substring(artifact.name, 0, 8) as commit, pstat_series.profile, pstat_series.cache, min(pstat.value), max(pstat.value) from pstat join pstat_series on series = pstat_series.id join artifact on aid = artifact.id where crate = 'ctfe-stress-4' and (artifact.name LIKE '731149c2%' or artifact.name LIKE '1fb612bd%' or artifact.name LIKE 'f4fbb93113aa4f0a%' or artifact.name LIKE '7c78a5f97de0%') and statistic = 'instructions:u' group by (profile, cache, artifact.id) order by (profile, cache, artifact.date); |
|
My guess is that we could likely track down the exact cause of the increased instruction counts by locally comparing two builds with cachegrind. I might actually suspect that the increased counts are coming from larger TyCtxt being copied around, but that might be spurious. OTOH, Span is another 64 bits so that's another instruction presumably every time we create or copy TyCtxt, and this PR introduces more TyCtxtAt than we had previously I think. But this is just a theory of course :) |
No, this PR means Unless you mean this PR means we construct |
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion |
|
⌛ Trying commit 2210abe with merge 70469937ac3dc60160860773f6fc701f61e60ca7... |
|
☀️ Try build successful - checks-azure |
|
Queued 70469937ac3dc60160860773f6fc701f61e60ca7 with parent 10326d8, future comparison URL. |
|
Finished benchmarking try commit (70469937ac3dc60160860773f6fc701f61e60ca7): comparison url. |
|
Yes, this seems to mostly fix the regressions. :) I wouldn't have thought that |
|
@bors r+ |
|
📌 Commit 2210abe has been approved by |
bors
added
S-waiting-on-bors
Rollup of 10 pull requests Successful merges: - rust-lang#72707 (Use min_specialization in the remaining rustc crates) - rust-lang#72740 (On recursive ADT, provide indirection structured suggestion) - rust-lang#72879 (Miri: avoid tracking current location three times) - rust-lang#72938 (Stabilize Option::zip) - rust-lang#73086 (Rename "cyclone" to "apple-a7" per changes in upstream LLVM) - rust-lang#73104 (Example about explicit mutex dropping) - rust-lang#73139 (Add methods to go from a nul-terminated Vec<u8> to a CString) - rust-lang#73296 (Remove vestigial CI job msvc-aux.) - rust-lang#73304 (Revert heterogeneous SocketAddr PartialEq impls) - rust-lang#73331 (extend network support for HermitCore) Failed merges: r? @ghost
Miri tracks the current instruction to execute in the call stack, but it also additionally has two
TyCtxtAtthat carry aSpanthat also tracks the current instruction. That is quite silly, so this PR usesTyCtxtinstead, and then uses a method for computing the current span when aTyCtxtAtis needed. Having less redundant (semi-)global state seems like a good improvement to me. :DTo keep the ConstProp errors the same, I had to add the option to
error_to_const_errorto overwrite the span. Also for some reason this changes cycle errors a bit -- not sure if we are now better or worse as giving those queries the right span. (It is unfortunately quite easy to accidentally useDUMMY_SPby calling the query on aTyCtxtinstead of aTyCtxtAt.)r? @oli-obk @eddyb