Hide idle threads #1218
Hide idle threads #1218
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1218 +/- ##
=========================================
+ Coverage 75.88% 75.9% +0.02%
=========================================
Files 144 144
Lines 9354 9364 +10
Branches 2316 2318 +2
=========================================
+ Hits 7098 7108 +10
Misses 2013 2013
Partials 243 243
Continue to review full report at Codecov.
|
|
Thanks, the code was very clear indeed. Looks good to me! |
| } | ||
| return false; | ||
|
|
||
| return true; |
brisad
Aug 25, 2018
Member
Nit: This is super minor since the comment above points out that the first branch will likely never happen, and the scenario I'm pointing out here is even less likely to happen.
But, if maxActiveStackCount is reduced in the loop, it will only be acted on in the second branch, and so if all remaining iterations in the loop enters the first branch (that is, all remaining samples are filtered), it is possible that the threshold could be breached as the limit shrinks, but it never hits the return false since the loop is exited before that. Replacing this line with return activeStackCount <= maxActiveStackCount would account for that scenario.
Nit: This is super minor since the comment above points out that the first branch will likely never happen, and the scenario I'm pointing out here is even less likely to happen.
But, if maxActiveStackCount is reduced in the loop, it will only be acted on in the second branch, and so if all remaining iterations in the loop enters the first branch (that is, all remaining samples are filtered), it is possible that the threshold could be breached as the limit shrinks, but it never hits the return false since the loop is exited before that. Replacing this line with return activeStackCount <= maxActiveStackCount would account for that scenario.
gregtatum
Aug 27, 2018
Author
Member
Ah, nice catch!
Ah, nice catch!
| /** | ||
| * Determine if a thread is idle, so that it can be hidden. It is really annoying for an | ||
| * end user to load a profile full of empty and idle threads. | ||
| * end user to load a profile full of empty and idle threads. This function goes through | ||
| * all of the samples in the thread, and sees if some large percentage of them are idle. |
julienw
Aug 27, 2018
Contributor
question: does that mean that older profiles that don't have this category won't have the logic applied to them?
Are we OK with this?
question: does that mean that older profiles that don't have this category won't have the logic applied to them?
Are we OK with this?
gregtatum
Aug 27, 2018
Author
Member
There are two cases where this code is run:
- New profile captures.
- Profiles viewed with no existing URL encoded viewing information.
For case 1. this will probably affect people profiling release, which Bug 1462784 rides the trains here in a few days, so I don't think this is really an issue.
For case 2. This could potentially affect looking at older Talos profiles, which seems like a smaller percentage of viewers, but most profiles should have URL information already associated with them.
I am OK with this, as it makes the code we maintain simpler, with only a few end-users potentially affected. What do you think @julienw?
There are two cases where this code is run:
- New profile captures.
- Profiles viewed with no existing URL encoded viewing information.
For case 1. this will probably affect people profiling release, which Bug 1462784 rides the trains here in a few days, so I don't think this is really an issue.
For case 2. This could potentially affect looking at older Talos profiles, which seems like a smaller percentage of viewers, but most profiles should have URL information already associated with them.
I am OK with this, as it makes the code we maintain simpler, with only a few end-users potentially affected. What do you think @julienw?
julienw
Aug 27, 2018
Contributor
got it, I didn't think (I should think more :) ) that most profiles actually have the information in the URL. So good for me :)
got it, I didn't think (I should think more :) ) that most profiles actually have the information in the URL. So good for me :)
This PR changes the way we hide idle threads. With the new category information, we can now know if a sample is idle or not. This change removes the previous guesses about not having RefreshDriverTicks in a main thread and generalizes the notion of an idle thread, so that it works for all sorts of threads.
Here is an example profile hash: https://deploy-preview-1218--perf-html.netlify.com/public/9123fab7068c9fa242d79396f257b614bcd67bce
@brisad, I pinged you for review on this. I don't think you've specifically looked at this part of the code before, but I think it should be understandable for what is going on.