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

Output can be truncated #3

Closed
wiltzius opened this issue Feb 14, 2019 · 7 comments
Closed

Output can be truncated #3

wiltzius opened this issue Feb 14, 2019 · 7 comments

Comments

@wiltzius
Copy link
Contributor

The writer thread is terminated without checking if its finished processing the event queue.

I sent a PR that addresses this: #2

@kwlzn
Copy link
Owner

kwlzn commented Feb 14, 2019

thanks for filing! closed in #2 - I'll cut a new pypi release to consume this when I have a chance.

@kwlzn kwlzn closed this as completed Feb 14, 2019
@kwlzn kwlzn mentioned this issue Feb 14, 2019
@wiltzius
Copy link
Contributor Author

Thanks! And thanks for writing this script.

I worked on the Chrome team that built about:tracing a few years ago, but I work mostly in Python now -- I always wanted to have a go at writing a tracing profiler for Python but by the time I finally got around to it you already had! I appreciate it.

I see so many posts of people mucking with flamegraphs etc; this method is really high overhead but the results are much easier to read and use IMHO.

@kwlzn
Copy link
Owner

kwlzn commented Feb 15, 2019

glad you're finding it useful.

this method is really high overhead but the results are much easier to read and use IMHO.

totally - this started out largely as a toy for basic larger-grained perf analysis where we were previously using elapsed timing in logs. I'd always intended to circle back and port the trace function to e.g. rust for lower overhead.

@wiltzius
Copy link
Contributor Author

wiltzius commented Feb 15, 2019 via email

@kwlzn
Copy link
Owner

kwlzn commented Feb 16, 2019

Is the best way to do that implement the trace function as the extension, and then still use sys.setprofile to install it?

as far as I know, yes. I believe it should be as simple as passing in a cffi function handle to that API - which should massively improve the trace call overhead vs a python def'd function.

I think it'd be cool to leverage something like https://github.com/getsentry/milksnake to have an in-line rust binary build + CFFI + sys.setprofile. rust is great to marry with python because of no GC/minimal runtime overhead, speed, modern dev toolchain etc.

@wiltzius
Copy link
Contributor Author

I gave this a go over the weekend. I'm unfamiliar with Rust and couldn't figure out the story for Python types in Rust (there's an issue on the Milksnake repo asking about this without much of a pointer; PyO3 also seemed a little experimental), so I just used pybind11 since it seemed well supported and this is my first binary Python extension.

Here's my first very barebones take that does at least work:

https://github.com/wiltzius/pytracing-cpp

It still needs a lot (eg. threaded output, integration with a context manager on the Python side, and it does a lot of unnecessary string allocation), but curious what you think. If this is a direction you're interested in taking things I can work toward a branch of this repo with this alternative implementation; otherwise I can do this on a fork.

@wiltzius
Copy link
Contributor Author

(advice on good patterns for writing C++ Python extensions particularly welcome, again I haven't built one before and there isn't a ton of good info out there. I'm in particular unclear about what the implication of forking a thread in this code is, how to avoid the GIL if forking, and whether it's a good idea to e.g. join the writer thread in the main Profiler object destructor).

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