Add a per-tree error cache to the obligation forest #53255
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
rust-highfive
added
the
S-waiting-on-review
orium
changed the title
orium
force-pushed
the
fix-bug-overflow-send
branch
from
3a840e5
to
fc66ffe
Compare
|
We can run through normal performance benchmarks as well, not sure anything beyond that is terribly useful (though doesn't hurt). @bors try |
|
⌛ Trying commit fc66ffedc67921ef5cb70190c13734a609228595 with merge 79875fd549f251d287d880260ec557b413e32b00... |
|
The job 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 |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
fc66ffe
to
cc9f823
Compare
|
Hmm. I have been hoping to solve this (along with many other related problems) by the transition to chalk, which handles coinductive predicates, cycles, and a whole slew of other things much more elegantly. |
Sure, but until then this still makes rustc a tiny bit better (assuming performance is ok). It also adds a test that will make sure that there will be no regression when we switch to chalk. |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
cc9f823
to
9cf2cee
Compare
|
ping |
|
@nikomatsakis, if you believe this issue should wait to be addressed by the new trait solver, I'm ok with closing this PR. |
|
☔ The latest upstream changes (presumably #53530) made this pull request unmergeable. Please resolve the merge conflicts. |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
9cf2cee
to
1a139c8
Compare
|
@orium I have a question: are you interested in helping push the new trait integration forward? I'm hoping over the next month or two to start spinning up tasks in this area, and we could definitely use more help. =) As for this PR, I'm sorry about the delay. I'll take a look in any case and see what I think. I don't want to let the perfect be the enemy of the good and block everything on the new system. |
Yes, I would very much like to help :). I would need some mentoring for sure, since I'm not familiar with the codebase, but that is one of the great things about working on rust. How can I make sure I don't miss those issues? Currently I'm subscribed to #48416 and I joined the discord wg-trait channel. |
Sounds about right -- right now things are dormant but I expect to lay some groundwork and make a general announcement when things are "getting going" again. Maybe I'll add you to the WG-traits Rust team then? |
|
Regarding this PR: I took a look. I confess I am not immediately sure if it is correct or not. =) It seems like it might be, but handling cycles has historically been a touch tricky. One thing I was wondering, can you sort of describe what is going wrong with the existing system? It seems like it is not detecting the cycle properly, but I wasn't immediately clear why that was. |
nikomatsakis
added
S-waiting-on-author
I see that you already did that :) thank you.
There is some stuff I need to understand better. I treated some parts of the code as a blackbox to solve the cycle problem in this PR, but to get a more complete picture I should probably open some of those blackboxes. I think one good way for me to do this is to be able to visualize how an obligation forest evolves (and how |
|
Ping from triage, @orium and @nikomatsakis; whose court is this ball in at the moment? Is discussion of the correctness of this change being discussed offline, and if so, should the PR remain open? |
|
@BatmanAoD This is waiting on me, and I should be done in the next few days. |
|
After some investigation I think I have a much more complete picture of what is going on. This problem is not related with cycle detection, since it is not caused by a cycle in the current obligation forest, i.e. there would be a cycle if we didn't remove obligations that would reach (I'm glad I added graphviz to the obligations forest. It make things much easier to understand. I will open a PR soon with that.) To see what causes the overflow lets take this example: struct List<T> {
value: Rc<T>,
next: Option<Arc<List<T>>>
}If we want to check that in the next iteration Note that the After this the obligations that failed will be cleared and the obligation forest will only have |
orium
added
S-waiting-on-review
|
☔ The latest upstream changes (presumably #54389) made this pull request unmergeable. Please resolve the merge conflicts. |
|
For example, not sure this is correct, I might have an |
|
Hi @orium, I'm not sure what to think about your PR. When the ObligationForest was introduced, once an obligation was We later switched to a different approach of caching things - the evaluation cache - due to issues such as #42796 and the evaluation cache being faster in these cases. This means that now AFAICT we don't make any particular assumptions on what it means for an obligation to be I can't find a place where we make this assumption today, but it was a design assumption for the obligation forset, so I'm not quite sure. |
|
I still prefer the "deduplicate errors that occur within the same |
|
Hi @arielb1, thank you for your feedback.
Ah, that is true! We would need to instead cache a predicate like "
Yeah, sounds like a nice solution. I will give it a go this weekend 🙂 |
orium
added
S-waiting-on-author
As I said, I don't think we do this sort of caching right now, but I'm not that sure of it. |
|
Yes, that is overkill. I will implement your suggestion of deduplicating errors in the same tree. |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
01f150b
to
66d6dd5
Compare
|
@arielb1 Implemented :) |
|
The job 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 |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
66d6dd5
to
a434745
Compare
src/librustc/traits/select.rs
Outdated
| @@ -2630,7 +2630,15 @@ impl<'cx, 'gcx, 'tcx> SelectionContext<'cx, 'gcx, 'tcx> { | |||
| snapshot) | |||
| }); | |||
|
|
|||
| obligations.extend(trait_obligations); | |||
There was a problem hiding this comment.
does this serve any purpose? If not, please remove it.
There was a problem hiding this comment.
(note that "measure-free perf hack" is not a valid purpose)
There was a problem hiding this comment.
It was just a way to avoid possibly multiple allocations-and-copy of the vector. We know that it is at most trait_obligations.len() elements and the vast majority will be trait_obligations.len() - 1.
I will remove it, but I personally think it is good practice to pre-allocate vectors if the maximum number of elements is known.
There was a problem hiding this comment.
pre-allocating is OK. the problem is that you added the if check. Please remove it. unless it serves a purpose.
There was a problem hiding this comment.
Ah, you were asking about that. See explanation for the if below. In that case I will re-add the pre-allocation.
| @@ -309,6 +356,8 @@ impl<O: ForestObligation> ObligationForest<O> { | |||
| error: err, | |||
| backtrace, | |||
| }); | |||
|
|
|||
| self.insert_into_error_cache(index); | |||
There was a problem hiding this comment.
Could you do the insert_into_error_cache when the node is removed from waiting_cache instead of here? This way it's more consistent with the done_cache logic.
|
r=me with changes done |
|
The job 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 |
orium
force-pushed
the
fix-bug-overflow-send
branch
from
cd19d99
to
c7861da
Compare
src/librustc/traits/select.rs
Outdated
| for trait_obligation in trait_obligations.into_iter() { | ||
| // Since auto traits are coinductive we can skip having a predicate with a nested | ||
| // obligation equal to itself. | ||
| if trait_obligation.predicate != ty::Predicate::Trait(obligation.predicate) { |
There was a problem hiding this comment.
Is this if statement here for a reason?
There was a problem hiding this comment.
trait_obligations will almost always contain obligation (which comes from impl_or_trait_obligations in line 2624), creating a cycle. This cycle will eventually be ignored by the obligation forest anyway, so this is not necessary to fix issue #40827.
So, this is part optimization (don't generate obligations that are trivially satisfied (given that we are talking about coinductive predicates)), and part a fix in the behavior of the function (it doesn't make much sense to say that A: Send is a sub-obligation of A: Send, even if that would be trivially acceptable).
There was a problem hiding this comment.
@orium are you available on some chat?
orium
force-pushed
the
fix-bug-overflow-send
branch
from
c5bab40
to
6bfa6aa
Compare
|
@bors r+ 6bfa6aa |
orium
changed the title
orium
removed
the
S-waiting-on-author
Add a per-tree error cache to the obligation forest This implements part of what @nikomatsakis mentioned in #30533 (comment): > 1. If you find that a new obligation is a duplicate of one already in the tree, the proper processing is: > * if that other location is your parent, you should abort with a cycle error (or accept it, if coinductive) > * if that other location is not an ancestor, you can safely ignore the new obligation In particular it implements the "if that other location is your parent accept it, if coinductive" part. This fixes #40827. I have to say that I'm not 100% confident that this is rock solid. This is my first pull request 🎉, and I didn't know anything about the trait resolver before this. In particular I'm not totally sure that comparing predicates is enough (for instance, do we need to compare `param_env` as well?). Also, I'm not sure what @nikomatsakis mentions [here](#30977 (comment)), but it might be something that affects this PR: > In particular, I am wary of getting things wrong around inference variables! We can always add things to the set in their current state, and if unifications occur then the obligation is just kind of out-of-date, but I want to be sure we don't accidentally fail to notice that something is our ancestor. I decided this was subtle enough to merit its own PR. Anyway, go ahead and review 🙂. Ref #30977. # Performance We are now copying vectors around, so I decided to do some benchmarking. A simple benchmark shows that this does not seem to affect performance in a measurable way: I ran `cargo clean && cargo build` 20 times on actix-web (84b27db) and these are the results: ```text rustc master: Mean Std.Dev. Min Median Max real 66.637 2.996 57.220 67.714 69.314 user 307.293 14.741 258.093 312.209 320.702 sys 12.524 0.653 10.499 12.726 13.193 rustc fix-bug-overflow-send: Mean Std.Dev. Min Median Max real 66.297 4.310 53.532 67.516 70.348 user 306.812 22.371 236.917 314.748 326.229 sys 12.757 0.952 9.671 13.125 13.544 ``` I will do a more comprehensive benchmark (compiling rustc stage1) and post the results. r? @nikomatsakis, @nnethercote PS: It is better to review this commit-by-commit.
|
☀️ Test successful - status-appveyor, status-travis |
This implements part of what @nikomatsakis mentioned in #30533 (comment):
In particular it implements the "if that other location is your parent accept it, if coinductive" part. This fixes #40827.
I have to say that I'm not 100% confident that this is rock solid. This is my first pull request 🎉, and I didn't know anything about the trait resolver before this. In particular I'm not totally sure that comparing predicates is enough (for instance, do we need to compare
param_envas well?). Also, I'm not sure what @nikomatsakis mentions here, but it might be something that affects this PR:Anyway, go ahead and review 🙂.
Ref #30977.
Performance
We are now copying vectors around, so I decided to do some benchmarking. A simple benchmark shows that this does not seem to affect performance in a measurable way:
I ran
cargo clean && cargo build20 times on actix-web (84b27db) and these are the results:I will do a more comprehensive benchmark (compiling rustc stage1) and post the results.
r? @nikomatsakis, @nnethercote
PS: It is better to review this commit-by-commit.