Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Only do sanity check with debug assertions on #51658

Merged
merged 1 commit into from
Jun 28, 2018
Merged

Only do sanity check with debug assertions on #51658

merged 1 commit into from
Jun 28, 2018

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Jun 20, 2018

r? @nnethercote

I'm slighty confused. These changes address code that the unused-warnings benchmark doesn't go through, yet I see a 5% improvement to nightly on the check run, and no improvement on the other runs.

Maybe this change allows unrelated code in the same function to be better optimized?

@estebank estebank added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 20, 2018
@nnethercote
Copy link
Contributor

The change looks fine to me, though I don't have review privileges so you should probably ask someone else as well.

I don't know why the instruction count would change (particularly by such a large amount) if this code isn't executed, as you say. But if it helps, might as well land it!

@Mark-Simulacrum
Copy link
Member

Presumably r? @nikomatsakis or @eddyb

@nnethercote
Copy link
Contributor

I see a 5% improvement to nightly on the check run

How exactly did you get this measurement? I just did a local benchmarking run of unused-warnings-check and saw no change with the patch applied. I then measured with Cachegrind, and again saw no change.

@nnethercote
Copy link
Contributor

Also, in #51414, you said:

This sanity check is on a hot path. I'll change it to a debug assert

How did you identify that as a hot path? Could the regression be caused by a different part of #51414's change?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2018

Yes the regression is very likely elsewhere. I just wanted to get this fix out of the way. I came to the conclusion that this code could be relevant by looking at what code transitively calls this function, and it's a lot.

Weird that you are not seeing the change. I ran it multiple times to ensure that the value doesn't change. Note that I used stage 1 because I didn't want to wait for stage 2.

But as I said this doesn't improve perf in non-check cases, so I'm not sure what's going on

@nnethercote
Copy link
Contributor

I ran it multiple times to ensure that the value doesn't change.

What exactly did you run? Are you using the rustc-perf harness?

@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 21, 2018 •
edited
Loading

What exactly did you run? Are you using the rustc-perf harness?

yes

I ran rustup nightly vs stage1

@nikomatsakis
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jun 21, 2018

⌛ Trying commit d145a9539ae0643ddc6c516c3847e62d92033e88 with merge 41d23dc3c8502ec880cb8a06f0c24e00adbf2ea0...

@nikomatsakis
Copy link
Contributor

The change seems ... fine, though I might rather keep an assert! (but not the println! -- yuck) than a debug_assert! if there is no real benefit. Particularly this case which is just a ptr equality check.

@Mark-Simulacrum
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Jun 21, 2018

⌛ Trying commit d145a9539ae0643ddc6c516c3847e62d92033e88 with merge f62ad42dd18ad2aeea92affe2e927ba808410683...

@kennytm
Copy link
Member

kennytm commented Jun 21, 2018

@bors r- try- retry

The try build is complete but we've restarted bors and got the states confused.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 21, 2018
@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 21, 2018
@kennytm
Copy link
Member

kennytm commented Jun 24, 2018

@Mark-Simulacrum perf is requested try#f62ad42dd18ad2aeea92affe2e927ba808410683 vs master#f790780451a724dce9be0ba03ff217955036ff88.

@Mark-Simulacrum
Copy link
Member

Perf queued.

@nikomatsakis
Copy link
Contributor

@Mark-Simulacrum perf available now?

@Mark-Simulacrum
Copy link
Member

@nikomatsakis I believe this is the right URL: https://perf.rust-lang.org/compare.html?start=f790780451a724dce9be0ba03ff217955036ff88&end=f62ad42dd18ad2aeea92affe2e927ba808410683&stat=instructions%3Au

@nikomatsakis
Copy link
Contributor

OK, seems like a wash by and large.

@nikomatsakis
Copy link
Contributor

@oli-obk maybe just remove the weird println! or convert them to debug!?

@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 26, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 26, 2018

Done.

@kennytm kennytm added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 26, 2018
@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 26, 2018

📌 Commit a693c92 has been approved by nikomatsakis

@bors 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 Jun 26, 2018
@oli-obk
Copy link
Contributor Author

oli-obk commented Jun 26, 2018

@bors rollup

kennytm added a commit to kennytm/rust that referenced this pull request Jun 27, 2018
@kennytm
Rollup merge of rust-lang#51658 - oli-obk:unregress_perf, r=nikomatsakis
1ebd9ad 
Only do sanity check with debug assertions on

r? @nnethercote

I'm slighty confused. These changes address code that the `unused-warnings` benchmark doesn't go through, yet I see a 5% improvement to nightly on the `check` run, and no improvement on the other runs.

Maybe this change allows unrelated code in the same function to be better optimized?
@kennytm kennytm mentioned this pull request Jun 27, 2018
Merged
bors added a commit that referenced this pull request Jun 27, 2018
@bors
Auto merge of #51859 - kennytm:rollup, r=kennytm
266afeb 
Rollup of 7 pull requests

Successful merges:

 - #49987 (Add str::split_ascii_whitespace.)
 - #50342 (Document round-off error in `.mod_euc()`-method, see issue #50179)
 - #51658 (Only do sanity check with debug assertions on)
 - #51799 (Lower case some feature gate error messages)
 - #51800 (Add a compiletest header for edition)
 - #51824 (Fix the error reference for LocalKey::try_with)
 - #51842 (Document that Layout::from_size_align does not allow align=0)

Failed merges:

r? @ghost
@bors bors merged commit a693c92 into rust-lang:master Jun 28, 2018
@oli-obk oli-obk deleted the unregress_perf branch June 15, 2020 15:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@nikomatsakis nikomatsakis

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@oli-obk @nnethercote @Mark-Simulacrum @nikomatsakis @bors @kennytm @estebank