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

Remove separate indexing of early-bound regions #99821

Merged
merged 9 commits into from
Aug 29, 2022

Conversation

cjgillot
Copy link
Contributor

@cjgillot cjgillot commented Jul 27, 2022

Based on #99728.

This PR copies some modifications from #97839 around object lifetime defaults.
These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by rustc_typeck::collect.

@rustbot rustbot added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 27, 2022
@rust-highfive
Copy link
Collaborator

r? @estebank

(rust-highfive has picked a reviewer for you, use r? to override)

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2022

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 27, 2022
@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Aug 2, 2022

☔ The latest upstream changes (presumably #100063) made this pull request unmergeable. Please resolve the merge conflicts.

@estebank
Copy link
Contributor

estebank commented Aug 4, 2022

r? rust-lang/compiler

@rust-highfive rust-highfive assigned nagisa and unassigned estebank Aug 4, 2022
@nagisa
Copy link
Member

nagisa commented Aug 15, 2022

r? rust-lang/compiler

I’ll be out on vacation for a while, so I won’t be able to get to this anytime soon.

@rust-highfive rust-highfive assigned estebank and unassigned nagisa Aug 15, 2022
@nagisa
Copy link
Member

nagisa commented Aug 15, 2022

(well, highfive should be adjusted somehow to not pass the buck straight back to the original reviewer ^^)

@estebank
Copy link
Contributor

I'm not confident reviewing this PR right now. Let's see if rerolling yields comedy.

r? rust-lang/compiler

@@ -1044,6 +1044,11 @@ impl<'a, 'tcx> EncodeContext<'a, 'tcx> {
record_array!(self.tables.inferred_outlives_of[def_id] <- inferred_outlives);
}
}
if let DefKind::TyParam | DefKind::ConstParam = def_kind {
Copy link
Member

Choose a reason for hiding this comment

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

I see other places in this PR were updated to handle DefKind::LifetimeParam | DefKind::TyParam | DefKind::ConstParam, should this also check for DefKind::LifetimeParam?

@wesleywiser
Copy link
Member

This looks good to me but I'm not an expert in this part of the compiler.

r? rust-lang/types

@compiler-errors
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Aug 29, 2022

📌 Commit da90ec1 has been approved by compiler-errors

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2022
Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Aug 29, 2022
…r-errors

Remove separate indexing of early-bound regions

~Based on rust-lang#99728

This PR copies some modifications from rust-lang#97839 around object lifetime defaults.
These modifications allow to stop counting generic parameters during lifetime resolution, and rely on the indexing given by `rustc_typeck::collect`.
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2022
Rollup of 8 pull requests

Successful merges:

 - rust-lang#98304 (Add MaybeUninit memset test)
 - rust-lang#98801 (Add a `File::create_new` constructor)
 - rust-lang#99821 (Remove separate indexing of early-bound regions)
 - rust-lang#100239 (remove an ineffective check in const_prop)
 - rust-lang#100337 (Stabilize `std::io::read_to_string`)
 - rust-lang#100819 (Make use of `[wrapping_]byte_{add,sub}`)
 - rust-lang#100934 (Remove a panicking branch from `fmt::builders::PadAdapter`)
 - rust-lang#101000 (Separate CountIsStar from CountIsParam in rustc_parse_format.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 5555e13 into rust-lang:master Aug 29, 2022
@rustbot rustbot added this to the 1.65.0 milestone Aug 29, 2022
@cjgillot cjgillot deleted the ast-lifetimes-2 branch August 29, 2022 22:54
@nnethercote
Copy link
Contributor

@cjgillot: the perf results here show that this was overall a performance win, but there were some regressions for diesel-1.4.8. Any idea what might be the cause, and how to remedy?

@nnethercote
Copy link
Contributor

Here is the first part of the Cachegrind diff, if that's helpful.

--------------------------------------------------------------------------------
Ir
--------------------------------------------------------------------------------
46,187,526  PROGRAM TOTALS

--------------------------------------------------------------------------------
Ir          file:function
--------------------------------------------------------------------------------
 8,016,144  ???:rustc_resolve::late::lifetimes::object_lifetime_default
 7,919,529  ???:<rustc_metadata::rmeta::encoder::EncodeContext>::encode_def_ids
 7,751,226  ???:rustc_query_system::query::plumbing::try_execute_query::<rustc_query_impl::plumbing::QueryCtxt, rustc_query_system::query::caches::DefaultCache<rustc_span::def_id::DefId, core::option::Option<rustc_middle::middle::resolve_lifetime::ObjectLifetimeDefault>>>
-6,925,800  ???:<rustc_middle::ty::generics::Generics as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
 6,027,485  ???:<rustc_hir::hir::WhereBoundPredicate>::is_param_bound
 4,748,382  ???:<alloc::rc::Rc<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree>> as core::ops::drop::Drop>::drop
 2,937,328  ???:<rustc_data_structures::sip128::SipHasher128>::finish128
-2,538,406  ???:<alloc::vec::Vec<rustc_ast::tokenstream::TokenTree> as core::ops::drop::Drop>::drop
-2,508,142  ???:rustc_middle::ty::codec::encode_with_shorthand::<rustc_query_impl::on_disk_cache::CacheEncoder, rustc_middle::ty::Ty, <rustc_query_impl::on_disk_cache::CacheEncoder as rustc_type_ir::codec::TyEncoder>::type_shorthands>
 2,311,692  ???:<hashbrown::raw::RawTable<(rustc_span::def_id::DefId, (core::option::Option<rustc_hir::def::DefKind>, rustc_query_system::dep_graph::graph::DepNodeIndex))>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_span::def_id::DefId, rustc_span::def_id::DefId, (core::option::Option<rustc_hir::def::DefKind>, rustc_query_system::dep_graph::graph::DepNodeIndex), core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
-2,311,692  ???:<hashbrown::raw::RawTable<(rustc_middle::ty::Ty, usize)>>::reserve_rehash::<hashbrown::map::make_hasher<rustc_middle::ty::Ty, rustc_middle::ty::Ty, usize, core::hash::BuildHasherDefault<rustc_hash::FxHasher>>::{closure
 2,207,430  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::try_mark_previous_green::<rustc_query_impl::plumbing::QueryCtxt>
 1,747,207  ???:<std::collections::hash::map::HashMap<rustc_hir::hir_id::ItemLocalId, &rustc_middle::ty::list::List<rustc_middle::ty::subst::GenericArg>, core::hash::BuildHasherDefault<rustc_hash::FxHasher>> as rustc_serialize::serialize::Encodable<rustc_query_impl::on_disk_cache::CacheEncoder>>::encode
 1,621,651  ???:<rustc_span::symbol::Symbol as rustc_serialize::serialize::Encodable<rustc_metadata::rmeta::encoder::EncodeContext>>::encode
 1,335,627  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::is_green
 1,320,616  ???:<rustc_middle::dep_graph::dep_node::DepKind as rustc_query_system::dep_graph::DepKind>::read_deps::<<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::read_index::{closure
 1,277,536  ???:rustc_query_system::query::plumbing::incremental_verify_ich::<rustc_query_impl::plumbing::QueryCtxt, rustc_span::def_id::DefId, core::option::Option<rustc_middle::middle::resolve_lifetime::ObjectLifetimeDefault>>
 1,232,078  ???:<rustc_query_system::dep_graph::graph::DepGraph<rustc_middle::dep_graph::dep_node::DepKind>>::prev_fingerprint_of
 1,225,744  ???:<rustc_middle::hir::map::Map>::get_generics 

@cjgillot
Copy link
Contributor Author

The main difference I see between diesel, which regresses, and all the other cases is that diesel uses many more trait objects. As this PR changes how object lifetime defaults are computed, having an effect correlated to the presence of trait object is consistent. That said, it does not explain the difference.
The detailed time view would point towards the object_lifetime_default query overhead and metadata generation cost.
I tried #101214 but this does not seem to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants