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

Hide idle threads #1218

Merged
merged 2 commits into from Aug 27, 2018

Conversation

Projects
None yet
3 participants
@gregtatum
Member

gregtatum commented Aug 23, 2018

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.

@gregtatum gregtatum requested a review from brisad Aug 23, 2018

@codecov

This comment has been minimized.

codecov bot commented Aug 23, 2018

Codecov Report

Merging #1218 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            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
Impacted Files Coverage Δ
src/profile-logic/profile-data.js 81.04% <ø> (ø) ⬆️
src/actions/receive-profile.js 72.99% <ø> (+0.72%) ⬆️
src/profile-logic/tracks.js 93.06% <100%> (+0.32%) ⬆️
src/test/fixtures/profiles/make-profile.js 96.53% <100%> (+0.02%) ⬆️
src/utils/unique-string-array.js 85% <0%> (-10%) ⬇️
src/components/timeline/StackGraph.js 96.8% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b1345a...408fd0e. Read the comment docs.

@gregtatum gregtatum force-pushed the gregtatum:hide-idle-threads branch from 6858b93 to bde8161 Aug 23, 2018

@brisad

brisad approved these changes Aug 25, 2018

Thanks, the code was very clear indeed. Looks good to me!

}
return false;

return true;

This comment has been minimized.

@brisad

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.

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

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.

This comment has been minimized.

@julienw

julienw Aug 27, 2018

Collaborator

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?

This comment has been minimized.

@gregtatum

gregtatum Aug 27, 2018

Member

There are two cases where this code is run:

  1. New profile captures.
  2. 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?

This comment has been minimized.

@julienw

julienw Aug 27, 2018

Collaborator

got it, I didn't think (I should think more :) ) that most profiles actually have the information in the URL. So good for me :)

@gregtatum gregtatum force-pushed the gregtatum:hide-idle-threads branch from bde8161 to f83e58a Aug 27, 2018

@gregtatum gregtatum force-pushed the gregtatum:hide-idle-threads branch from f83e58a to 408fd0e Aug 27, 2018

@gregtatum gregtatum merged commit 0b84472 into devtools-html:master Aug 27, 2018

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 75.88%)
Details
codecov/project 75.9% (+0.02%) compared to 0b1345a
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
deploy/netlify Deploy preview ready!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment