Skip to content

Shrink Nonterminal #67340

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom
Merged
merged 2 commits into from
Jan 31, 2020
Merged

Shrink Nonterminal #67340

merged 2 commits into from
Jan 31, 2020

Conversation

nnethercote
Copy link
Contributor

These commits shrink Nonterminal from 240 bytes to 40 bytes. When building serde_derive they reduce the number of memcpy calls from 9.6M to 7.4M, and it's a tiny win on a few other benchmarks.

r? @petrochenkov

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2019
@nnethercote
Copy link
Contributor Author

Notable local check-clean results:

serde-serde_derive-check
        avg: -0.8%      min: -0.8%      max: -0.8%
coercions-check
        avg: -0.4%?     min: -0.4%?     max: -0.4%?
deep-vector-check
        avg: -0.3%      min: -0.3%      max: -0.3%
tuple-stress-check
        avg: 0.2%       min: 0.2%       max: 0.2%
helloworld-check
        avg: -0.1%      min: -0.1%      max: -0.1%
issue-46449-check
        avg: -0.1%      min: -0.1%      max: -0.1%
unify-linearly-check
        avg: -0.1%      min: -0.1%      max: -0.1%
syn-check
        avg: -0.1%      min: -0.1%      max: -0.1%
unicode_normalization-check
        avg: 0.1%       min: 0.1%       max: 0.1%
await-call-tree-check
        avg: -0.1%      min: -0.1%      max: -0.1%
tokio-webpush-simple-check
        avg: -0.1%      min: -0.1%      max: -0.1%

Note that serde_derive is not part of rustc-perf, but I have included it in my local copy to get the above results.

@nnethercote nnethercote mentioned this pull request Dec 16, 2019
Closed
@Centril
Copy link
Contributor

Centril commented Dec 16, 2019

Would appreciate if we could block merging this PR on first merging #67131 since this one will generate a lot of conflicts with that PR.

@Centril
Copy link
Contributor

Centril commented Dec 16, 2019

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Dec 16, 2019

⌛ Trying commit ed9ba9b with merge 23f3174...

bors added a commit that referenced this pull request Dec 16, 2019
@bors
Auto merge of #67340 - nnethercote:shrink-Nonterminal, r=<try>
23f3174 
Shrink `Nonterminal`

These commits shrink `Nonterminal` from 240 bytes to 40 bytes. When building `serde_derive` they reduce the number of `memcpy` calls from 9.6M to 7.4M, and it's a tiny win on a few other benchmarks.

r? @petrochenkov
@bors
Copy link
Collaborator

bors commented Dec 16, 2019

☀️ Try build successful - checks-azure
Build commit: 23f3174 (23f3174f3dafe415f12b9c931d59df4853cadedd)

@rust-timer
Copy link
Collaborator

Queued 23f3174 with parent a605441, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 23f3174, comparison URL.

@nnethercote
Copy link
Contributor Author

Here are the serde_derive results, for comparison:

serde-serde_derive-check
- avg: -1.1%  min: -1.8%  max: -0.7% 
- clean                 11,770,595,200.00  11,668,371,819.00  -0.9% 
- baseline incremental  14,431,966,930.00  14,328,177,470.00  -0.7% 
- clean incremental      5,746,154,634.00   5,642,802,601.00  -1.8%

@petrochenkov
Copy link
Contributor

r=me after rebase

@petrochenkov petrochenkov 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2019
@joelpalmer
Copy link

Ping from Triage: Any updates @nnethercote?

@JohnTitor JohnTitor added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 19, 2020
@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Jan 27, 2020
@nnethercote
Use P for NtTraitItem, NtImplItem, and NtForeignItem.
0d69fe8 
This commit reduces the size of `Nonterminal` from a whopping 240 bytes
to 72 bytes (on x86-64), which gets it below the `memcpy` threshold.

It also removes some impedance mismatches with `Annotatable`, which
already uses `P` for these variants.
@nnethercote
Use P for NtMeta.
7d2173e 
This commit reduces the size of `Nonterminal` from a 72 bytes to 40 bytes (on
x86-64).
@nnethercote
Copy link
Contributor Author

Back now from PTO, I have rebased.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Jan 30, 2020

⌛ Trying commit 7d2173e with merge 1941303dc808ee27858ab3d907f612c9ab9c972d...

@bors
Copy link
Collaborator

bors commented Jan 30, 2020

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

@rust-timer
Copy link
Collaborator

Queued 1941303dc808ee27858ab3d907f612c9ab9c972d with parent 9ed29b6, future comparison URL.

@nnethercote
Copy link
Contributor Author

rust-timer didn't give us a "finished" message, but the results are in and look as expected -- a small win.

r? @petrochenkov

@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Collaborator

bors commented Jan 31, 2020

📌 Commit 7d2173e has been approved by petrochenkov

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 31, 2020
@bors
Copy link
Collaborator

bors commented Jan 31, 2020

⌛ Testing commit 7d2173e with merge b1cb3c0...

bors added a commit that referenced this pull request Jan 31, 2020
@bors
Auto merge of #67340 - nnethercote:shrink-Nonterminal, r=petrochenkov
b1cb3c0 
Shrink `Nonterminal`

These commits shrink `Nonterminal` from 240 bytes to 40 bytes. When building `serde_derive` they reduce the number of `memcpy` calls from 9.6M to 7.4M, and it's a tiny win on a few other benchmarks.

r? @petrochenkov
@bors
Copy link
Collaborator

bors commented Jan 31, 2020

☀️ Test successful - checks-azure
Approved by: petrochenkov
Pushing b1cb3c0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 31, 2020
@bors bors merged commit 7d2173e into rust-lang:master Jan 31, 2020
@rust-highfive
Copy link
Contributor

📣 Toolstate changed by #67340!

Tested on commit b1cb3c0.
Direct link to PR: #67340

🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jan 31, 2020
@rust-highfive
📣 Toolstate changed by rust-lang/rust#67340!
46cb4a1 
Tested on commit rust-lang/rust@b1cb3c0.
Direct link to PR: <rust-lang/rust#67340>

🎉 rustc-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m, @rust-lang/infra).
@nnethercote nnethercote deleted the shrink-Nonterminal branch February 2, 2020 20:24
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
merged-by-bors This PR was explicitly merged by bors. 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.
8 participants
@nnethercote @Centril @rust-timer @bors @petrochenkov @joelpalmer @rust-highfive @JohnTitor