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

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

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

Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented May 5, 2024

Codecov Report

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

Project coverage is 92.93%. Comparing base (41248ed) to head (807f71b).
Report is 47 commits behind head on main.

Files Patch % Lines
src/memray/reporters/_textual_hacks.py 83.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #589      +/-   ##
==========================================
+ Coverage   92.55%   92.93%   +0.38%     
==========================================
  Files          91       92       +1     
  Lines       11304    11307       +3     
  Branches     1581     2076     +495     
==========================================
+ Hits        10462    10508      +46     
+ Misses        837      799      -38     
+ Partials        5        0       -5     
Flag Coverage Δ
cpp 92.93% <98.83%> (+6.99%) ⬆️
python_and_cython 92.93% <98.83%> (-2.79%) ⬇️

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.

Signed-off-by: Marta Gomez Macias <[email protected]>
@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! :)

Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
Without this code, you are able to keep moving among
all threads while in merged mode. Once you go back to
the regular mode, you won't go back to where you left
but where you've been moving while in merged mode.

This is a confusing behaviour and this commit blocks it.

Signed-off-by: Marta Gomez Macias <[email protected]>
The merge threads attribute was not watched by the
allocation table widget, so changing it made no
effect. When the app was connected, the table was
updated because a new snapshot was available, not
because the merge threads attribute change was
detected either.

This commit fixes that behaviour

Signed-off-by: Marta Gomez Macias <[email protected]>
@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
Signed-off-by: Marta Gomez Macias <[email protected]>
@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!

Signed-off-by: Marta Gomez Macias <[email protected]>
@mgmacias95
Copy link
Contributor Author

Hello @godlygeek,

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

Thanks!

Signed-off-by: Marta Gomez Macias <[email protected]>
Signed-off-by: Marta Gomez Macias <[email protected]>
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.

None yet

3 participants