Feat: added caching schema for debuginfo type names #88898

Sl1mb0 · 2021-09-12T20:51:23Z

This is a WIP addressing #86431. This PR aims to accomplish two things:

Implements a caching schema to avoid redundant calls to compute_debuginfo_type_name().
Add profiling support for determining the amount of redundant computations that would be performed, if the caching schema were not implemented.
- As for how: I plan to just use counters to track how many times we call compute_debuginfo_type_name() and how many times we return a name for the cache. @rylev also suggested that determining how many individual memory allocations occur each time redundant work is performed, but I am unsure of how to go about this.

rust-highfive · 2021-09-12T20:51:26Z

r? @jackh726

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

Sl1mb0 · 2021-09-12T21:04:01Z

r? @michaelwoerister

bjorn3 · 2021-09-13T13:03:07Z

compiler/rustc_codegen_llvm/src/debuginfo/metadata.rs

@@ -106,6 +106,8 @@ pub struct TypeMap<'ll, 'tcx> {
    type_to_metadata: FxHashMap<Ty<'tcx>, &'ll DIType>,
    /// A map from types to `UniqueTypeId`. This is an N:1 mapping.
    type_to_unique_id: FxHashMap<Ty<'tcx>, UniqueTypeId>,
+    /// A map from `UniqueTypeId` to it's type-name string.
+    unique_id_to_type_name: FxHashMap<UniqueTypeId, String>,


A UniqueTypeId already contains the string. A Symbol is an interned string.

So we can just check the type_to_unique_id map instead right?

If there is a mapping, return the string by calling get_unique_type_id_as_string(), otherwise create the mapping.

This would also mean that the Symbol stored in UniqueTypeId is the result of compute_debuginfo_type_name(), but it isn't. the name is computed on this line in get_unique_type_id_of_type()

218 let key = self.unique_id_interner.intern(&unique_type_id)

My question is: if the resulting strings from the above line and compute_debuginfo_type_name() are the same, isn't compute_debuginfo_type_name() unnecessary in the first place?

Edit:

I am pretty sure that the string computed in compute_debuginfo_type_name() and the Symbol within a UniqueTypeId are not the same.

Right, seems like it is a hex encoded hash.

lqd · 2021-09-13T17:52:36Z

@bors try @rust-timer queue

rust-timer · 2021-09-13T17:52:37Z

Awaiting bors try build completion.

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

bors · 2021-09-13T17:52:44Z

⌛ Trying commit 03361c46dd5c34a4b73ec159438486c64c15310d with merge db7e55346581a97b2acf2430c0b7b42c9da07e0b...

bors · 2021-09-13T19:01:43Z

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

rust-timer · 2021-09-13T19:01:45Z

Queued db7e55346581a97b2acf2430c0b7b42c9da07e0b with parent b0ee495, future comparison URL.

rust-timer · 2021-09-13T20:41:23Z

Finished benchmarking commit (db7e55346581a97b2acf2430c0b7b42c9da07e0b): comparison url.

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

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

michaelwoerister · 2021-09-14T12:18:09Z

Thanks for the PR, @Sl1mb0!

I think for this to really make a performance difference the caching would have to be integrated into compute_debuginfo_type_name() itself, so that two separate calls for two types like foo::Bar1<foo::Baz> and foo::Bar2<foo::Baz> would be able to re-use foo::Baz. You can experiment with adding such a cache as a parameter to compute_debuginfo_type_name(). It would be something like a &mut FxHashMap<(Ty<'tcx>, bool), String> where the bool in the key stands for the qualified parameter.

Sl1mb0 · 2021-09-16T18:55:28Z

@michaelwoerister Do you have any suggestions on where caching should occur?

michaelwoerister · 2021-09-17T07:59:34Z

The cache could live in the CrateDebugContext, I think.

rust/compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

Lines 59 to 72 in e2750ba

    
           pub struct CrateDebugContext<'a, 'tcx> { 
        
               llcontext: &'a llvm::Context, 
        
               llmod: &'a llvm::Module, 
        
               builder: &'a mut DIBuilder<'a>, 
        
               created_files: RefCell<FxHashMap<(Option<String>, Option<String>), &'a DIFile>>, 
        
               created_enum_disr_types: RefCell<FxHashMap<(DefId, Primitive), &'a DIType>>, 
        
               type_map: RefCell<TypeMap<'a, 'tcx>>, 
        
               namespace_map: RefCell<DefIdMap<&'a DIScope>>, 
        
               // This collection is used to assert that composite types (structs, enums, 
        
               // ...) have their members only set once: 
        
               composite_types_completed: RefCell<FxHashSet<&'a DIType>>, 
        
           }

You can try to add a type_name_cache: RefCell<FxHashMap<(Ty<'tcx>, bool), String>> there and see if you run into any problems.

bors · 2021-09-17T21:37:26Z

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

Sl1mb0 · 2021-09-18T14:18:51Z

So I've changed a few things; but now the compiler is panicking due to a BorrowMut error, and I am unsure why.

My guess is that it's because I am passing around a mutable reference to the cache, as opposed to passing around a RefCell, and attempting to borrow from that RefCell.

The changes in these commits are:

Moved type_name_cache up to the CrateDebugContext
Add type_name_cache arg to compute_debuginfo_type_name() and it's helper functions.
Upon entry into push_debuginfo_type_name() check if we have computed this name before, compute it if we haven't, and return the computed name.

Sl1mb0 · 2021-09-19T03:36:56Z

I changed &mut to &RefCell, and everything compiles. However debuginfo/pretty-slices.rs fails when running ./x.py test debuginfo.

stdout:

failures:

---- [debuginfo-lldb] debuginfo/pretty-slices.rs stdout ----
NOTE: compiletest thinks it is using LLDB version 1205
NOTE: compiletest thinks it is using LLDB without native rust support

error: line not found in debugger output:  (&mut [i32]) $1 = size=4 { [0] = 2 [1] = 3 [2] = 5 [3] = 7 }
status: exit status: 0
command: "/usr/bin/python3" "/Users/t1mb0sl1c3/Developer/rust-lang/rust/src/etc/lldb_batchmode.py" "/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/a" "/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/pretty-slices.debugger.script"
stdout:
------------------------------------------
LLDB batch-mode script
----------------------
Debugger commands script is '/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/pretty-slices.debugger.script'.
Target executable is '/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/a'.
Current working directory is '/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler'
Creating a target for '/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/a'
settings set auto-confirm true

version
lldb-1205.0.27.3 Apple Swift version 5.4 (swiftlang-1205.0.26.9 clang-1205.0.19.55) 
command script import /Users/t1mb0sl1c3/Developer/rust-lang/rust/./src/etc/lldb_lookup.py
type synthetic add -l lldb_lookup.synthetic_lookup -x '.*' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)String$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^&(mut )?str$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^&(mut )?\[.+\]$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(std::ffi::([a-z_]+::)+)OsString$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)Vec<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)VecDeque<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)BTreeSet<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)BTreeMap<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(std::collections::([a-z_]+::)+)HashMap<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(std::collections::([a-z_]+::)+)HashSet<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)Rc<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(alloc::([a-z_]+::)+)Arc<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(core::([a-z_]+::)+)Cell<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(core::([a-z_]+::)+)Ref<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(core::([a-z_]+::)+)RefMut<.+>$' --category Rust
type summary add -F lldb_lookup.summary_lookup  -e -x -h '^(core::([a-z_]+::)+)RefCell<.+>$' --category Rust
type category enable Rust

breakpoint set --file 'pretty-slices.rs' --line 45
DEBUG: breakpoint added, id = 1
Breakpoint 1: where = a`pretty_slices::main::h926e671f9dfe1c03 + 155 at pretty-slices.rs:45:5, address = 0x0000000100002cab 
DEBUG: registering breakpoint callback, id = 1
Error while trying to register breakpoint callback, id = 1, message = error: could not get num args: can't find callable: breakpoint_callback

run
Process 39336 stopped * thread #1, queue = 'com.apple.main-thread', stop reason = breakpoint 1.1 frame #0: 0x0000000100002cab a`pretty_slices::main::h926e671f9dfe1c03 at pretty-slices.rs:45:5 42 let mut mut_str_slice_buffer = String::from("mutable string slice"); 43 let mut_str_slice: &mut str = mut_str_slice_buffer.as_mut_str(); 44 -> 45 b(); // #break ^ 46 } Target 0: (a) stopped. Process 39336 launched: '/Users/t1mb0sl1c3/Developer/rust-lang/rust/compiler/build/x86_64-apple-darwin/test/debuginfo/pretty-slices.lldb/a' (x86_64) 
print slice
(&[i32]) $0 = size=3 { [0] = 0 [1] = 1 [2] = 2 } 
print mut_slice
(&mut &[i32]) $1 = { data_ptr = 0x00007ffeefbff288 length = 4 } 
print str_slice
(&str) $2 = "string slice" { data_ptr = 0x0000000100003e58 "string slicemutable string slicecalled `Result::unwrap()` on an `Err` value" length = 12 } 
print mut_str_slice
(&mut &str) $3 = { data_ptr = 0x0000000100606070 "mutable string slice" length = 20 } 
quit


------------------------------------------
stderr:
------------------------------------------

------------------------------------------



failures:
    [debuginfo-lldb] debuginfo/pretty-slices.rs

test result: FAILED. 1 passed; 1 failed; 131 ignored; 0 measured; 0 filtered out; finished in 4.69s

Some tests failed in compiletest suite=debuginfo mode=debuginfo host=x86_64-apple-darwin target=x86_64-apple-darwin

michaelwoerister · 2021-09-22T08:50:07Z

Hm, that looks like a legitimate error: (&mut [i32]) vs (&mut &[i32]). I'll try to take a look tomorrow. It looks like you are on the right track though otherwise.

Meanwhile, if you clean up the merge conflicts we can run some performance tests via CI.

michaelwoerister

I left a few comments and might have found the reason for test failures. Give it a try :)

compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs

michaelwoerister · 2021-09-23T13:41:52Z

compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

@@ -438,12 +438,11 @@ impl DebugInfoMethods<'tcx> for CodegenCx<'ll, 'tcx> {
            substs: SubstsRef<'tcx>,
            name_to_append_suffix_to: &mut String,
        ) -> &'ll DIArray {
-            let type_name_cache = &mut *debug_context(cx).type_name_cache.borrow_mut();


I think holding the RefCell "lock" here for longer than just the type_names::push_generic_params() call might have been the reason why you saw the BorrowMut panics because the type_metadata() call farther below will also try to lock the cache.

michaelwoerister · 2021-09-23T19:30:14Z

@bors try @rust-timer queue

rust-timer · 2021-09-23T19:30:16Z

Awaiting bors try build completion.

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

bors · 2021-09-23T19:30:23Z

⌛ Trying commit fdd211a with merge 7cd33b678f6735d7b26fb78172256d815cb27b33...

bors · 2021-09-23T20:54:50Z

☀️ Try build successful - checks-actions
Build commit: 7cd33b678f6735d7b26fb78172256d815cb27b33 (7cd33b678f6735d7b26fb78172256d815cb27b33)

rust-timer · 2021-09-23T20:54:51Z

Queued 7cd33b678f6735d7b26fb78172256d815cb27b33 with parent bf64232, future comparison URL.

rust-timer · 2021-09-24T01:26:28Z

Finished benchmarking commit (7cd33b678f6735d7b26fb78172256d815cb27b33): comparison url.

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

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

michaelwoerister

It looks like this doesn't improve performance much so far. We can try to improve things a little more by avoiding some string data allocations as described below. But I think that's as far as we can get as long as we don't find another way of dealing with recursive types.

michaelwoerister · 2021-10-01T09:50:53Z

compiler/rustc_codegen_llvm/src/debuginfo/mod.rs

@@ -62,6 +63,7 @@ pub struct CrateDebugContext<'a, 'tcx> {
    builder: &'a mut DIBuilder<'a>,
    created_files: RefCell<FxHashMap<(Option<String>, Option<String>), &'a DIFile>>,
    created_enum_disr_types: RefCell<FxHashMap<(DefId, Primitive), &'a DIType>>,
+    type_name_cache: RefCell<FxHashMap<(Ty<'tcx>, bool), String>>,


Replace String with Rc<str> so that we can cheaply copy strings. A String can easily converted into an Rc<str> as described here: https://doc.rust-lang.org/std/string/struct.String.html#impl-From%3CString%3E-3

compiler/rustc_codegen_ssa/src/debuginfo/type_names.rs

nagisa · 2021-10-04T22:06:55Z

@bors try @rust-timer queue

rust-timer · 2021-10-04T22:06:56Z

Awaiting bors try build completion.

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

bors · 2021-10-04T22:07:22Z

⌛ Trying commit 353e42e with merge a35f2fa0b1709e3bffed26d72fff54832568660f...

bors · 2021-10-04T23:29:38Z

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

rust-timer · 2021-10-04T23:29:39Z

Queued a35f2fa0b1709e3bffed26d72fff54832568660f with parent 175b8db, future comparison URL.

rust-timer · 2021-10-05T01:06:22Z

Finished benchmarking commit (a35f2fa0b1709e3bffed26d72fff54832568660f): comparison url.

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

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

michaelwoerister · 2021-10-11T09:20:47Z

Well, it looks like there are no real performance improvements to be had here, unfortunately. Given that the changes add some complexity with no apparent gain, I'd opt for closing the PR (unless you have other ideas that we haven't explored yet).

Sl1mb0 · 2021-10-11T11:48:56Z

The only thing I can think of (without making a huge change) is that the mapping is too broad. The only field used from a Ty<'tcx> is it's TyKind;

If we get two different types, and they are each the same kind, we are computing that string for each type.

I'll try it and if there's still no improvements I'll close the PR.

Sl1mb0 · 2021-10-11T14:25:00Z

Seems using a TyKind is discouraged; closing.

michaelwoerister · 2021-10-13T08:15:16Z

I hope you still had an interesting learning experience working on this, @Sl1mb0!

rust-highfive assigned jackh726 Sep 12, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 12, 2021

rust-highfive assigned michaelwoerister and unassigned jackh726 Sep 12, 2021

This comment has been minimized.

Sign in to view

bjorn3 reviewed Sep 13, 2021

View reviewed changes

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 13, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 13, 2021

Sl1mb0 force-pushed the cache-debuginfo-type-names branch from 03361c4 to 69e911c Compare September 15, 2021 22:25

Sl1mb0 force-pushed the cache-debuginfo-type-names branch from a206b3a to cdd2ba2 Compare September 22, 2021 14:56

This comment has been minimized.

Sign in to view

michaelwoerister reviewed Sep 23, 2021

View reviewed changes

Feat: added caching schema for debuginfo type names

65b9e51

Sl1mb0 force-pushed the cache-debuginfo-type-names branch from 7588659 to fdd211a Compare September 23, 2021 18:16

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 23, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Sep 24, 2021

michaelwoerister reviewed Oct 1, 2021

View reviewed changes

Changed String to Rc<str> and added extra cache check

13a502d

This comment has been minimized.

Sign in to view

Changed formatting

353e42e

rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 4, 2021

rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Oct 5, 2021

Sl1mb0 closed this Oct 11, 2021

Sl1mb0 deleted the cache-debuginfo-type-names branch October 16, 2021 19:50

michaelwoerister mentioned this pull request Feb 9, 2023

Investigate performance impact of debuginfo typename generation #86431

Closed

Feat: added caching schema for debuginfo type names #88898

Feat: added caching schema for debuginfo type names #88898

Conversation

Sl1mb0 commented Sep 12, 2021

rust-highfive commented Sep 12, 2021

Sl1mb0 commented Sep 12, 2021

This comment has been minimized.

bjorn3 Sep 13, 2021

Choose a reason for hiding this comment

Sl1mb0 Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

bjorn3 Sep 13, 2021

Choose a reason for hiding this comment

lqd commented Sep 13, 2021

rust-timer commented Sep 13, 2021

bors commented Sep 13, 2021

bors commented Sep 13, 2021

rust-timer commented Sep 13, 2021

rust-timer commented Sep 13, 2021

michaelwoerister commented Sep 14, 2021

Sl1mb0 commented Sep 16, 2021

michaelwoerister commented Sep 17, 2021

bors commented Sep 17, 2021

Sl1mb0 commented Sep 18, 2021

Sl1mb0 commented Sep 19, 2021

michaelwoerister commented Sep 22, 2021

This comment has been minimized.

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Sep 23, 2021

Choose a reason for hiding this comment

michaelwoerister commented Sep 23, 2021

rust-timer commented Sep 23, 2021

bors commented Sep 23, 2021

bors commented Sep 23, 2021

rust-timer commented Sep 23, 2021

rust-timer commented Sep 24, 2021

michaelwoerister left a comment

Choose a reason for hiding this comment

michaelwoerister Oct 1, 2021

Choose a reason for hiding this comment

This comment has been minimized.

nagisa commented Oct 4, 2021

rust-timer commented Oct 4, 2021

bors commented Oct 4, 2021

bors commented Oct 4, 2021

rust-timer commented Oct 4, 2021

rust-timer commented Oct 5, 2021

michaelwoerister commented Oct 11, 2021

Sl1mb0 commented Oct 11, 2021

Sl1mb0 commented Oct 11, 2021 • edited Loading

michaelwoerister commented Oct 13, 2021

Sl1mb0 Sep 13, 2021 •

edited

Loading

Sl1mb0 commented Oct 11, 2021 •

edited

Loading