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.

Sign up for GitHub

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

self-profile: Cache more query key strings when doing self-profiling. #75452

Merged
merged 1 commit into from
Aug 14, 2020
Merged

self-profile: Cache more query key strings when doing self-profiling. #75452

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 45 additions & 1 deletion src/librustc_middle/ty/query/profiling_support.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use crate::ty::context::TyCtxt;
use crate::ty::WithOptConstParam;
use measureme::{StringComponent, StringId};
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::profiling::SelfProfiler;
use rustc_hir::def_id::{CrateNum, DefId, DefIndex, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::def_id::{CrateNum, DefId, DefIndex, LocalDefId, CRATE_DEF_INDEX, LOCAL_CRATE};
use rustc_hir::definitions::DefPathData;
use rustc_query_system::query::QueryCache;
use rustc_query_system::query::QueryState;
Expand Down Expand Up @@ -154,6 +155,49 @@ impl SpecIntoSelfProfilingString for DefIndex {
}
}

impl SpecIntoSelfProfilingString for LocalDefId {
fn spec_to_self_profile_string(
&self,
builder: &mut QueryKeyStringBuilder<'_, '_, '_>,
) -> StringId {
builder.def_id_to_string_id(DefId { krate: LOCAL_CRATE, index: self.local_def_index })
}
}

impl<T: SpecIntoSelfProfilingString> SpecIntoSelfProfilingString for WithOptConstParam<T> {
fn spec_to_self_profile_string(
&self,
builder: &mut QueryKeyStringBuilder<'_, '_, '_>,
) -> StringId {
// We print `WithOptConstParam` values as tuples to make them shorter
// and more readable, without losing information:
//
// "WithOptConstParam { did: foo::bar, const_param_did: Some(foo::baz) }"
// becomes "(foo::bar, foo::baz)" and
// "WithOptConstParam { did: foo::bar, const_param_did: None }"
// becomes "(foo::bar, _)".
Copy link
Contributor

@lcnr lcnr Aug 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

What does self profiling need the WithOptConstParam for?

You probably want to uniquely identify query calls? Considering that self.const_param_did should be unique for each DefId (as we otherwise cause an ICE while building mir) you might also be able to store this as (did, bool) for some additional wins.

Copy link
Member Author

Choose a reason for hiding this comment

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

What does self profiling need the WithOptConstParam for?

Some queries (e.g. mir_build) take WithOptConstParam values as keys. Since self-profiling can record a string representation for each query key that gets used some of those keys will be stringified WithOptConstParam values. You can find the full list of queries (and their keys) in https://github.com/rust-lang/rust/blob/master/src/librustc_middle/query/mod.rs.

You probably want to uniquely identify query calls? Considering that self.const_param_did should be unique for each DefId (as we otherwise cause an ICE while building mir) you might also be able to store this as (did, bool) for some additional wins.

I'm not quite sure I follow. You are saying that there is an invariant around const_param_did that makes sure that it is always the same (or None) for a given did value?

In any case, I think it's a good idea to keep this as close to the original value as possible. The encoding format should make sure that there is no data redundancy (assuming that that DefId was already mentioned somewhere already).

Copy link
Contributor

Choose a reason for hiding this comment

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

Since self-profiling can record a string representation for each query key that gets used

That's what I wasn't sure about, thanks 👍

I'm not quite sure I follow. You are saying that there is an invariant around const_param_did that makes sure that it is always the same (or None) for a given did value?

yes.

In any case, I think it's a good idea to keep this as close to the original value as possible. The encoding format should make sure that there is no data redundancy (assuming that that DefId was already mentioned somewhere already).

👍


let did = StringComponent::Ref(self.did.to_self_profile_string(builder));

let const_param_did = if let Some(const_param_did) = self.const_param_did {
let const_param_did = builder.def_id_to_string_id(const_param_did);
StringComponent::Ref(const_param_did)
} else {
StringComponent::Value("_")
};

let components = [
StringComponent::Value("("),
did,
StringComponent::Value(", "),
const_param_did,
StringComponent::Value(")"),
];

builder.profiler.alloc_string(&components[..])
}
}

impl<T0, T1> SpecIntoSelfProfilingString for (T0, T1)
where
T0: SpecIntoSelfProfilingString,
Expand Down