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

Trim executable name when getting function color #329

Merged
merged 2 commits into from
Jul 29, 2024

Conversation

mkeeter
Copy link
Contributor

@mkeeter mkeeter commented Jul 26, 2024

On illumos, the DTrace script prints the library or executable before the function in the stack trace, e.g.

libc.so.1`__fdsync+0xa
cru-ds-fdsync-v1`std::fs::File::sync_all::hcd4d0768a77cbc2e+0x14
cru-ds-fdsync-v1`<crucible_downstairs::extent_inner_raw::RawInner as crucible_downstairs::extent::ExtentInner>::flush::h68fcf1774758d74e+0x9c
cru-ds-fdsync-v1`crucible_downstairs::extent::Extent::flush::h0eac5b95dfa4f5f0+0x472
cru-ds-fdsync-v1`std::panicking::try::h7ba6611983f64757+0x4c
cru-ds-fdsync-v1`_$LT$rayon_core..job..HeapJob$LT$BODY$GT$$u20$as$u20$rayon_core..job..Job$GT$::execute::h60b9d586fc4f8a0f (.llvm.8052073739315931670)+0x46
cru-ds-fdsync-v1`rayon_core::registry::WorkerThread::wait_until_cold::haa78671c0e7aa9b1+0x50f
cru-ds-fdsync-v1`rayon_core::registry::ThreadBuilder::run::hf28d413d115bded0+0x398
cru-ds-fdsync-v1`std::sys_common::backtrace::__rust_begin_short_backtrace::hfcd6324a3e87fc1e+0x48
cru-ds-fdsync-v1`core::ops::function::FnOnce::call_once{{vtable.shim}}::hbfa4fc2e086997af+0xb2
cru-ds-fdsync-v1`std::sys::unix::thread::Thread::new::thread_start::h1783cbcbbf061711+0x29
libc.so.1`_thrp_setup+0x77
libc.so.1`_lwp_start

This means that tests like name.starts_with("core::") don't work.

This PR updates the Rust color selector to drop anything before the backtick, so that calls to starts_with(..) correctly classify functions in core / std / etc.

I only did it for Rust because that's what I'm using; if you want, I can make the equivalent change for Java and Python resolvers (which also use starts_with).

Thanks!

jonhoo
jonhoo previously approved these changes Jul 27, 2024
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

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

Just a small nit! I'm okay landing it for just Rust if we're not sure whether you end up with the same kinds of trace lines for other languages. If on the other hand you have evidence that the exact same thing applies to other languages, I think it's worthwhile changing for all of them!

Comment on lines 132 to 136
let name = if let Some(i) = name.find('`') {
&name[i + 1..]
} else {
name
};
Copy link
Owner

Choose a reason for hiding this comment

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

I think you can use split_once to great effect here:

Suggested change
let name = if let Some(i) = name.find('`') {
&name[i + 1..]
} else {
name
};
let name = name.split_once('`').map(|(_, after)| after).unwrap_or(name);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice, done in c41a5ba

@mkeeter
Copy link
Contributor Author

mkeeter commented Jul 29, 2024

I'm okay landing it for just Rust if we're not sure whether you end up with the same kinds of trace lines for other languages. If on the other hand you have evidence that the exact same thing applies to other languages, I think it's worthwhile changing for all of them!

It looks like Java and Python are profiled differently (using AsyncProfiler and austin respectively?), so I'm going to leave this as Rust-exclusive for now.

@jonhoo jonhoo merged commit 1c60840 into jonhoo:main Jul 29, 2024
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants