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

Support searching for and demangling symbols #62

Merged
merged 4 commits into from
Mar 23, 2023

Conversation

Mark-Simulacrum
Copy link
Member

This adds an API for demangling a stream of data which consists of a mix of mangled and non-mangled symbols. This is gated behind a new Cargo feature ("std") since the new API uses I/O traits and allocates memory.

This API is broadly useful for any downstream users that are processing streams with interleaved symbols (e.g., perf script output) and don't want to filter down ahead of time. Previously the best option for this (to my knowledge) was either shelling out to the rustfilt binary or copying an implementation much like it.

The implementation added in this commit is roughly 2x faster than that in the rustfilt binary today (on a test input I have 40 seconds -> 20 seconds); if this is accepted moving rustfilt to use this will probably make sense.

This adds an API for demangling a stream of data which consists of a mix
of mangled and non-mangled symbols. This is gated behind a new Cargo
feature ("std") since the new API uses I/O traits and allocates memory.

This API is broadly useful for any downstream users that are processing
streams with interleaved symbols (e.g., perf script output) and don't
want to filter down ahead of time. Previously the best option for this
(to my knowledge) was either shelling out to the `rustfilt` binary or
copying an implementation much like it.

The implementation added in this commit is roughly 2x faster than that
in the rustfilt binary today; if this is accepted moving rustfilt to use
this will make sense.
@Mark-Simulacrum
Copy link
Member Author

Fuzz target failure looks unrelated to this PR - probably upstream change? I ran the fuzzing locally and it seems to be fine.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Seems reasonable to me! Could you update the version of cargo-fuzz installed in CI? That should fix the fuzz failure.

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@Mark-Simulacrum
Copy link
Member Author

Alright, CI is passing.

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Ok, I'll leave that part up to you then as to whether to push the stream further down.

@Mark-Simulacrum
Copy link
Member Author

I'll probably look into optimizing further since waiting 20+ seconds for demangling isn't fun, but in any case I think we can probably merge this - did you want me to do merge + publish for you? I technically have the right access through infra but the ownership for this crate feels somewhat unclear today.

@alexcrichton
Copy link
Member

Sure yeah that'd be appreciated!

@Mark-Simulacrum Mark-Simulacrum merged commit 69fb82b into rust-lang:main Mar 23, 2023
@Mark-Simulacrum Mark-Simulacrum deleted the stream-api branch March 23, 2023 14:19
@Mark-Simulacrum
Copy link
Member Author

Alright, merged this and publish 0.1.22 with the new APIs. Thanks!

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