-
-
Notifications
You must be signed in to change notification settings - Fork 457
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
Darwin: scan thread information #546
Conversation
@cgzones looks good to me. Might be worth setting 'isThread = false;' in DarwinProcess_new instead of relying on the calloc there, for code clarity. If that's not preferred and calloc is OK - which is fine by me - there's some other initializations-to-zero in that function that could be removed. Also I wonder about the taskAccess field (not from your new code, but related to threads) - it seems like this field might cause incorrect behaviour if htop samples a process before it creates any threads - if it only creates threads later in its life, those threads would never be counted because this flag was set first time through. If thats the case, I think it could be safely removed - it appears to be an optimization only, AFAICS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I honestly don't really see a difference with this change.
I've added a quick screenshot of my local output on my macbook, where the executable a.out
contains two concurrently running pthreads. However, these are not visualized in any way.
For comparison, the same application built for Linux clearly shows the two running threads (highlighted in green):
darwin/DarwinProcess.c
Outdated
mach_msg_type_number_t thread_info_count = THREAD_BASIC_INFO_COUNT; | ||
ret = thread_info(thread_list[i], THREAD_BASIC_INFO, (thread_info_t)thinfo, &thread_info_count); | ||
mach_msg_type_number_t thread_info_count = THREAD_EXTENDED_INFO_COUNT; | ||
ret = thread_info(thread_list[i], THREAD_EXTENDED_INFO_COUNT, (thread_info_t)thinfo, &thread_info_count); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are you sure that the thinfo
struct is passed by copy and not by pointer-to? I've seen a couple of SO posts passing it in via (thread_info_t) &thinfo
: https://stackoverflow.com/a/33414855/4583130
the same actually applies to the similar logic above for obtaining the TASK_BASIC_INFO
.
Just a small tip: Press Esc once to hide the process list cursor. Also Tree Mode might be of help to group the threads optically below their parent. |
fcc37d8
to
92814d7
Compare
Please check if this also fixes #622. |
I checked out this lcoally and built it but I still don't see the threads, even running the tool with sudo. On MBP M2 Ventura 13.3 |
Have you checked that your configuration has display of userland threads activated. |
Yep I had unchecked "Hide userland process threads" and also tried toggling it on and off with (btw v3.2.2 also doesn't show threads per process, but I can see the total number of user threads under tasks) |
Okay, strange. I can confirm this is somewhat strange behaviour. @cgzones Can you check your patch again? |
2dc145c
to
4059f8e
Compare
Recent versions of MacOS limit the access to foreign threads via |
I also just compiled Not sure how relevant, the followings are broken:
here is a side-by-side comparison of |
Works on macOS 14.0 |
What do you mean it's too high? That's what I get. ![]() |
@cgzones Can you rebase? What'S missing to push this along? |
@emadpres I was talking about the width: Your PID have 7 digits but htop is fixed for 5 digits. |
d9f9237
to
d62f335
Compare
Inspired by: hishamhm/htop#848 Closes: htop-dev#542
d62f335
to
357bbe6
Compare
Testing: check all threads are displayed, no memory is leaked, tree glitches. (Only compile tested the latest rebase.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. No obvious memleaks.
Minor nitpick: Neither identifer_info_count
nor extended_info_count
are used or checked after their assignment through the call to thread_info
, but access to at least the first item of the returned arrays is assumed.
The output parameters |
Ah, I see. So basically buffer sizes. In that case, at least checking for "a whole item was returned" should suffice. But as mentioned, that is somewhat nitpicky. |
Any update on this? Would be great to see threads on Mac! |
@dundee: Did you test the cgzones:darwin_threads branch? |
@fasterit Yes, it works fine for me (using sudo). |
@cgzones @BenBE thanks for this PR! |
If your from-source build reports That aside: from the commit mentioned in the merge commit you can determine exactly when this landed, but I'd guess it's highly likely this was after 3.3.0 was released; thus this is probably not included in any release yet. Have a look at the milestone of this PR to check for which release it is planned. |
@BenBE apologies. i checked version wrong. my from-source build is
oh, i didn't notice that, but that's so nice. |
Origin: hishamhm/htop#848
Closes: #542
/cc @robaho @Karsten1987 @fasterit