Skip to content

Only do sanity check with debug assertions on - #51658

Merged
bors merged 1 commit into
rust-lang:masterfrom
oli-obk:unregress_perf
Jun 28, 2018
Merged

Only do sanity check with debug assertions on#51658
bors merged 1 commit into
rust-lang:masterfrom
oli-obk:unregress_perf

Conversation

@oli-obk

@oli-obkoli-obk commented Jun 20, 2018

Copy link
Copy Markdown
Contributor

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?

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

nnethercote commented Jun 20, 2018

Copy link
Copy Markdown
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

Mark-Simulacrum commented Jun 20, 2018

Copy link
Copy Markdown
Member

Presumably r? @nikomatsakis or @eddyb

@nnethercote

nnethercote commented Jun 21, 2018

Copy link
Copy Markdown
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

nnethercote commented Jun 21, 2018

Copy link
Copy Markdown
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

oli-obk commented Jun 21, 2018

Copy link
Copy Markdown
ContributorAuthor

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

nnethercote commented Jun 21, 2018

Copy link
Copy Markdown
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

oli-obk commented Jun 21, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

yes

I ran rustup nightly vs stage1

@nikomatsakis

nikomatsakis commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

@bors try

@bors

bors commented Jun 21, 2018

Copy link
Copy Markdown
Collaborator

⌛ Trying commit d145a9539ae0643ddc6c516c3847e62d92033e88 with merge 41d23dc3c8502ec880cb8a06f0c24e00adbf2ea0...

@nikomatsakis

nikomatsakis commented Jun 21, 2018

Copy link
Copy Markdown
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

Mark-Simulacrum commented Jun 21, 2018

Copy link
Copy Markdown
Member

@bors retry

@bors

bors commented Jun 21, 2018

Copy link
Copy Markdown
Collaborator

⌛ Trying commit d145a9539ae0643ddc6c516c3847e62d92033e88 with merge f62ad42dd18ad2aeea92affe2e927ba808410683...

@kennytm

kennytm commented Jun 21, 2018

Copy link
Copy Markdown
Member

@bors r- try- retry

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

@borsbors 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
@kennytmkennytm 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

kennytm commented Jun 24, 2018

Copy link
Copy Markdown
Member

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

@Mark-Simulacrum

Mark-Simulacrum commented Jun 24, 2018

Copy link
Copy Markdown
Member

Perf queued.

@nikomatsakis

nikomatsakis commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

@Mark-Simulacrum perf available now?

@Mark-Simulacrum

Mark-Simulacrum commented Jun 25, 2018

Copy link
Copy Markdown
Member

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

@nikomatsakis

nikomatsakis commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

OK, seems like a wash by and large.

@nikomatsakis

nikomatsakis commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

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

@kennytmkennytm 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

oli-obk commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Done.

@kennytmkennytm 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

nikomatsakis commented Jun 26, 2018

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Jun 26, 2018

Copy link
Copy Markdown
Collaborator

📌 Commit a693c92 has been approved by nikomatsakis

@borsbors 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

oli-obk commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

@bors rollup

kennytm added a commit to kennytm/rust that referenced this pull request Jun 27, 2018
@kennytm
Rollup merge ofrust-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?
@kennytmkennytm 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:masterJun 28, 2018
@oli-obk
oli-obk deleted the unregress_perf branch June 15, 2020 15:28
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@nikomatsakisnikomatsakis

Labels

S-waiting-on-borsStatus: 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