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

perf: Merge done_cache and active_cache in ObligationForest #67892

Closed
wants to merge 4 commits into from
Closed

perf: Merge done_cache and active_cache in ObligationForest #67892

wants to merge 4 commits into from

Conversation

Marwes
Copy link
Contributor

@Marwes Marwes commented Jan 5, 2020

In the first commit, then a couple of refactorings to clarify ObligationForest

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(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 Jan 5, 2020
@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 Jan 5, 2020

⌛ Trying commit ef83c93 with merge 00f61087aa85f4929c9cc4e71b161f462cfbea97...

@petrochenkov
Copy link
Contributor

cc @nnethercote

@bors
Copy link
Contributor

bors commented Jan 5, 2020

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

@rust-timer
Copy link
Collaborator

Queued 00f61087aa85f4929c9cc4e71b161f462cfbea97 with parent 7785834, future comparison URL.

@davidtwco
Copy link
Member

r? @nnethercote (thought he's currently on PTO, might be worth re-assigning but I'm not sure who else is a good choice)

@Marwes
Copy link
Contributor Author

Marwes commented Jan 6, 2020

rust-timer didn't post the completion message and the regressions look rather excessive... The changes should really just make it do less work (modulo some added branches which should not affect things much).

bjorn3
bjorn3 reviewed Jan 6, 2020
View reviewed changes
src/librustc_data_structures/obligation_forest/mod.rs
&mut self,
obligation: O,
parent: Option<NodeIndex>,
) -> Result<(), ()> {
match self.active_cache.entry(obligation.as_predicate().clone()) {
Copy link
Member

Choose a reason for hiding this comment

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

This now clones the predicate even when it is done. Maybe that is the problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looked into it the predicate type, it is always a ty::Predicate (or &str in tests) which is copyable and only takes up 32 bytes which isn't tiny but shouldn't really affect things...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, ok I think it is because active_cache now gets really big, so the retain becomes really expensive as it fills up with None values...

@Marwes
Avoid the need to call retain on active_cache
f33df29 
The `alternative_predicates` is instead used to track when predicates
changes so that they still get removed/marked as done on completion.
Unsure if this is the best way but it seems to work.
@Marwes
Copy link
Contributor Author

Marwes commented Jan 6, 2020

I think I have a way to remove the retain on done_cache but it is not as clean as I'd like as the behavior around when the inner predicate changes is a bit hard to understand.

As a side effect the need for rewriting node indices in compress were removed which did at least remove some other messy code.

@jonas-schievink
Copy link
Contributor

Let's give this another go then.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jan 6, 2020

⌛ Trying commit f33df29 with merge 20e1c5fd4d787c557ee4c2b745ff727a5e3e87a5...

@bors
Copy link
Contributor

bors commented Jan 7, 2020

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

@rust-timer
Copy link
Collaborator

Queued 20e1c5fd4d787c557ee4c2b745ff727a5e3e87a5 with parent ebbb2bf, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 20e1c5fd4d787c557ee4c2b745ff727a5e3e87a5, comparison URL.

@Marwes Marwes closed this Jan 9, 2020
@Marwes
Copy link
Contributor Author

Marwes commented Jan 9, 2020

Closing until I can figure out a way to avoid the regressions.

@nnethercote
Copy link
Contributor

I'm late to the party here, due to PTO, but my main comment is that ObligationForest's code is really hot in some cases (esp. keccak and inflate) and has been highly optimized. Even small and seemingly innocuous changes can cause measureable regressions -- I have given up on dozens of such changes for that reason -- and this PR has a relatively large change in the data structures.

@Marwes
Copy link
Contributor Author

Marwes commented Jan 30, 2020

I am aware that it can be really hot (that's why I looked at improving it!). While there are many micro optimizations which help improve the status quo the algorithm as a whole I do believe it can be improved, iterating and filtering through all obligations on each process_call can definitely baloon in runtime if it ends up going through it multiple times without being able to actually remove any obligations (I suspect this is part of the problem in #68666, though lack of memoization is probably the main problem)

@Marwes
Copy link
Contributor Author

Marwes commented Feb 5, 2020

Looked into why inflate is so heavy on ObligationForest and the culprit appears to be that ~3k obligations end up as pending which are then iterated through on every call to process_obligations, checking if they can make progress.

This makes it clear why ObligationForest are so resistant to changes in data structures. The current way of representing the data as a plain Vec is more or less optimal when iterating through obligations to search for one that can make progress. Any changes that adds indirections to that single process ends up getting heavily penalized even if there are wins elsewhere.

So to actually get some wins here we need to do one or more of the following

  • Don't generate so many obligations that can't be solved (yet) - Might be helped by lazy normalization perhaps?

  • Track which obligations actually can make progress - If we know which (if any) obligations can make progress, there is no need to search for them. I have a rough idea, though it may very well have more overhead than it is worth.

  • Call process_obligations less - May give ObligationForest more information between each pass so that less full iterations are needed.

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

@bjorn3 bjorn3 bjorn3 left review comments

Assignees

@nnethercote nnethercote

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@Marwes @rust-highfive @jonas-schievink @rust-timer @bors @petrochenkov @davidtwco @nnethercote @bjorn3