Closed Bug 1156264 Opened 5 years ago Closed 5 years ago

Make it possible to [de]activate jank monitoring and CPOW monitoring separately

Categories

(Toolkit :: Performance Monitoring, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files)

No description provided.
Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem
Attachment #8615253 - Flags: review?(jdemooij)
Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop
Attachment #8615254 - Flags: review?(dtownsend)
Attachment #8615254 - Flags: review?(dtownsend) → review+
Comment on attachment 8615254 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop

https://reviewboard.mozilla.org/r/10231/#review9007

Ship It!
Comment on attachment 8615253 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem

https://reviewboard.mozilla.org/r/10229/#review9171

r=me with comments addressed.

::: js/src/vm/Runtime.cpp:892
(Diff revision 1)
> -js::IsStopwatchActive(JSRuntime* rt)
> +js::SetStopwatchIsMonitoringCPOW(JSRuntime* rt, bool value) {

And here.

::: js/src/vm/Runtime.cpp:897
(Diff revision 1)
> +js::GetStopwatchIsMonitoringCPOW(JSRuntime* rt) {

And here.

::: js/src/vm/Runtime.cpp:882
(Diff revision 1)
> -js::SetStopwatchActive(JSRuntime* rt, bool isActive)
> +js::SetStopwatchIsMonitoringJank(JSRuntime* rt, bool value) {

'{' should be on its own line, as it was before

Also, can we change the return type to |void|?

::: js/src/vm/Runtime.cpp:887
(Diff revision 1)
> +js::GetStopwatchIsMonitoringJank(JSRuntime* rt) {

And here.

::: js/ipc/CPOWTimer.cpp:17
(Diff revision 1)
> +        return;

cx_ and startInterval_ are left uninitialized in this case, right? The destructor relies on cx_ being nullptr in this case. Please initialize them.
Attachment #8615253 - Flags: review?(jdemooij) → review+
Ugh, ReviewBoard is not ordering comments by line number apparently :( It should be clear what the "And here" refers to though..
https://reviewboard.mozilla.org/r/10229/#review9179

> '{' should be on its own line, as it was before
> 
> Also, can we change the return type to |void|?

Oops, forgot to propagate the return value. Thanks for the catch.
Attachment #8615253 - Flags: review+ → review?(jdemooij)
Comment on attachment 8615253 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem

Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem
Comment on attachment 8615254 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop

Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop
Attachment #8615254 - Flags: review+ → review?(dtownsend)
Comment on attachment 8615253 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem

Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (low-level);r=jandem
Attachment #8615253 - Flags: review?(jdemooij)
Comment on attachment 8615254 [details]
MozReview Request: Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop

Bug 1156264 - Activate/deactivate jank and CPOW monitoring separately (high-level);r=mossop
Attachment #8615254 - Flags: review?(dtownsend)
Assignee: nobody → dteller
You need to log in before you can comment on or make changes to this bug.