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

Convert ensure_answer_recursively to be iterative instead of recursive #281

Merged
merged 37 commits into from Dec 6, 2019

Conversation

@jackh726
Copy link
Contributor

jackh726 commented Nov 10, 2019

Basically all of pursue_strand, pursue_next_strand, incorporate_result_from_positive_subgoal, and incorporate_result_from_negative_subgoal got merged into the one function, with changes made to make it iterative.

Right now, a lot of code has not been moved into separate functions, but I'm definitely working on this. All in all, it's a bit more LOC, but I've also added in a lot of documentation/comments to help follow what's happening.

Copy link
Collaborator

nikomatsakis left a comment

Initial round of review comments :) Didn't through the whole PR yet.

This is probably a good idea, but I'm taking a bit of time to get used to it.

As an aside, I wish we had better performance information. We're not at the point where we can even measure such things for real. I suppose we should be optimizing for cleanest code right now more than anything.

chalk-engine/src/forest.rs Outdated Show resolved Hide resolved
if test(&strand) {
return Some(strand);
}
self.strands.push_front(strand);

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 11, 2019

Collaborator

isn't this going to just test and re-test the same strand over and over?

This comment has been minimized.

Copy link
@jackh726

jackh726 Nov 11, 2019

Author Contributor

So, it would, but the assumption is that any Strands after this have a >= work.

*entry.get_mut() = false;
true
if *was_ambiguous && !answer.ambiguous {
panic!("New answer was not ambiguous whereas previous answer was.");

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 11, 2019

Collaborator

This doesn't seem like a valid reason to panic to me -- it can definitely happen that we find an ambiguous answer through one route, but a valid (unambiguous) answer through another.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 11, 2019

Collaborator

I don't quite understand why it was changed.

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 11, 2019

Collaborator

I should probably create some tests to exercise this path, huh?

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Nov 11, 2019

Collaborator

That said, I think we could just ignore the answer and return false if we already have a better answer

This comment has been minimized.

Copy link
@jackh726

jackh726 Nov 11, 2019

Author Contributor

So yes, this path is never taken in any tests. I changed it to an explicit panic here because returning true here means that upstream the answer index gets incremented (since it's assumed true here means a new answer.

This comment has been minimized.

Copy link
@jackh726

jackh726 Nov 11, 2019

Author Contributor

So, if we want the behavior here to remain the same (that new unambiguous answers can replace ambiguous ones), then code elsewhere needs to change too.

chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Nov 21, 2019

Before this is merged I need to rebase to master and remove the commits from the CanonicalStrand pr. I'll get this done sometime today. For the sake of simplicity of review, I'll make these as new commits (so the branch won't actually be mergeable probably, but the complete diff will reflect the final changes). @nikomatsakis when you're done reviewing, I'll squash all the commits and actually make it mergeable. Otherwise, I think it'll be kind of difficult to reapply the commits for this PR onto master directly.

@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Nov 22, 2019

@nikomatsakis Ok, I've reverted most of the changes from the CanonicalStrand PR here. However, there are a couple things that aren't basically identical to master:

  • In push_initial_strands_instantiated, I've kept the change that clones the InferenceTable instead of relying on resolvent_clause to return a CanonicalStrand and rolling back the changes. I don't really think there's a performance difference and it's a bit cleaner this way in my opinion (and makes it easier in the future if/when we want to remove CanonicalStrand.
  • instantiate_ex_clause returns a (C::InferenceTable, Strand<C>) rather than taking a impl Fn(C::InferenceTable, Strand<C>) -> R. When logic is recursive, this is fine, because the instantiated Strand is passed into pursue_strand. But with the iterative approach, this gets into the active_strand on the Stack. If we really need to go back to having it take a impl Fn, I think it's doable, but I didn't want to add yet another thing to follow.

Again, it still hasn't been rebased to the current master, but I'm just going to squash the commits before I do that and want to leave them separated for review, because I think that'll be easier.
Actually it was simple enough to rebase onto master, so I did that.

jackh726 added 22 commits Nov 7, 2019
… work we do. When a cyclic strand is encountered, immediately push it back onto the table (while tracking the work timestamp). Only take strands that are less than our time.
Make push_answer panic if previous answer had same substs, but was ambiguous whereas current answer isn't. This is needed because logic upstream assumes that returning true from push_answer is a *new* answer.
…_answer_recursively checks to separate function
…RootSearchResult at this point. Separate enum just for TableCheck
@jackh726 jackh726 force-pushed the jackh726:fuel_friendly branch from de5261e to 2036fbf Nov 22, 2019
@nikomatsakis

This comment has been minimized.

Copy link
Collaborator

nikomatsakis commented Nov 26, 2019

Per our discussion, @jackh726, I'm going to try and review this first -- sorry for the delay. On vacation this week so that'll slow me up but I might do a bit since this sort of thing is actually pretty fun. :)

Copy link
Collaborator

nikomatsakis left a comment

next round of comments -- mostly nits -- didn't quite finish reading the logic all the way through

chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
chalk-engine/src/logic.rs Outdated Show resolved Hide resolved
…instead inlines the checks at the two sites where it's used. (Really one and a half, since for the initial_table/answer, it can't even be cyclic.) Separated out the on_positive_cycle and on_coinductive_cycle logic to separate functions. This just makes it easier to reason about what is happening and why.
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Dec 2, 2019

@nikomatsakis your comments should be addressed (other than the one nit I disagree with, see comment). I decided to just inline the check_table checks since half of them aren't needed for one of the two call sites anyways.

nikomatsakis added 13 commits Dec 6, 2019
The old name didn't sound like something which had side-effects.  Not
sure if the new name is that much better, but at least it sounds
side-effect-y to me.
This was confusing to me because `vec.pop()` returns the popped item,
but `stack.pop()` was returning the top-most element.
It seems to be always incremented when we push something onto the
stack and that is it.
I'm shooting for a style that is separating out the comment a bit more
from the code, since I was finding it a bit hard to get the
"high-level picture" at present.
@nikomatsakis nikomatsakis force-pushed the jackh726:fuel_friendly branch from 087bef4 to a448714 Dec 6, 2019
@nikomatsakis nikomatsakis merged commit bb359ba into rust-lang:master Dec 6, 2019
3 checks passed
3 checks passed
Test (stable)
Details
Test (nightly)
Details
Format
Details
@jackh726

This comment has been minimized.

Copy link
Contributor Author

jackh726 commented Dec 6, 2019

🎉

@jackh726 jackh726 deleted the jackh726:fuel_friendly branch Dec 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.