Factor ThreadHangStats classes out of Telemetry.cpp

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Telemetry
P3
trivial
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: Iaroslav Sheptykin, Assigned: Iaroslav Sheptykin, Mentored)

Tracking

Trunk
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

5 months ago
Definition of the TimeHistogram, HangStack and HangHistogram, is in [ThreadHangStats](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/ThreadHangStats.h) but the implementations of these classes live in [Telemetry.cpp](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#2603-2674). We should refactor the implementation into a standalone file.
(Assignee)

Updated

5 months ago
Assignee: nobody → yarik.sheptykin
Priority: -- → P3
(Assignee)

Comment 1

5 months ago
:gfritzsche, waiting here for Bug 1371900 to land, otherwise we will get unnecessary conflicts while re-basing.
Flags: needinfo?(gfritzsche)
(Assignee)

Updated

5 months ago
Depends on: 1371900
Comment hidden (mozreview-request)
(Assignee)

Comment 3

5 months ago
Thanks for landing Bug 1371900, Georg! Here is the fist working patch for refactoring ThreadHangStats out.
Comment on attachment 8880341 [details]
Bug 1373900: Factor ThreadHangStats implementation out of Telemetry.cpp.

Chris, do you have some time to look over this?
Flags: needinfo?(gfritzsche)
Attachment #8880341 - Flags: review?(gfritzsche) → review?(chutten)

Comment 5

5 months ago
mozreview-review
Comment on attachment 8880341 [details]
Bug 1373900: Factor ThreadHangStats implementation out of Telemetry.cpp.

https://reviewboard.mozilla.org/r/151706/#review156726

Looks good otherwise. How's our test coverage of these functions?

::: toolkit/components/telemetry/ThreadHangStats.cpp:15
(Diff revision 1)
> +#include "jsapi.h"
> +
> +namespace {
> +
> +using namespace mozilla;
> +using namespace HangMonitor;

Should be mozilla::HangMonitor

::: toolkit/components/telemetry/ThreadHangStats.cpp:16
(Diff revision 1)
> +
> +namespace {
> +
> +using namespace mozilla;
> +using namespace HangMonitor;
> +using namespace Telemetry;

Should be mozilla::Telemetry
Attachment #8880341 - Flags: review?(chutten) → review-
Comment hidden (mozreview-request)
(Assignee)

Comment 7

5 months ago
Thanks for taking a look Chris! cpp parts don't have unit tests. But there is some [JS coverage](https://dxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/tests/unit/test_ThreadHangStats.js) for thread hang stats.
Flags: needinfo?(chutten)

Comment 8

5 months ago
mozreview-review
Comment on attachment 8880341 [details]
Bug 1373900: Factor ThreadHangStats implementation out of Telemetry.cpp.

https://reviewboard.mozilla.org/r/151706/#review156744
Attachment #8880341 - Flags: review?(chutten) → review+

Updated

5 months ago
Flags: needinfo?(chutten)
(Assignee)

Comment 9

5 months ago
Hi guys! I reviewed failed tests for the patch (https://treeherder.mozilla.org/#/jobs?repo=try&revision=97476bbb1bc6) and to me they seem unrelated. I am therefore adding a [checkin-needed] here.
(Assignee)

Updated

5 months ago
Keywords: checkin-needed

Comment 10

5 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a86be309d9f4
Factor ThreadHangStats implementation out of Telemetry.cpp. r=chutten
Keywords: checkin-needed

Comment 11

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a86be309d9f4
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.