rustc_serialize: make assert more informative.

[matthiaskrgr]

#91650 (comment)

Make sure it prints the left/right values when it fails.

[rust-highfive]

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

[klensy]

This code was recently changed in #91407

[oli-obk]

@bors r+ rollup

[bors]

📌 Commit 90e7f49 has been approved by oli-obk

[Mark-Simulacrum]

@bors rollup=never - potentially perf sensitive

[the8472]

The use of assert instead of assert_eqwas requested during review.

[matthiaskrgr]

@bors r-
Hmm, do you think something like adding an assert_eq-like message so that the values are printed still effects perf negatively?
assert!(x==y, "{} vs {}", x, y);

[Mark-Simulacrum]

@bors try @rust-timer queue

I think we should verify (if we haven't yet) whether it's actually a perf hit, first.

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 90e7f49 with merge dcf1ec9f0d09e1f0fe4f36e1a0224097753a8ed4...

[the8472]

I didn't directly verify the impact of the change, but the post-PR perf run looks ~0.1% better than the perf run before the review changes.

I am a tiny bit annoyed to be told to do one thing to see a PR undoing it.

Hmm, do you think something like adding an assert_eq-like message so that the values are printed still effects perf negatively?
assert!(x==y, "{} vs {}", x, y);

I agree with the original review comment here:

And in this particular case the better error message from assert_eq! doesn't really give additional information. So could you try just putting an assert! here?

To explain: left and right are probably of little use without a lot more context (offsets, raw data of what is being deserialized, what are we trying to deserialize etc.). The assert fires when the serialized data does not match the expected serialization format. Which wrong byte you find at that particular location probably doesn't matter, the fact that it is wrong at all reveals a bigger problem.

I haven't looked at the assert! expansion to figure out what @michaelwoerister meant wrt. codegen impact.

[matthiaskrgr]

Sorry, I did not mean to discredit any of your work.
I did not read through the original PR before making this PR which would have been wise.

I just saw the "assertion failure x == y" on my screen and though "Huh, that does not tell a lot" so I patched it locally to see what was going on, and also made a pr because why not.

About the assert_eq error message; I don't fully agree:
While I don't understand the particular meaning of the number, I think it can still provide some kind of context.
For example "0" instead of "193" may indicate that something is missing.
"194" or "192" might indicate an off-by-one somewhere in the code.
I was also a bit relieved to find out that the error would consistently be 0 vs 192 instead of a different number on every run, so I can at least assume that the error is kind of consistent.

[matthiaskrgr]

Looking at godbolt, it seems that assert indeed generates much less instructions than assert_eq, interesting.
https://rust.godbolt.org/z/57oT1P68z

Still curious about the actual perf result though. :)

[the8472]

"194" or "192" might indicate an off-by-one somewhere in the code.

The value is not calculated, it is a constant. It can't be off by one.

For example "0" instead of "193" may indicate that something is missing.

Well, any value other than the expected constant would indicate that the sentinel byte itself is missing. Which value you happen to end up seeing in this place would depend on what comes next after the string in that particular data structure that triggers the assert first.

Sorry, I did not mean to discredit any of your work.

My annoyance only stems from the left hand not knowing what the right hand was doing. Which can happen, doesn't make it not annoying. I'm not seeing it as an attack on my work, just... suboptimal communication, a bit of unnecessary friction, that's all.

[bors]

☀️ Try build successful - checks-actions
Build commit: dcf1ec9f0d09e1f0fe4f36e1a0224097753a8ed4 (dcf1ec9f0d09e1f0fe4f36e1a0224097753a8ed4)

[rust-timer]

Queued dcf1ec9f0d09e1f0fe4f36e1a0224097753a8ed4 with parent 4459e72, future comparison URL.

[rust-timer]

Finished benchmarking commit (dcf1ec9f0d09e1f0fe4f36e1a0224097753a8ed4): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression