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

Improve merge_from_succ #68790

Merged
merged 2 commits into from
Feb 6, 2020
Merged

Improve merge_from_succ #68790

merged 2 commits into from
Feb 6, 2020
+11 −3

Conversation

nnethercote
Copy link
Contributor

A couple of small performance wins.

r? @nikomatsakis

@nnethercote
Fix up merge_from_succ.
0eb297d 
This function has a variable `changed` that is erroneously used for two
related-but-different purpose:
- to detect if the current element has changed;
- to detect if any elements have changed.

As a result, its use for the first purpose is broken, because if any
prior element changed then the code always thinks the current element
has changed. This is only a performance bug, not a correctness bug,
because we frequently end up calling `assign_unpacked` unnecessarily to
overwrite the element with itself.

This commit adds `any_changed` to correctly distinguish between the two
purposes. This is a small perf win for some benchmarks.
@nnethercote
Pull out a special case in merge_from_succ.
d62b6f2 
This is a small perf win.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2020
@nnethercote
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Feb 3, 2020

⌛ Trying commit d62b6f2 with merge e7835ee33c2030551c58f51ce5ac1f594bfc48d5...

@nnethercote
Copy link
Contributor Author

Some local results:

clap-rs-check
        avg: -1.4%      min: -1.4%      max: -1.4%
inflate-check
        avg: -1.2%      min: -1.2%      max: -1.2%
cranelift-codegen-check
        avg: -0.6%      min: -0.6%      max: -0.6%

@bors
Copy link
Contributor

bors commented Feb 3, 2020

☀️ Try build successful - checks-azure
Build commit: e7835ee33c2030551c58f51ce5ac1f594bfc48d5 (e7835ee33c2030551c58f51ce5ac1f594bfc48d5)

@rust-timer
Copy link
Collaborator

Queued e7835ee33c2030551c58f51ce5ac1f594bfc48d5 with parent 8f49d46, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit e7835ee33c2030551c58f51ce5ac1f594bfc48d5, comparison URL.

@nnethercote
Copy link
Contributor Author

@bors r=nikomatsakis

@bors
Copy link
Contributor

bors commented Feb 3, 2020

📌 Commit d62b6f2 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 Feb 3, 2020
@bors
Copy link
Contributor

bors commented Feb 4, 2020

⌛ Testing commit d62b6f2 with merge add3e5105559c64935116c58ee40a635ce39ed8f...

@rust-highfive
Copy link
Collaborator

Your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

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 @TimNN. (Feature Requests)

@bors
Copy link
Contributor

bors commented Feb 4, 2020

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 4, 2020
@nnethercote
Copy link
Contributor Author

Looks like the failure was an infra problem, unrelated to the commits.

@bors retry

@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 Feb 4, 2020
Centril added a commit to Centril/rust that referenced this pull request Feb 5, 2020
@Centril
Rollup merge of rust-lang#68790 - nnethercote:improve-merge_from_succ…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
a9cc6dc 
…, r=nikomatsakis

Improve `merge_from_succ`

A couple of small performance wins.

r? @nikomatsakis
@Centril Centril mentioned this pull request Feb 5, 2020
Closed
Centril added a commit to Centril/rust that referenced this pull request Feb 5, 2020
@Centril
Rollup merge of rust-lang#68790 - nnethercote:improve-merge_from_succ…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
eac6e90 
…, r=nikomatsakis

Improve `merge_from_succ`

A couple of small performance wins.

r? @nikomatsakis
@Centril Centril mentioned this pull request Feb 5, 2020
Closed
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 5, 2020
@Dylan-DPC
Rollup merge of rust-lang#68790 - nnethercote:improve-merge_from_succ…

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
c177941 
…, r=nikomatsakis

Improve `merge_from_succ`

A couple of small performance wins.

r? @nikomatsakis
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Feb 5, 2020
Merged
bors added a commit that referenced this pull request Feb 5, 2020
@bors
Auto merge of #68861 - Dylan-DPC:rollup-0m09hsg, r=Dylan-DPC
337b2d4 
Rollup of 8 pull requests

Successful merges:

 - #68762 (Strip unnecessary subexpression)
 - #68790 (Improve `merge_from_succ`)
 - #68809 (Make more arithmetic functions unstably const)
 - #68832 (Clean up E0264, E0267 and E0268 explanations)
 - #68840 (On suggesting `#![recursion_limit = "X"]`, note current crate name)
 - #68846 (doc fix on doc attribute)
 - #68851 (Fix issue number of `capacity` method)
 - #68858 (Merge item id stable hashing functions)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Feb 6, 2020
@bors
Auto merge of #68861 - Dylan-DPC:rollup-0m09hsg, r=Dylan-DPC
ed853b8 
Rollup of 8 pull requests

Successful merges:

 - #68762 (Strip unnecessary subexpression)
 - #68790 (Improve `merge_from_succ`)
 - #68809 (Make more arithmetic functions unstably const)
 - #68832 (Clean up E0264, E0267 and E0268 explanations)
 - #68840 (On suggesting `#![recursion_limit = "X"]`, note current crate name)
 - #68846 (doc fix on doc attribute)
 - #68851 (Fix issue number of `capacity` method)
 - #68858 (Merge item id stable hashing functions)

Failed merges:

r? @ghost
@bors bors merged commit d62b6f2 into rust-lang:master Feb 6, 2020
@nnethercote nnethercote deleted the improve-merge_from_succ branch February 6, 2020 21:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikomatsakis nikomatsakis

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.

None yet
5 participants
@nnethercote @rust-timer @bors @rust-highfive @nikomatsakis