Closed Bug 1188248 Opened 6 years ago Closed 6 years ago

Merge CPOW into the jank array, get rid of CPOW-specific add-on warnings

Categories

(Toolkit :: Performance Monitoring, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(2 files, 1 obsolete file)

Right now, CPOW is measured as a number of milliseconds. Rather, we should measure it by the jank it causes, as the currently existing array of jank.
Actually, there is no good reason to separate CPOW from the jank array. This complicates the code and this contributes to false alerts.
Summary: Consider measuring CPOW as jank array → Merge CPOW into the jank array, get rid of CPOW-specific add-on warnings
Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r?jandem
Attachment #8641063 - Flags: review?(jdemooij)
Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r?mossop
Attachment #8641064 - Flags: review?(dtownsend)
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

https://reviewboard.mozilla.org/r/14417/#review13131
Attachment #8641063 - Flags: review?(jdemooij)
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Oops, Review Board again.
Attachment #8641063 - Flags: review+
Assignee: nobody → dteller
Attachment #8641064 - Flags: review?(dtownsend) → review+
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

https://reviewboard.mozilla.org/r/14419/#review13307

Ship It!
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Bug 1188248 - Move Stopwatch-related code to Stopwatch.h;r?jandem
Attachment #8641063 - Attachment description: MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r?jandem → MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.h;r?jandem
Attachment #8641063 - Flags: review+ → review?(jdemooij)
Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem
Attachment #8641064 - Attachment description: MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r?mossop → MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

https://reviewboard.mozilla.org/r/14417/#review17817

Thanks for doing this. The patch has a number of style issues though, please pay more attention to this in the future so I don't have to keep pointing these out, the list below is not complete.

::: js/src/vm/Interpreter.cpp:58
(Diff revision 2)
> +#include "vm/Stopwatch.h"

This should go after the other non-inl headers or you'll get `make check-style` failures.

::: js/src/vm/Stopwatch.h:111
(Diff revision 2)
> +private:

Indent with 2 spaces.

::: js/src/vm/Stopwatch.h:210
(Diff revision 2)
> +  Groups& groups() {

Why the change from 4 space indent to 2 spaces here?

::: js/src/vm/Stopwatch.h:545
(Diff revision 2)
> + public:

2 space indent.

::: js/src/vm/Stopwatch.h:553
(Diff revision 2)
> +   private:

2 space indent.

::: js/src/vm/Stopwatch.cpp:27
(Diff revision 2)
> +    }

No {}
Attachment #8641063 - Flags: review?(jdemooij)
https://reviewboard.mozilla.org/r/14417/#review17817

Sorry about that. I keep switching between 3 or 4 styles, which makes things a bit complex. Do you have a written guideline somewhere?

> Why the change from 4 space indent to 2 spaces here?

Oh. I think the two different files from which this comes have different indentations...
Attachment #8641063 - Attachment description: MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.h;r?jandem → MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r?jandem
Attachment #8641063 - Flags: review?(jdemooij)
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r?jandem
Comment on attachment 8659427 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #11)
> Sorry about that. I keep switching between 3 or 4 styles, which makes things
> a bit complex. Do you have a written guideline somewhere?

I've found https://wiki.mozilla.org/JavaScript:SpiderMonkey:Coding_Style to be a good reference.
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

https://reviewboard.mozilla.org/r/14417/#review18041

::: js/src/vm/Stopwatch.cpp:416
(Diff revision 3)
> +  MOZ_GUARD_OBJECT_NOTIFIER_INIT;

Should use 4 space indent here and below.
Attachment #8641063 - Flags: review?(jdemooij) → review+
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r=jandem
Attachment #8641063 - Attachment description: MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r?jandem → MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r=jandem
Comment on attachment 8659427 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem
Attachment #8659427 - Flags: review?(jdemooij)
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8659427 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem
Attachment #8659427 - Flags: review?(jdemooij)
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric
Attachment #8641063 - Attachment description: MozReview Request: Bug 1188248 - Move Stopwatch-related code to Stopwatch.*;r=jandem → MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric
Attachment #8641063 - Flags: review?(dteller)
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Attachment #8659427 - Attachment is obsolete: true
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

https://reviewboard.mozilla.org/r/14417/#review20981
Attachment #8641063 - Flags: review?(dteller) → review+
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8641063 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric

Bug 1188248 - Merge jank monitoring and CPOW monitoring (low-level);r=jandem,yoric
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
Comment on attachment 8641064 [details]
MozReview Request: Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop

Bug 1188248 - Merge jank monitoring and CPOW monitoring (high-level);r=mossop
https://hg.mozilla.org/mozilla-central/rev/986b736f3615
https://hg.mozilla.org/mozilla-central/rev/f34c8ac43aff
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.