Skip to content

Eliminate ObligationCauseData. #73983

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
Closed
wants to merge 1 commit into from
Closed

Eliminate ObligationCauseData. #73983

wants to merge 1 commit into from

Conversation

nnethercote
Copy link
Contributor

PR #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.

r? @ghost

@nnethercote
Eliminate ObligationCauseData.
b1bf35a 
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 Author

Local measurements gave mixed results. Let's see what CI says.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Jul 3, 2020

⌛ Trying commit b1bf35a with merge 8cfe1e8001940dfc9d8f795bde769bb7615d8082...

@nnethercote nnethercote mentioned this pull request Jul 3, 2020
Merged
@bors
Copy link
Collaborator

bors commented Jul 3, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 8cfe1e8001940dfc9d8f795bde769bb7615d8082 (8cfe1e8001940dfc9d8f795bde769bb7615d8082)

@rust-timer
Copy link
Collaborator

Queued 8cfe1e8001940dfc9d8f795bde769bb7615d8082 with parent cd1a46d, future comparison URL.

@rust-timer
Copy link
Collaborator

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

@ecstatic-morse
Copy link
Contributor

Is there a semantic difference between dummy and misc? We could try to unify the two instead.

lcnr
lcnr reviewed Jul 3, 2020
View reviewed changes
src/librustc_trait_selection/traits/select/mod.rs
@@ -2124,7 +2124,7 @@ impl<'tcx> TraitObligationExt<'tcx> for TraitObligation<'tcx> {
// by using -Z verbose or just a CLI argument.
let derived_cause = DerivedObligationCause {
parent_trait_ref: obligation.predicate.to_poly_trait_ref(),
parent_code: Rc::new(obligation.cause.code.clone()),
parent_code: Rc::new(obligation.cause.code().clone()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
parent_code: Rc::new(obligation.cause.code().clone()),
parent_code: obligation.cause.code(),

And also change parent_code to Option<Rc<ObligationCauseCode>>

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in general, all occurences of parent_code currently reallocate

@nnethercote
Copy link
Contributor Author

The perf results are mixed. inflate and keccak are known to copy obligations around a lot, I wonder if waiting for #69218 to land will help them. The wins on other benchmarks are pretty small.

@nnethercote
Copy link
Contributor Author

Is there a semantic difference between dummy and misc? We could try to unify the two instead.

When talking about an ObligationCauseCode, the two are equivalent. When talking about an ObligationCause, dummy means than the span and the body_id are also dummy values. I agree that the current terminology is confusing and could be improved.

@crlf0710
Copy link
Member

Ping from triage: There's a merge conflict now.

@crlf0710 crlf0710 added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Jul 24, 2020
@Muirrum Muirrum 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 13, 2020
@nnethercote
Copy link
Contributor Author

This isn't going anywhere.

@nnethercote nnethercote mentioned this pull request Dec 13, 2021
Merged
nnethercote pushed a commit to nnethercote/rust that referenced this pull request Dec 19, 2021
@nnethercote
Eliminate ObligationCauseData.
f09b1fa 
This makes `Obligation` two words bigger, but avoids allocating a lot of
the time.

I previously tried this in rust-lang#73983 and it didn't help much, but local
timings look more promising now.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 20, 2021
@bors
Auto merge of rust-lang#91844 - nnethercote:rm-ObligationCauseData-2,…
ed7a206 
… r=Mark-Simulacrum

Eliminate `ObligationCauseData`

This makes `Obligation` two words bigger, but avoids allocating a lot of the time.

I previously tried this in rust-lang#73983 and it didn't help much, but local timings look more promising now.

r? `@ghost`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lcnr lcnr lcnr left review comments

Assignees
No one assigned
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@nnethercote @rust-timer @bors @ecstatic-morse @crlf0710 @lcnr @Muirrum