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

Jump to bottom

debuginfo: Simplify TypeMap used during LLVM debuginfo generation. #93644

Merged
merged 2 commits into from
Feb 25, 2022
Merged

debuginfo: Simplify TypeMap used during LLVM debuginfo generation. #93644

merged 2 commits into from
Feb 25, 2022

Conversation

michaelwoerister
Copy link
Member

This PR simplifies the TypeMap that is used in rustc_codegen_llvm::debuginfo::metadata. It was unnecessarily complicated because it was originally implemented when types were not yet normalized before codegen. So it did it's own normalization and kept track of multiple unnormalized types being mapped to a single unique id.

This PR is based on #93503, which is not merged yet.

The PR also removes the arena used for allocating string ids and instead uses InlinableString from the inlinable_string crate. That might not be the best choice, since that crate does not seem to be very actively maintained. The flexible-string crate would be an alternative.

r? @ghost

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Feb 4, 2022
@michaelwoerister michaelwoerister force-pushed the simpler-debuginfo-typemap branch from 2c3bdfb to 334c5d7 Compare February 4, 2022 14:29
@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 4, 2022
@bors
Copy link
Contributor

bors commented Feb 4, 2022

⌛ Trying commit 334c5d7 with merge 5bd0fa171f55db4cf16549b05876f000099019b7...

@michaelwoerister michaelwoerister added the A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) label Feb 4, 2022
@bors
Copy link
Contributor

bors commented Feb 4, 2022

☀️ Try build successful - checks-actions
Build commit: 5bd0fa171f55db4cf16549b05876f000099019b7 (5bd0fa171f55db4cf16549b05876f000099019b7)

@rust-timer
Copy link
Collaborator

Queued 5bd0fa171f55db4cf16549b05876f000099019b7 with parent 4e8fb74, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (5bd0fa171f55db4cf16549b05876f000099019b7): comparison url.

Summary: This benchmark run did not return any relevant results. 20 results were found to be statistically significant but too small to be relevant.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Feb 4, 2022
@michaelwoerister michaelwoerister force-pushed the simpler-debuginfo-typemap branch from 334c5d7 to 7746f15 Compare February 7, 2022 09:17
@michaelwoerister
Copy link
Member Author

Let's see what performance looks like without the small-string optimization.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 7, 2022
@bors
Copy link
Contributor

bors commented Feb 7, 2022

⌛ Trying commit 7746f15 with merge 67817846bea0a55c9b9a3b46cb3c92d4a3b4239e...

@bors
Copy link
Contributor

bors commented Feb 7, 2022

☀️ Try build successful - checks-actions
Build commit: 67817846bea0a55c9b9a3b46cb3c92d4a3b4239e (67817846bea0a55c9b9a3b46cb3c92d4a3b4239e)

@rust-timer
Copy link
Collaborator

Queued 67817846bea0a55c9b9a3b46cb3c92d4a3b4239e with parent 926e784, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (67817846bea0a55c9b9a3b46cb3c92d4a3b4239e): comparison url.

Summary: This benchmark run did not return any relevant results.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Feb 7, 2022
@michaelwoerister
Copy link
Member Author

Looks like the small-string optimization is not needed for keeping performance stable. Better to look into that separately.

This should be ready to review now.

r? rust-lang/compiler

@michaelwoerister michaelwoerister changed the title [draft] debuginfo: Simplify TypeMap used during LLVM debuginfo generation. debuginfo: Simplify TypeMap used during LLVM debuginfo generation. Feb 8, 2022
@oli-obk
Copy link
Contributor

oli-obk commented Feb 17, 2022

I think someone from @rust-lang/wg-llvm should probably look over it

@nagisa nagisa self-assigned this Feb 17, 2022
@nagisa
Copy link
Member

nagisa commented Feb 17, 2022

(assigned myself as a secondary reviewer so that it falls into my review queue)

@michaelwoerister
Copy link
Member Author

Maybe @wesleywiser can review this as part of rust-lang/compiler-team#482.

@michaelwoerister michaelwoerister force-pushed the simpler-debuginfo-typemap branch from 7746f15 to e72e639 Compare February 21, 2022 12:05
@michaelwoerister michaelwoerister mentioned this pull request Feb 22, 2022
Merged
@michaelwoerister michaelwoerister force-pushed the simpler-debuginfo-typemap branch from 81030fd to cd472b4 Compare February 24, 2022 16:23
@rust-log-analyzer

This comment has been minimized.

Sign in to view
wesleywiser
wesleywiser approved these changes Feb 24, 2022
View reviewed changes
Copy link
Member

@wesleywiser wesleywiser left a comment

Choose a reason for hiding this comment

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

r=me

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs Outdated Show resolved Hide resolved
@oli-obk
Copy link
Contributor

oli-obk commented Feb 24, 2022

r? @wesleywiser

@rust-highfive rust-highfive assigned wesleywiser and unassigned oli-obk and nagisa Feb 24, 2022
@michaelwoerister michaelwoerister force-pushed the simpler-debuginfo-typemap branch from cd472b4 to bb2059f Compare February 25, 2022 09:31
@michaelwoerister
Copy link
Member Author

Thanks for the review, @wesleywiser!

@bors r=wesleywiser

@bors
Copy link
Contributor

bors commented Feb 25, 2022

📌 Commit bb2059f has been approved by wesleywiser

@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 Feb 25, 2022
@bors
Copy link
Contributor

bors commented Feb 25, 2022

⌛ Testing commit bb2059f with merge 9b2a465...

@bors
Copy link
Contributor

bors commented Feb 25, 2022

☀️ Test successful - checks-actions
Approved by: wesleywiser
Pushing 9b2a465 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 25, 2022
@bors bors merged commit 9b2a465 into rust-lang:master Feb 25, 2022
@rustbot rustbot added this to the 1.61.0 milestone Feb 25, 2022
This was referenced Feb 25, 2022
Closed
Closed
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (9b2a465): comparison url.

Summary: This benchmark run shows 4 relevant improvements 🎉 to instruction counts.

  • Arithmetic mean of relevant improvements: -0.8%
  • Largest improvement in instruction counts: -0.9% on incr-unchanged builds of externs debug

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

@Mark-Simulacrum
Copy link
Member

Likely just noise -- externs is currently a little noisy (and we haven't absorbed that noisiness into our statistical estimation yet). Seems to be due to #93839, which makes me suspect this is related to PGO or inlining decisions of some kind. #94373 might help.

bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 15, 2022
@bors
Auto merge of rust-lang#94261 - michaelwoerister:debuginfo-types-refa…
0407030 
…ctor, r=wesleywiser

debuginfo: Refactor debuginfo generation for types

This PR implements the refactoring of the `rustc_codegen_llvm::debuginfo::metadata` module as described in MCP rust-lang/compiler-team#482.

In particular it
- changes names to use `di_node` instead of `metadata`
- uniformly names all functions that build new debuginfo nodes `build_xyz_di_node`
- renames `CrateDebugContext` to `CodegenUnitDebugContext` (which is more accurate)
- removes outdated parts from `compiler/rustc_codegen_llvm/src/debuginfo/doc.md`
- moves `TypeMap` and functions that work directly work with it to a new `type_map` module
- moves enum related builder functions to a new `enums` module
- splits enum debuginfo building for the native and cpp-like cases, since they are mostly separate
- uses `SmallVec` instead of `Vec` in many places
- removes the old infrastructure for dealing with recursion cycles (`create_and_register_recursive_type_forward_declaration()`, `RecursiveTypeDescription`, `set_members_of_composite_type()`, `MemberDescription`, `MemberDescriptionFactory`, `prepare_xyz_metadata()`, etc)
- adds `type_map::build_type_with_children()` as a replacement for dealing with recursion cycles
- adds many (doc-)comments explaining what's going on
- changes cpp-like naming for C-Style enums so they don't get a `enum$<...>` name (because the NatVis visualizer does not apply to them)
- fixes detection of what is a C-style enum because some enums where classified as C-style even though they have fields
- changes cpp-like naming for generator enums so that NatVis works for them
- changes the position of discriminant debuginfo node so it is consistently nested inside the top-level union instead of, sometimes, next to it

The following could be done in subsequent PRs:
- add caching for `closure_saved_names_of_captured_variables`
- add caching for `generator_layout_and_saved_local_names`
- fix inconsistent handling of what is considered a C-style enum wrt to debuginfo
- rename `metadata` module to `types`
- move common generator fields to front instead of appending them

This PR is based on rust-lang#93644 which is not merged yet.

Right now, the changes are all done in one big commit. They could be split into smaller commits but hopefully the list of changes above makes it tractable to review them as a single commit too.

For now: r? `@ghost` (let's see if this affects compile times)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wesleywiser wesleywiser wesleywiser approved these changes

Assignees

@wesleywiser wesleywiser

Labels
A-debuginfo Area: Debugging information in compiled programs (DWARF, PDB, etc.) merged-by-bors This PR was explicitly merged by bors. 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.
Projects
None yet
Milestone
1.61.0
Development

Successfully merging this pull request may close these issues.
9 participants
@michaelwoerister @rust-timer @bors @oli-obk @nagisa @rust-log-analyzer @Mark-Simulacrum @wesleywiser @rustbot