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.

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

store ObligationCause on the heap #72962

Merged
merged 2 commits into from
Jun 16, 2020
Merged

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jun 3, 2020

Stores ObligationCause on the heap using an Rc.

This PR trades off some transient memory allocations to reduce the size of–and thus the number of instructions required to memcpy–a few widely used data structures in trait solving.

@rust-highfive
Copy link
Collaborator

r? @ecstatic-morse

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 3, 2020
@lcnr lcnr changed the title Obligation cause lrc store ObligationCause on the heap Jun 3, 2020
@lcnr lcnr force-pushed the ObligationCause-lrc branch from ae3661b to 91f5ff5 Compare June 3, 2020 22:05
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 3, 2020

⌛ Trying commit 91f5ff558068ff7659e7d7c87b62594ffeb9ca7f with merge 7e933e1ada25d4898101cc7e4e2fa77498f948e0...

@jonas-schievink
Copy link
Contributor

@bors try-

@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 3, 2020

⌛ Trying commit aef40a48b1750ead607eef1a4d7b19c345ea200a with merge 8ff82fab989f4a07dfd168708da2ffc413addb04...

@bors
Copy link
Contributor

bors commented Jun 4, 2020

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

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 4, 2020

Queued 8ff82fab989f4a07dfd168708da2ffc413addb04 with parent 56daaf6, future comparison URL.

@rust-timer
Copy link
Collaborator

rust-timer commented Jun 4, 2020

Finished benchmarking try commit (8ff82fab989f4a07dfd168708da2ffc413addb04): comparison url.

@lcnr
Copy link
Contributor Author

lcnr commented Jun 4, 2020

This perf is interesting. I think this PR would be worth it as is. I might try and look a bit more into why some benchmarks are negative and what can be done here...

@lcnr lcnr force-pushed the ObligationCause-lrc branch from 28d273f to 61429d6 Compare June 4, 2020 21:57
@jonas-schievink
Copy link
Contributor

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jun 4, 2020

⌛ Trying commit 61429d6b01a297568decae2ea552e8df6710ff2b with merge ba0ecc9e775e63cb1e782322e6ead5f8b018a463...

@bors
Copy link
Contributor

bors commented Jun 5, 2020

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

@rust-timer
Copy link
Collaborator

Queued ba0ecc9e775e63cb1e782322e6ead5f8b018a463 with parent 3d5d0f8, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (ba0ecc9e775e63cb1e782322e6ead5f8b018a463): comparison url.

@jonas-schievink
Copy link
Contributor

could not find start commit for bound Commit("3d5d0f898c2f3998e50c2180c6202f193c3acdbc")

:(

@mati865
Copy link
Contributor

mati865 commented Jun 5, 2020

@jonas-schievink it's still in the queue: https://perf.rust-lang.org/status.html but it has been:

Currently benchmarking: 8ff82fab989f4
37 benchmarks left out of 38 total

for 2 hours so it appears to be stuck.

@Mark-Simulacrum
Copy link
Member

Sorry, we were migrating to a new backend for the site (and somewhat the collector), the link(s) should be good to go now.

@lcnr
Copy link
Contributor Author

lcnr commented Jun 5, 2020

Perf is looking fairly good already, I want to look into 1 more thing before this is merged though

@lcnr lcnr force-pushed the ObligationCause-lrc branch from 4603ed1 to 57fc011 Compare June 6, 2020 20:42
@ecstatic-morse
Copy link
Contributor

Thank you!

@bors r+ rollup=never

@bors
Copy link
Contributor

bors commented Jun 6, 2020

📌 Commit 57fc011ae4ff3c925d61b8b8cf7c252b3dfb6705 has been approved by ecstatic-morse

@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 6, 2020
@ecstatic-morse
Copy link
Contributor

ecstatic-morse commented Jun 6, 2020

Oh, and if you feel like squashing this, ping me and I'll re-approve.

@lcnr lcnr force-pushed the ObligationCause-lrc branch from 57fc011 to ea668d9 Compare June 6, 2020 22:08
@lcnr
Copy link
Contributor Author

lcnr commented Jun 6, 2020

@ecstatic-morse done 👍

@ecstatic-morse
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 7, 2020

📌 Commit ea668d9 has been approved by ecstatic-morse

@nnethercote
Copy link
Contributor

@nnethercote, do you want me to ping you on stuff like this?

I'm happy to be CC'd on things like this, thanks.

@Dylan-DPC-zz
Copy link

@bors p=1

@bors
Copy link
Contributor

bors commented Jun 16, 2020

⌛ Testing commit ea668d9 with merge c8a9c34...

@bors
Copy link
Contributor

bors commented Jun 16, 2020

☀️ Test successful - checks-azure
Approved by: ecstatic-morse
Pushing c8a9c34 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 16, 2020
@bors bors merged commit c8a9c34 into rust-lang:master Jun 16, 2020
@lcnr lcnr deleted the ObligationCause-lrc branch June 16, 2020 14:45
@jonas-schievink jonas-schievink added relnotes-perf Performance improvements that should be mentioned in the release notes. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 22, 2020
@jonas-schievink jonas-schievink added this to the 1.46 milestone Jun 22, 2020
@nnethercote
Copy link
Contributor

Perf results from landing show a win, as expected. Nice work!

@nnethercote
Copy link
Contributor

This was a clear perf win, but the number of additional allocations done is significant. DHAT shows me that, on numerous benchmarks, around 5% of all allocations done are for ObligationCause::misc. I'm wondering if that could be improved somehow.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 2, 2020

I didn't yet try storing only the ObligationCauseCode on the heap, this would increase the size of Obligation by 16 (still 24 less than the original size), but probably saves a lot of allocations if we don't alloc for ObligationCauseCode::MiscObligation.

I don't have the time to try this myself though

nnethercote added a commit to nnethercote/rust that referenced this pull request Jul 3, 2020
PR rust-lang#72962 shrank `Obligation` at the cost of more heap allocations;
overall it was a perf win.

This PR partly undoes that change, making `Obligation` a little bigger
(though not as big as it was) while reducing the number of heap
allocations.
@nnethercote
Copy link
Contributor

I didn't yet try storing only the ObligationCauseCode on the heap, this would increase the size of Obligation by 16 (still 24 less than the original size), but probably saves a lot of allocations if we don't alloc for ObligationCauseCode::MiscObligation.

I've tried this in #73983.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. relnotes-perf Performance improvements that should be mentioned in the release notes. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.