Improve assert_eq! and assert_ne! #79100

a1phyr · 2020-11-16T15:00:37Z

This PR improves assert_eq! and assert_ne! by moving the panicking code in an external function.

It does not change the fast path, but the move of the formatting in the cold path (the panic) may have a positive effect on in instruction cache use and with inlining.

Moreover, the use of trait objects instead of generic may improve compile times for assert_eq!-heavy code.

Godbolt link: ~~https://rust.godbolt.org/z/TYa9MT~~
Updated: https://rust.godbolt.org/z/bzE84x

rust-highfive · 2020-11-16T15:00:41Z

r? @cramertj

(rust_highfive has picked a reviewer for you, use r? to override)

pickfire · 2020-11-16T15:49:23Z

Does this improves runtime performance?

Godbolt link: https://rust.godbolt.org/z/TYa9MT

I can't seemed to notice the difference in the assembly.

Moreover, the use of trait objects instead of generic may improve compile times for assert_eq!-heavy code.

Looks nice but I wonder if we need 2x/4x the trait objects? Is it possible to have one <T, U> (merged both assert_eq! and assert_ne! but with different output string) and maybe another one for Arguments (I am not sure if we can merge this too)?

library/core/src/macros/internals.rs

a1phyr · 2020-11-16T15:55:26Z

I can't seemed to notice the difference in the assembly.

The difference is not in the fast path, but in the panicking one.

Looks nice but I wonder if we need 2x/4x the trait objects? Is it possible to have one <T, U> (merged both assert_eq! and assert_ne! but with different output string) and maybe another one for Arguments (I am not sure if we can merge this too)?

The only formatting that change between assert_eq! and assert_ne! is that (left == right) becomes (left != right). We could eventually give the comparison operator as parameter to assert_eq_failed.

pickfire · 2020-11-16T16:10:54Z

Looks nice but I wonder if we need 2x/4x the trait objects? Is it possible to have one <T, U> (merged both assert_eq! and assert_ne! but with different output string) and maybe another one for Arguments (I am not sure if we can merge this too)?

The only formatting that change between assert_eq! and assert_ne! is that (left == right) becomes (left != right). We could eventually give the comparison operator as parameter to assert_eq_failed.

Yes, but doesn't the current implementation makes it another function which doubles the code if a user use assert_eq! and assert_ne! on the same T and U?

Not sure about the Arguments case since I am not sure if that could be optimized into a separate function.

pickfire · 2020-11-16T16:20:54Z

I can't seemed to notice the difference in the assembly.

The difference is not in the fast path, but in the panicking one.

Diffing the two assembly outputs seemed to give me nothing, are you sure you posted the correct link? Or maybe I copied the wrong thing?

a1phyr · 2020-11-16T16:22:21Z

The only formatting that change between assert_eq! and assert_ne! is that (left == right) becomes (left != right). We could eventually give the comparison operator as parameter to assert_eq_failed.

Yes, but doesn't the current implementation makes it another function which doubles the code if a user use assert_eq! and assert_ne! on the same T and U?

Yes, it does. We can add an operator parameter, which is equal to == for assert_eq! and != for assert_ne!, and use it for the formatting later.

Not sure about the Arguments case since I am not sure if that could be optimized into a separate function.

Well, it could, but we definitely want to keep a separate generic function when there is no Argument, because it has to be created in the function calling assert_eq, and we want the generated code here as thin as possible. IMHO it is not worth the trouble.

a1phyr · 2020-11-16T16:27:14Z

Diffing the two assembly outputs seemed to give me nothing, are you sure you posted the correct link? Or maybe I copied the wrong thing?

You don't have any difference between old_basic and new_basic ? Or between old_args and new_args ? Weird... I checked the link, it works on bot my computer and my phone.

andjo403 · 2020-11-16T18:06:09Z

think that @pickfire expected something like this https://rust.godbolt.org/z/4co4f1 where there is a diff between two editors that implement the same thing in two ways. I think that I split @a1phyr:s example correct in that godbolt link.

a1phyr · 2020-11-16T18:10:44Z

@andjo403 Oh, I see. I didn't know you could produce diffs like that in Godbold.

a1phyr · 2020-11-17T00:24:26Z

This is very strange, some tests fail but seem totally unrelated (eg this test), and I'm not sure what to do about them.

Anyway, let's see if it improves compile time

@bors try @rust-timer queue

rust-timer · 2020-11-17T00:24:27Z

Insufficient permissions to issue commands to rust-timer.

bors · 2020-11-17T00:24:27Z

@a1phyr: 🔑 Insufficient privileges: not in try users

bors · 2020-11-17T00:27:57Z

@a1phyr: 🔑 Insufficient privileges: not in try users

rust-timer · 2020-11-17T00:27:57Z

Insufficient permissions to issue commands to rust-timer.

pickfire · 2020-11-17T00:28:04Z

@andjo403 Oh, I didn't know you can produce diff in godbolt.

It seemed like the assembly lines increased.

pickfire · 2020-11-17T00:49:44Z

Updated godbolt to include 6 functions, it seemed like the number of assembly lines increased, removing #[track_caller] can slightly reduce it (380 vs 512). https://rust.godbolt.org/z/EsGWbG

Merging the one with Arguments into the same function seemed to produce a slightly more lines of assembly (512 -> 540). https://rust.godbolt.org/z/a7rqrj

a1phyr · 2020-11-18T12:45:38Z

Can someone start rust-timer to see if this has a positive impact on build time ?

And also, does someone has a clue what makes this test fail ? It does not seem related to my changes.

pickfire · 2020-11-18T15:52:30Z

@jyn514

jyn514 · 2020-11-18T17:15:30Z

@pickfire please don't ping me specifically for perf runs, there's a Zulip stream for that.

@a1phyr are you sure it makes sense to measure the perf before fixing the tests? Your fix might itself have a perf impact.

a1phyr · 2020-11-18T17:44:54Z

I think the only changes that I have left to do are tests themselves (eg changing a mir output), but you're right that we should wait them to be fixed.

jyn514 · 2020-11-18T18:14:43Z

@bors try @rust-timer queue

rust-timer · 2020-11-18T18:14:45Z

Awaiting bors try build completion

bors · 2020-11-18T18:15:00Z

⌛ Trying commit 0dbf6d0f280da53dfadc453a699d01b577a33a4b with merge 7827eb84fe5a82fab0dcc16f6eb5121188f2846e...

tmiasko · 2021-02-14T23:25:25Z

Looking at last bors failure, it is likely that you will have to set profiler = true in config.toml and additionally bless run-make-fulldeps tests with ./x.py test src/test/run-make-fulldeps/ --bless.

a1phyr · 2021-02-16T08:04:34Z

Thanks ! I didn't know about the profiler config, so I thought the issue only happened on msvc target.

The test should pass now.

a1phyr · 2021-02-20T17:01:16Z

@m-ou-se I don't think I can retry myself, could you do it please ?

m-ou-se · 2021-02-20T17:05:41Z

Oh sorry, missed your previous message.

@bors r+ rollup=maybe

bors · 2021-02-20T17:05:42Z

📌 Commit 7333759 has been approved by m-ou-se

panstromek · 2021-02-20T21:31:47Z

Note: this should probably be rollup=never, it affects perf

jyn514 · 2021-02-20T21:58:42Z

@bors rollup=never

jyn514 · 2021-02-20T22:00:43Z

also @m-ou-se rollup=maybe is not the same as rollup=iffy, it's the same as not adding rollup at all: https://forge.rust-lang.org/compiler/reviews.html#rollups

m-ou-se · 2021-02-20T22:04:36Z

Oh I'm aware of rollup=maybe undoing the rollup command, but I didn't mean to apply it to this PR though. Looks like I got it mixed up with another PR I was looking at while trying to multitask, oops. Thanks for catching my mistake, @panstromek.

bors · 2021-02-21T05:41:14Z

⌛ Testing commit 7333759 with merge ed58a2b...

bors · 2021-02-21T08:22:07Z

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing ed58a2b to master...

rust-log-analyzer · 2021-02-21T08:23:36Z

A job failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

  git switch -

Turn off this advice by setting config variable advice.detachedHead to false

HEAD is now at ed58a2b03 Auto merge of #79100 - a1phyr:better_assert_eq, r=m-ou-se
##[group]Run src/ci/publish_toolstate.sh
src/ci/publish_toolstate.sh
env:
  SCCACHE_BUCKET: rust-lang-ci-sccache2
  DEPLOY_BUCKET: rust-lang-ci2
  TOOLSTATE_REPO: https://github.com/rust-lang-nursery/rust-toolstate
---
  CACHE_DOMAIN: ci-caches.rust-lang.org
  TOOLSTATE_REPO_ACCESS_TOKEN: ***
##[endgroup]
Cloning into 'rust-toolstate'...
/home/runner/work/rust/rust/src/tools/publish_toolstate.py:121: DeprecationWarning: 'U' mode is deprecated
📣 Toolstate changed by rust-lang/rust#79100!
  with open(path, 'rU') as f:

Tested on commit rust-lang/rust@ed58a2b03b6284b070fae2349898b16df98b7765.
Direct link to PR: <https://github.com/rust-lang/rust/pull/79100>

🎉 miri on windows: test-fail → test-pass (cc @oli-obk @eddyb @RalfJung).
🎉 miri on linux: test-fail → test-pass (cc @oli-obk @eddyb @RalfJung).
Traceback (most recent call last):
Traceback (most recent call last):
  File "/home/runner/work/rust/rust/src/tools/publish_toolstate.py", line 338, in <module>
    response = urllib2.urlopen(urllib2.Request(
  File "/usr/lib/python3.8/urllib/request.py", line 222, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.8/urllib/request.py", line 522, in open
    req = meth(req)
  File "/usr/lib/python3.8/urllib/request.py", line 1281, in do_request_
    raise TypeError(msg)
TypeError: POST data should be bytes, an iterable of bytes, or a file object. It cannot be of type str.
##[error]Process completed with exit code 1.

ghost · 2021-03-13T10:05:51Z

library/core/src/panicking.rs

+            Some(args) => panic!(
+                r#"assertion failed: `(left {} right)`
+  left: `{:?}`,
+ right: `{:?}: {}`"#,


Was the change that moves : {} inside the ` quotes intentional?

The panic message now looks suboptimal to me: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=0cbe6c5669d0b7458c6d9f67a3ea7518

Nightly

right: `3: 1 + 1 should definitely be 3`', src/main.rs:2:5

Stable

right: `3`: 1 + 1 should definitely be 3', src/main.rs:2:5

(In case it was unintentional, I have a patch.)

@hyd-dev oh, good catch! Can you send that patch as a PR? (Feel free to assign me with r? @m-ou-se.)

Fix panic message of `assert_failed_inner` cc rust-lang#79100 (comment) r? `@m-ou-se`

Fix panic message of `assert_failed_inner` cc rust-lang#79100 (comment) r? ``@m-ou-se``

Debug-print result when an unstable fingerprint is detected Helps with issues like rust-lang#83311 I had previously tried to do this in rust-lang#80692, but it had a significant performance impact (even though the code was never actually run). Hopefully, this will be better now that rust-lang#79100 has been merged.

rust-highfive assigned cramertj Nov 16, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 16, 2020

a1phyr force-pushed the better_assert_eq branch from 550d4de to f1e7052 Compare November 16, 2020 15:27

pickfire reviewed Nov 16, 2020

View reviewed changes

library/core/src/macros/internals.rs Outdated Show resolved Hide resolved

a1phyr force-pushed the better_assert_eq branch 2 times, most recently from 0c9f27d to 5d79f11 Compare November 16, 2020 23:30

Fix run-make-fulldeps test

7333759

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 20, 2021

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 21, 2021

bors merged commit ed58a2b into rust-lang:master Feb 21, 2021

rustbot added this to the 1.52.0 milestone Feb 21, 2021

bors mentioned this pull request Feb 21, 2021

Remove MIR assignments to ZST types #80493

Closed

ghost reviewed Mar 13, 2021

View reviewed changes

ghost mentioned this pull request Mar 13, 2021

Fix panic message of assert_failed_inner #83081

Merged

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 13, 2021

Rollup merge of rust-lang#83081 - hyd-dev:assert-message, r=m-ou-se

56b25c0

Fix panic message of `assert_failed_inner` cc rust-lang#79100 (comment) r? `@m-ou-se`

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Mar 14, 2021

Rollup merge of rust-lang#83081 - hyd-dev:assert-message, r=m-ou-se

f8206ac

Fix panic message of `assert_failed_inner` cc rust-lang#79100 (comment) r? ``@m-ou-se``

Aaron1011 mentioned this pull request Mar 20, 2021

Debug-print result when an unstable fingerprint is detected #83314

Merged

a1phyr mentioned this pull request Apr 1, 2021

Pretty print assertion failures in tests #79001

Closed

m-ou-se mentioned this pull request Oct 19, 2021

Clippy breaks when we change anything about the std macros rust-lang/rust-clippy#7843

Closed

a1phyr mentioned this pull request Feb 10, 2023

Constify assert_eq! and assert_ne! #103639

Closed

a1phyr deleted the better_assert_eq branch March 12, 2024 17:14

Improve assert_eq! and assert_ne! #79100

Improve assert_eq! and assert_ne! #79100

Conversation

a1phyr commented Nov 16, 2020 • edited Loading

rust-highfive commented Nov 16, 2020

pickfire commented Nov 16, 2020 • edited Loading

a1phyr commented Nov 16, 2020 • edited Loading

pickfire commented Nov 16, 2020 • edited Loading

pickfire commented Nov 16, 2020

a1phyr commented Nov 16, 2020

a1phyr commented Nov 16, 2020

andjo403 commented Nov 16, 2020

a1phyr commented Nov 16, 2020

a1phyr commented Nov 17, 2020 • edited Loading

rust-timer commented Nov 17, 2020

bors commented Nov 17, 2020

bors commented Nov 17, 2020

rust-timer commented Nov 17, 2020

pickfire commented Nov 17, 2020

pickfire commented Nov 17, 2020

a1phyr commented Nov 18, 2020

pickfire commented Nov 18, 2020

jyn514 commented Nov 18, 2020

a1phyr commented Nov 18, 2020

jyn514 commented Nov 18, 2020

rust-timer commented Nov 18, 2020

bors commented Nov 18, 2020

tmiasko commented Feb 14, 2021

a1phyr commented Feb 16, 2021 • edited Loading

a1phyr commented Feb 20, 2021

m-ou-se commented Feb 20, 2021

bors commented Feb 20, 2021

panstromek commented Feb 20, 2021

jyn514 commented Feb 20, 2021

jyn514 commented Feb 20, 2021

m-ou-se commented Feb 20, 2021 • edited Loading

bors commented Feb 21, 2021

bors commented Feb 21, 2021

rust-log-analyzer commented Feb 21, 2021

ghost Mar 13, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Nightly

Stable

m-ou-se Mar 13, 2021

Choose a reason for hiding this comment

a1phyr commented Nov 16, 2020 •

edited

Loading

pickfire commented Nov 16, 2020 •

edited

Loading

a1phyr commented Nov 16, 2020 •

edited

Loading

pickfire commented Nov 16, 2020 •

edited

Loading

a1phyr commented Nov 17, 2020 •

edited

Loading

a1phyr commented Feb 16, 2021 •

edited

Loading

m-ou-se commented Feb 20, 2021 •

edited

Loading

ghost Mar 13, 2021 •

edited by ghost

Loading