Skip to content
This repository has been archived by the owner on May 5, 2022. It is now read-only.

Child hangs #31

Open
wants to merge 7 commits into
base: gh-pages
Choose a base branch
from
Open

Child hangs #31

wants to merge 7 commits into from

Conversation

Uberi
Copy link
Collaborator

@Uberi Uberi commented Jan 26, 2016

Note: This is not ready yet, since https://bugzilla.mozilla.org/show_bug.cgi?id=1242777 is in review.

  • Add support for child process thread hang stats.
  • This required a relatively large number of changes due to child thread hang stats being asynchronous. The updating functionality in Statuser has been changed to be fully asynchronous as well.

@Uberi
Copy link
Collaborator Author

Uberi commented Jan 27, 2016

Here's a screenshot of the new hang stats in action:

screenshot from 2016-01-27 15-04-46

let lastChildHangsRetrievedTime = -Infinity;
function getChildThreadHangs() {
if (Date.now() - lastChildHangsRetrievedTime < 300) {
return new Promise((resolve) => { resolve(cachedPreviousChildHangs); });
Copy link
Owner

Choose a reason for hiding this comment

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

Isn't this just return Promise.resolve(cachedPreviousChildHangs);?

@Uberi
Copy link
Collaborator Author

Uberi commented Jan 29, 2016

By the way, I added changes according to your feedback and a mode to allow people to select parent only thread hangs, or parent + child. Is there a use case for child only hangs?

@avih
Copy link

avih commented Jan 29, 2016

By the way, I added changes according to your feedback and a mode to allow people to select parent only thread hangs, or parent + child. Is there a use case for child only hangs?

I think there is. Child only focuses on content without the "noise" from firefox itself. Besides, it sounds easy enough to add ;)

* Switching modes now clears the hang count properly.
* Fix the input event response lag detection.
* Fix warning banner being hidden when it should be shown in certain cases.
let cachedPreviousChildHangs = null;
let lastChildHangsRetrievedTime = -Infinity;
function getChildThreadHangs() {
if (Date.now() - lastChildHangsRetrievedTime < 300) {
Copy link
Owner

Choose a reason for hiding this comment

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

Magic number ought to be a constant

@chutten
Copy link
Owner

chutten commented Feb 2, 2016

You split the count from the stacks? (checkboxes for the stacks, radio button for the count)... Our audience is pretty savvy, so a mismatch between the two is less likely, but I would've assumed the two would remain linked (though, since the threshold is variable for the count and not the hangs, maybe they are already conceptually unlinked...).

Anyhoo.

* Use Promise.reject() to simplify error handling.
* Clear hangs on update to prevent flashes of incorrect numbers upon switching modes or clearing the count.
* Use constants and update comments as appropriate.
* Handle ranges better in child-only mode, to fix computed thresholds.

As these all work together, I've put them in one commit.
@Uberi
Copy link
Collaborator Author

Uberi commented Feb 2, 2016

Hmm, I originally split them because it wouldn't make sense to show stacks when using the event loop lag or input event response time modes, but it's still useful to have them visible in those cases.

Do you think it would be better to link them to the mode, and hide the entire hang stacks section when a non-thread-hang mode is selected?

@vdjeric
Copy link

vdjeric commented Feb 16, 2016

Is the child-process support getting released soon?

@Uberi
Copy link
Collaborator Author

Uberi commented Feb 16, 2016

I forgot to push this earlier, but now the hang stacks that show up are dependent on the mode selected; in child mode it will only show child hangs, in parent mode only parent hangs, etc. (that means the checkboxes are no longer necessary).

Here's a fresh testing XPI (unsigned): @statuser-0.1.4.xpi.zip

Also, the corresponding patch is already live in aurora and beta, so this should already work with the latest versions of these. Without the patch, it'll detect that and show a warning banner.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants