bloomberg / memray

Memray is a memory profiler for Python
https://bloomberg.github.io/memray/
Apache License 2.0
13.36k stars 397 forks source link

Print the thread name in the live TUI #562

Closed pablogsal closed 5 months ago

pablogsal commented 8 months ago

To allow users to determine thread in a multithreaded application corresponds to the "Thread X of Y" number displayed in the live view, include the thread name that we collect in the TUI.

Closes: #561

pablogsal commented 8 months ago

Not really a fan of having pretty_thread_name and thread_name. Also, I think this PR makes the output nicer but it doesn't really solve that is requested in https://github.com/bloomberg/memray/issues/561 yet, so I would prefer to not merge this until we find a way to solve that as well.

godlygeek commented 8 months ago

Not really a fan of having pretty_thread_name and thread_name.

Me neither, but I don't think pretty_thread_name should be an attribute of the record at all. It's obscuring the actual thread name of the record by doing formatting, and presentation should be a concern of the reporter, rather than something exposed as an attribute of the record.

If you'd like me to revert the changes to have both pretty_thread_name and thread_name, I could, but the end result would be that the TUI has to parse the thread name out of the record.thread_name by doing something like

thread_name = record.thread_name
thread_name = thread_name[thread_name.find("(") : thread_name.rfind(")") + 1]

which, yuck.

Alternatively (and I think preferably), I could push forward with my cleanup by removing pretty_thread_name from the record and factoring it into a def format_thread_name(record) -> str: function in reporters/__init__.py or a new reporters/common.py, and update our reporters to get it from there.

I think this PR makes the output nicer but it doesn't really solve that is requested in #561 yet

It doesn't, but it does display the thread name when we have the thread name, which is more than never. It also makes it so that the only piece missing for solving #561 is for us to capture the Python thread name in the tracker as well.

I would prefer to not merge this until we find a way to solve that as well

Well, flipping this to draft until we decide, but it seems to me that this is already a reasonably sized self-contained PR that moves the ball forward and displays information that we already collect and which can be useful to people using the TUI.

I think #561 can really be broken down into 2 related work items:

  1. capture Python thread names as well as OS thread names
  2. displaying the best name we have for each thread on the TUI

We need both pieces, and I don't think both need to be in the same PR (or even the same release). This is already useful today in applications that use pthread_setname_np or prctl(PR_SET_NAME), and some Python libraries do (confluent_kafka does, for instance).

codecov-commenter commented 8 months ago

Codecov Report

Attention: Patch coverage is 97.87234% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 93.04%. Comparing base (41248ed) to head (378be7f). Report is 72 commits behind head on main.

Files Patch % Lines
src/memray/_memray/tracking_api.cpp 86.66% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #562 +/- ## ========================================== + Coverage 92.55% 93.04% +0.49% ========================================== Files 91 94 +3 Lines 11304 11407 +103 Branches 1581 2089 +508 ========================================== + Hits 10462 10614 +152 + Misses 837 793 -44 + Partials 5 0 -5 ``` | [Flag](https://app.codecov.io/gh/bloomberg/memray/pull/562/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | Coverage Δ | | |---|---|---| | [cpp](https://app.codecov.io/gh/bloomberg/memray/pull/562/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `93.04% <97.87%> (+7.10%)` | :arrow_up: | | [python_and_cython](https://app.codecov.io/gh/bloomberg/memray/pull/562/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg) | `93.04% <97.87%> (-2.68%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=bloomberg#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

godlygeek commented 8 months ago

Alternatively (and I think preferably), I could push forward with my cleanup by removing pretty_thread_name from the record and factoring it into a def format_thread_name(record) -> str: function in reporters/__init__.py or a new reporters/common.py, and update our reporters to get it from there.

I've done this (last commit on this PR)

I think #561 can really be broken down into 2 related work items:

  1. capture Python thread names as well as OS thread names
  2. displaying the best name we have for each thread on the TUI

I've implemented item 1 (second to last commit on this PR), using the descriptor-based plan we discussed offline earlier today. It turned out to be a bit more involved than it seemed at first glance, because we needed to worry about names being set before a thread is started, or one thread's name being set from a different thread. It turned out not to be too tough to support both of those things, though.

Before you ask: there's a C-style cast in this PR to convert a pthread_t to a uint64_t. I couldn't figure out any other way to do this conversion:

I stopped wrestling with it after a while and just switched to a C-style cast, so that it reinterprets when we need reinterpreting and statically casts when we need a conversion between integer types.