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

Increase code sharing between py-spy and rbspy #350

Open
acj opened this issue Jan 1, 2022 · 3 comments
Open

Increase code sharing between py-spy and rbspy #350

acj opened this issue Jan 1, 2022 · 3 comments

Comments

@acj
Copy link
Member

acj commented Jan 1, 2022

I've been thinking about refactoring the initialize and address_finder code to make it easier to test, fully migrate to goblin where we can (#50), etc. py-spy has a cleaner and more mature solution to the problems that those modules solve. Maybe we could share more code?

There was an earlier effort in this direction (#180) that seemed healthy for both projects.

Things that seem straightforward to share:

  • Code for locating binaries, shared libs, etc. (PythonProcessInfo)
  • Version detection and parsing (varies only by symbol name?)
  • Code that uses goblin to parse binaries (binary_parser.rs)
  • Containerized environment handling

And maybe also:

  • Irregular sampling interval logic
  • Top-like viewer
  • Trait for stack traces
  • Visualization

The first group falls under a kind of "process info discovery" umbrella that doesn't vary much between the projects. We need to find different symbols for ruby vs. python, but the code for finding them—and dealing with any OS quirks—could probably be the same. My plan has been to build a RubySpy struct (based on PythonSpy) and see what happens. I'm hoping to get a first version of that working this week. Much of the work has effectively been s/python/ruby, which seems promising.

Does factoring out a spytools crate still feel like a good move? Should they be a set of smaller, more focused crates instead? Could these responsibilities be moved from py-spy into remoteprocess? (I'm open to suggestions!)

cc @benfred

@benfred
Copy link
Collaborator

benfred commented Jan 1, 2022

I think sharing more code is a great idea - and is something I've been meaning to do for a while but haven't had time.

I'm wondering if we want to merge all of remoteprocess into this spytools crate ? I think having one more fully fleshed out crate will make life a little easier for us -

@acj
Copy link
Member Author

acj commented Jan 1, 2022

That sounds good to me. I think my first refactor attempt will be pretty rough (not merge-worthy), but it should give us a sense of what's possible. I'll ping you here when that's done so that we can think through next steps.

@acj
Copy link
Member Author

acj commented Jan 16, 2022

WIP branch here: https://github.com/acj/rbspy/tree/spytools-crate

So far, I've moved PythonProcessInfo (now ProcessInfo) and BinaryParser into the new crate (just a subdirectory right now), and they work pretty well. I was able to delete a lot of scruffy address lookup code. The main compatibility snag is that there's a list of symbols (get_windows_symbols), a handful of regexes (is_lib), and a hard coded name or two (is_framework) that we'll need to parameterize somehow if py-spy and rbspy are going to share the code.

To keep it simple for the calling code, I was thinking about defining a trait in the spytools crate that has a method for each bit of info that varies between python and ruby, and then adding a generic parameter to ProcessInfo::new that implements that trait. Not sure whether the trait implementations should live in spytools or in the respective spy repos. If you know of a cleaner way, or if that seems like a bad idea, let me know.

Once the dust settles on ProcessInfo, I'll take a look at merging remoteprocess into the crate, and then I'll put up a PR for feedback.

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

No branches or pull requests

2 participants