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

feat: Merge threads in live view #589

Merged
merged 2 commits into from
May 30, 2024
Merged

feat: Merge threads in live view #589

merged 2 commits into from
May 30, 2024

Conversation

mgmacias95
Copy link
Contributor

Issue number of the reported bug or feature request: #587

Describe your changes
This PR adds a new option in the live view called "merge threads" that summarizes all allocations in a single view. It also hides the buttons ">" and "<" because you can't move between threads in that moment.

In addition, I added some missing docs to the readme.

Testing performed
Manual testing + unit tests included in this PR

@codecov-commenter
Copy link

codecov-commenter commented May 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.92%. Comparing base (41248ed) to head (be0a687).
Report is 70 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
+ Coverage   92.55%   92.92%   +0.37%     
==========================================
  Files          91       92       +1     
  Lines       11304    11323      +19     
  Branches     1581     2076     +495     
==========================================
+ Hits        10462    10522      +60     
+ Misses        837      801      -36     
+ Partials        5        0       -5     
Flag Coverage Δ
cpp 92.92% <100.00%> (+6.98%) ⬆️
python_and_cython 92.92% <100.00%> (-2.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pablogsal pablogsal requested a review from godlygeek May 6, 2024 10:48
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

This seems like a great start, but I'm spotting lots of little issues as I try to use it.

For one, if the process is still running and output isn't paused, then threads seem to become un-merged again as soon as the next update happens.

For another, this disables the < and > keys, but not the left and right arrow keys, which do the same thing (as long as the grid isn't wide enough to scroll - if it is, they scroll the grid instead).

Also, if after the tracked process dies you use the > key to select a specific thread and then enable thread merging, the table doesn't get redrawn to include allocations from all threads, even though the header gets redrawn to say that allocations from all threads are being shown together.

Can you spend some time tracking down and fixing these issues?

README.md Outdated Show resolved Hide resolved
src/memray/reporters/tui.py Outdated Show resolved Hide resolved
@mgmacias95
Copy link
Contributor Author

Hello @godlygeek,

I plan working on this by the end of the week. Thank you for your review! :)

@mgmacias95
Copy link
Contributor Author

Hey @godlygeek,

So when I said I would work on this by the end of the week, I meant by the end of the day 😅 😂

I fixed the issues you pointed out. Can you please review it again?

Thanks!

@mgmacias95 mgmacias95 requested a review from godlygeek May 7, 2024 20:18
@godlygeek
Copy link
Contributor

It looks like, while it's in "merge threads" mode, the header winds up incorrectly showing a thread count again the first time a new thread is created after entering "merge threads" mode. See this screenshot, for instance:
image
You'll see that it shows "Thread 1 of 23" at the top, but "M Unmerge threads" at the bottom, so it's still in merge threads mode, but incorrectly showing a thread index and thread count.

Try using this test program:

import mmap
import threading
import time


def allocate():
    with mmap.mmap(-1, 1024 * 1024):
        time.sleep(1)


threads = [threading.Thread(target=allocate) for _ in range(3)]
for thread in threads:
    time.sleep(3)
    thread.start()

and pressing "m" to merge threads as soon as the TUI shows up. You'll see that the heading switches to saying "Displaying all threads." but as soon as a new thread shows up, it switches back to displaying a thread count.

@mgmacias95
Copy link
Contributor Author

Hello @godlygeek,

Thank you for your review again, I just pushed a commit fixing that problem. Can you take another look? Thanks!

Cheers!

@mgmacias95
Copy link
Contributor Author

Hello @godlygeek,

I have fixed some conflicts that appeared in 2f50fa7. Can you take a look again please? 🥺

Thanks!

This allows users to choose between seeing allocations for a single
thread at a time, or seeing allocations across all threads.

Signed-off-by: Marta Gomez Macias <[email protected]>
Allow users to opt out when they want to see what individual threads are
allocating. This is a better default, since it more closely matches what
the other reporters do, but still allows power users to opt in to
getting more information (and a busier UI).

Signed-off-by: Matt Wozniski <[email protected]>
Copy link
Contributor

@godlygeek godlygeek left a comment

Choose a reason for hiding this comment

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

I picked this up and tweaked it a small bit, but this was great work. (Even more so because you were trying to keep up to date with some of the hacks being applied in other PRs 😅)

Thanks so much for the contribution, @mgmacias95! 🎉

@godlygeek godlygeek enabled auto-merge (rebase) May 30, 2024 19:13
@godlygeek godlygeek merged commit 2ebca3f into bloomberg:main May 30, 2024
18 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.

3 participants