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

Uniquely identify CGU's and accurately track their size and merges #89308

Open
1 of 3 tasks
Sl1mb0 opened this issue Sep 27, 2021 · 5 comments
Open
1 of 3 tasks

Uniquely identify CGU's and accurately track their size and merges #89308

Sl1mb0 opened this issue Sep 27, 2021 · 5 comments
Labels
A-codegen Area: Code generation C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Sl1mb0
Copy link
Contributor

Sl1mb0 commented Sep 27, 2021

Overview

This issue is in relation to optimizing codegen scheduling. To better profile codegen and see where/what we need to optimize, we need to get metrics for individual codegen units. We can uniquely identify CGU's using their names, so there is already a mechanism for tracking individual CGU's.

CGU Size

Tracking CGU size by itself should not be too hard; at some arbitrary point in time we determine the CGU's size and map this quantity to a CGU ID for later lookup. @Mark-Simulacrum mentioned that we should have support for knowing if a CGU is big just because, or because it absorbed other CGU's; and that this is something that should be displayed on perf.r-l.o. my guess is we can do this using a similar structure to what's already in place in SelfProfiler:

pub struct SelfProfiler {
    profiler: Profiler,
    event_filter_mask: EventFilter,

    string_cache: RwLock<FxHashMap<String, StringId>>,

    query_event_kind: StringId,
    generic_activity_event_kind: StringId,
    incremental_load_result_event_kind: StringId,
    incremental_result_hashing_event_kind: StringId,
    query_blocked_event_kind: StringId,
    query_cache_hit_event_kind: StringId,
    /*** NEW ***/
    codegen_unit_merge_event_kind: StringId,
}

However, as I understand it, this approach ignores tracking which CGU's have been merged together and just informs
us when and where a merge has occurred; shouldn't we also know what got merged? Or does using a CodegenUnitId solve this?

Lastly, it has been mentioned that the size metric is not currently accurate, and that the quantity of LLVM instructions
might not provide enough insight. Until I know more, I plan on using CGU instruction count as the size metric.

To Do

  • identify individual CGU's
  • Change size metric to reflect inst counts of CGU's
  • Add support for tracking which CGU's have been merged; and where and when it happened.

cc @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I don't think numeric IDs are a good idea, since keeping them stable across changes to the underlying structure is a little painful, and figuring out which module is which is probably also hard.

There's some code which generates CGU names already, e.g., around here. My hope is that some of that can be reused for our purposes here.

I think the steps here are roughly:

  • Verify if existing CGU name building works in terms of providing stable IDs (e.g., a name like crate::module, likely with some disambiguating component for the various kinds -- iirc, we have some notion of splitting a module based on public API and such)
  • If so, adjust self-profile to have an event kind for this -- something like CGU_NAMES and start emitting events when we create modules under that kind (if selected on the CLI) with the estimated size.
  • Edit the CGU merging code to emit a "merged_modules" or so event that has which modules were merged together in each step.

Perf can then take these newly added events and additional_data and display it appropriately.

cc @rust-lang/wg-compiler-performance on whether the steps above make sense -- I'm not super (recently) familiar with our measureme APIs and such, I hope I got these mostly right. Of course, can adjust if there's problems.

@Mark-Simulacrum
Copy link
Member

It might be that this wants to rely on support similar to the artifact size profiling work @rylev (IIRC) was doing, which is a separate table from events I think? Not sure.

@tgnottingham
Copy link
Contributor

I'm not sure there's an assumption being made here about this being so, but in case there is, I would just be careful of baking in the assumption that the initial CGU breakdown of two per module is the right approach. Maybe it is, but I just want to question it.

For example, you might imagine alternative CGU partitioning scheme in which each CGU is initially just the call graph of one root mono item. (That's a starting point where there would obviously be a lot of duplication between CGUs, but the merging algorithm would take care of that.) Not saying that's a good idea, but it could be.

I wish I had more to say than that at this point, but I just haven't thought about the problem enough.

@Mark-Simulacrum
Copy link
Member

I think we support an arbitrary amount of extra "args" put onto each event, so we should be fine in that regard. This format also need not be stable, fwiw, though once perf is consuming it we'd want to be careful with breakage.

I think more granular names should also integrate fine - I don't expect perf to parse those, just pass through to the UI, initially.

@michaelwoerister
Copy link
Member

The CGU name already is a unique identifier:

/// CGU names should fulfill the following requirements:
/// - They should be able to act as a file name on any kind of file system
/// - They should not collide with other CGU names, even for different versions
/// of the same crate.
///
/// Consequently, we don't use special characters except for '.' and '-' and we
/// prefix each name with the crate-name and crate-disambiguator.
///
/// This function will build CGU names of the form:
///
/// ```
/// <crate-name>.<crate-disambiguator>[-in-<local-crate-id>](-<component>)*[.<special-suffix>]
/// <local-crate-id> = <local-crate-name>.<local-crate-disambiguator>
/// ```
///
/// The '.' before `<special-suffix>` makes sure that names with a special
/// suffix can never collide with a name built out of regular Rust
/// identifiers (e.g., module paths).
pub fn build_cgu_name<I, C, S>(

If the compiler is invoked with -Zhuman-readable-cgu-names you get a nice, structured name. Otherwise you get a hash value of that nice structured name.

Also, the self-profiling infrastructure will soon grow some functionality for recording non-timing events: #87404.

Self-profiling events also can have an arbitrary number of string arguments. It should not be too hard to make the compiler record all the data you need and then write an analysis tool similar to the other tools in measureme.

@Sl1mb0 Sl1mb0 mentioned this issue Oct 5, 2021
@Dylan-DPC Dylan-DPC added A-codegen Area: Code generation C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-codegen Area: Code generation C-feature-request Category: A feature request, i.e: not implemented / a PR. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants