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

[WIP] traits/fulfill: track GenericArgss instead of InferTys in stalled_on. #70181

Closed
wants to merge 1 commit into from

Conversation

eddyb
Copy link
Member

@eddyb eddyb commented Mar 20, 2020

EDIT: superseded by #70213 for performance reasons.

This PR addresses the representation side of #70180, but only actually collects ty::Infers via Ty::walk into stalled_on (collecting ty::ConstKind::Infers requires #70164).

However, it should be enough to handle #70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no walk-ing is involved).

This PR will make step-by-step changes in order to check the perf impact of each.

r? @nikomatsakis cc @nnethercote

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 20, 2020
@eddyb eddyb force-pushed the fulfill-pending-generic-arg branch from 244a9d7 to e63c410 Compare March 20, 2020 14:21
@eddyb
Copy link
Member Author

eddyb commented Mar 20, 2020

I'm doing a perf run with just this first commit, to assess the impact of having to match on Ty to get at the ty::InferTy inside each stalled_on entry:

traits/fulfill: track full Tys instead of InferTys in stalled_on.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Mar 20, 2020

⌛ Trying commit e63c410 with merge fc63e0e7ac01afe8ebf2cbb38466eb80c1d42922...

@varkor varkor added the F-const_generics `#![feature(const_generics)]` label Mar 20, 2020
@bors
Copy link
Contributor

bors commented Mar 20, 2020

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

@rust-timer
Copy link
Collaborator

Queued fc63e0e7ac01afe8ebf2cbb38466eb80c1d42922 with parent f4c675c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit fc63e0e7ac01afe8ebf2cbb38466eb80c1d42922, comparison URL.

@Centril
Copy link
Contributor

Centril commented Mar 20, 2020

Perf seems mostly improved I'd say, but it's a bit of a wash.

@eddyb
Copy link
Member Author

eddyb commented Mar 21, 2020

@Centril most of the perf improvements around 1% I've seen lately look dubious and unrelated (i.e. likely noise), so I focus on the regressions and try to find a relevant one.

Besides, it should be impossible for this PR to make anything faster, so there's that.

keccak-opt clean and inflate-check clean paint a pretty clear picture: type-checking is slower.

I think the efficient approach moving forward would be to make an enum StalledOnInferVar with these variants (Ty, Int and Float) plus a Const one:

ty::TyVar(v) => {
use self::type_variable::TypeVariableValue;
// If `inlined_probe` returns a `Known` value its `kind` never
// matches `infer`.
match self.infcx.inner.borrow_mut().type_variables.inlined_probe(v) {
TypeVariableValue::Unknown { .. } => false,
TypeVariableValue::Known { .. } => true,
}
}
ty::IntVar(v) => {
// If inlined_probe_value returns a value it's always a
// `ty::Int(_)` or `ty::UInt(_)`, which never matches a
// `ty::Infer(_)`.
self.infcx.inner.borrow_mut().int_unification_table.inlined_probe_value(v).is_some()
}
ty::FloatVar(v) => {
// If inlined_probe_value returns a value it's always a
// `ty::Float(_)`, which never matches a `ty::Infer(_)`.
//
// Not `inlined_probe_value(v)` because this call site is colder.
self.infcx.inner.borrow_mut().float_unification_table.probe_value(v).is_some()
}

I'm closing this PR to serve as a tombstone for the inefficient approach.
EDIT: proposed approach submitted as #70213.

@eddyb eddyb closed this Mar 21, 2020
t: ty::PolyTraitRef<'tcx>,
) -> Vec<ty::InferTy> {
t.skip_binder() // ok b/c this check doesn't care about regions
ty: ty::PolyTraitRef<'tcx>,
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, I should've renamed this to trait_ref.

Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Centril added a commit to Centril/rust that referenced this pull request Mar 24, 2020
…omatsakis

traits/fulfill: allow `stalled_on` to track `ty::Const::Infer(_)` (unused yet).

This PR addresses the representation side of rust-lang#70180, but only *actually collects* `ty::Infer`s via `Ty::walk` into `stalled_on` (collecting `ty::ConstKind::Infer`s requires rust-lang#70164).

However, it should be enough to handle rust-lang#70107's needs (WF obligations are stalled only on the outermost type/const being an inference variable, no `walk`-ing is involved).

This is my second attempt, see rust-lang#70181 for the previous one, which unacceptably regressed perf.

r? @nikomatsakis cc @nnethercote
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-const_generics `#![feature(const_generics)]` S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants