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

clear out projection subobligations after they are processed #43999

Merged
merged 1 commit into from
Aug 28, 2017

Conversation

arielb1
Copy link
Contributor

@arielb1 arielb1 commented Aug 20, 2017

After a projection was processed, its derived subobligations no longer
need any processing when encountered, and can be removed. This improves
the status of #43787.

This is actually complementary to #43938 - that PR fixes selection
caching (and @remram44's example, which "accidentally" worked because of
the buggy projection caching) while this PR fixes projection caching.

r? @nikomatsakis

}
};

// Now that the
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea

Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

This makes a lot of sense. Have you measured the impact on performance?

@@ -475,7 +475,8 @@ fn process_predicate<'a, 'gcx, 'tcx>(
trait_ref_type_vars(selcx, data.to_poly_trait_ref(tcx));
Ok(None)
}
Ok(v) => Ok(v),
// FIXME: call ProjectionCache::complete when we can.
Copy link
Contributor

Choose a reason for hiding this comment

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

This would basically be when the obligation forest decides we are done with a projection predicate, right? In other words, the FIXME is sort of in the wrong place? I would expect us to iterate over outcome.completed (elsewhere in fulfill.rs...) and invoke complete() from there.

@alexcrichton
Copy link
Member

@arielb1 should this be nominated for a backport?

@nikomatsakis
Copy link
Contributor

@alexcrichton I think that @arielb1 backed out the relevant code for beta, so this applies to nightly only.

@aidanhs aidanhs added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Aug 23, 2017
After a projection was processed, its derived subobligations no longer
need any processing when encountered, and can be removed. This improves
the status of rust-lang#43787.

This is actually complementary to rust-lang#43938 - that PR fixes selection
caching (and @remram44's example, which "accidentally" worked because of
the buggy projection caching) while this PR fixes projection caching
@arielb1 arielb1 force-pushed the immediate-project branch from 962ffb2 to 7534f73 Compare August 27, 2017 15:13
@arielb1
Copy link
Contributor Author

arielb1 commented Aug 27, 2017

Added comments.

arielb1 added a commit to arielb1/rust that referenced this pull request Aug 27, 2017
This allows caching closure signatures and kinds in the normal selection
and evaluation caches, and fixes the exponential worst-case in
@remram44's example, which is a part of rust-lang#43787.

This improvement is complenentary to rust-lang#43999 - they fix different cases.
Copy link
Contributor

@nikomatsakis nikomatsakis left a comment

Choose a reason for hiding this comment

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

Nice comments. r=me.

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Aug 28, 2017

📌 Commit 7534f73 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Aug 28, 2017

⌛ Testing commit 7534f73 with merge bef07b8...

bors added a commit that referenced this pull request Aug 28, 2017
clear out projection subobligations after they are processed

After a projection was processed, its derived subobligations no longer
need any processing when encountered, and can be removed. This improves
the status of #43787.

This is actually complementary to #43938 - that PR fixes selection
caching (and @remram44's example, which "accidentally" worked because of
the buggy projection caching) while this PR fixes projection caching.

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Aug 28, 2017

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing bef07b8 to master...

@bors bors merged commit 7534f73 into rust-lang:master Aug 28, 2017
@arielb1 arielb1 added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Aug 29, 2017
@arielb1
Copy link
Contributor Author

arielb1 commented Aug 29, 2017

beta-nominating for 1.21

arielb1 added a commit to arielb1/rust that referenced this pull request Aug 29, 2017
This allows caching closure signatures and kinds in the normal selection
and evaluation caches, and fixes the exponential worst-case in
@remram44's example, which is a part of rust-lang#43787.

This improvement is complenentary to rust-lang#43999 - they fix different cases.
bors added a commit that referenced this pull request Aug 31, 2017
[beta] Track closure signatures & kinds in freshened types

This allows caching closure signatures and kinds in the normal selection
and evaluation caches, and fixes the exponential worst-case in
@remram44's example, which is a part of #43787.

This improvement is complenentary to #43999 - they fix different cases.

This is the pre-generators variation of #43938, cloned for beta.
@alexcrichton alexcrichton added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Sep 26, 2017
@nikomatsakis nikomatsakis added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Sep 27, 2017
@nikomatsakis
Copy link
Contributor

Marking as beta-accepted. Medium patch, but important. cc @rust-lang/compiler

@nikomatsakis
Copy link
Contributor

Backporting now.

This was referenced Sep 28, 2017
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Sep 28, 2017
bors added a commit that referenced this pull request Sep 28, 2017
Beta 20170928

Backports of:

- Allow unused extern crate again #44825
- macros: fix bug in collecting trait and impl items with derives. #44757
-  `--cap-lints allow` switches off `can_emit_warnings` #44627
-  Update the libc submodule #44116
- limit and clear cache obligations opportunistically #44269
- clear out projection subobligations after they are processed #43999
bors added a commit that referenced this pull request Oct 7, 2017
Beta 20170928

Backports of:

- Allow unused extern crate again #44825
- macros: fix bug in collecting trait and impl items with derives. #44757
-  `--cap-lints allow` switches off `can_emit_warnings` #44627
-  Update the libc submodule #44116
- limit and clear cache obligations opportunistically #44269
- clear out projection subobligations after they are processed #43999
-  fix logic error in #44269's `prune_cache_value_obligations` #45065
- REVERTS #43543: Cleanup some remains of `hr_lifetime_in_assoc_type` compatibility lint:
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

6 participants