Closed
Bug 1147664
Opened 9 years ago
Closed 9 years ago
PerformanceStatsService should offer a detailed mode that doesn't collapse the platform in a single group
Categories
(Toolkit :: Performance Monitoring, defect)
Tracking
()
RESOLVED
FIXED
mozilla42
Tracking | Status | |
---|---|---|
firefox42 | --- | fixed |
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(3 files, 1 obsolete file)
10.39 KB,
patch
|
Details | Diff | Splinter Review | |
40 bytes,
text/x-review-board-request
|
Details | |
40 bytes,
text/x-review-board-request
|
Details |
No description provided.
Assignee | ||
Updated•9 years ago
|
Component: General → Performance Monitoring
Assignee | ||
Comment 1•9 years ago
|
||
Attaching a prototype. Manual testing works, I'll add unit tests asap.
Assignee: nobody → dteller
Assignee | ||
Comment 2•9 years ago
|
||
Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Attachment #8629303 -
Flags: review?(jdemooij)
Assignee | ||
Comment 3•9 years ago
|
||
Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Attachment #8629304 -
Flags: review?(felipc)
Comment 4•9 years ago
|
||
Comment on attachment 8629304 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop https://reviewboard.mozilla.org/r/12555/#review11145 r=felipe for the high-level bits
Attachment #8629304 -
Flags: review?(felipc) → review+
Assignee | ||
Comment 5•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Assignee | ||
Updated•9 years ago
|
Attachment #8629304 -
Attachment is obsolete: true
Comment 6•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem https://reviewboard.mozilla.org/r/12553/#review11071 r=me with comments below addressed. ::: js/src/jsapi.h:5561 (Diff revision 1) > // Initially set to `nullptr` until the first cal to `getGroup`. Nit: s/cal/call/ ::: js/src/vm/Runtime.cpp:964 (Diff revision 2) > + return ownGroup_ = runtime_->new_<PerformanceGroup>(cx, nullptr); Would prefer making the assignment more explicit: ownGroup_ = ...; return ownGroup_; ::: js/src/vm/Runtime.cpp:978 (Diff revision 2) > - runtime_->stopwatch.groups_.lookupForAdd(key); > + runtime_->stopwatch.groups().lookupForAdd(key); Nit: this probably fits on one line (99 chars): ::: js/src/vm/Interpreter.cpp:537 (Diff revision 2) > + JSContext* const cx_; While you're moving these fields, can you move them to the top of this struct? (And maybe make it a class so they are automatically private without needing an extra "private:" label.) ::: js/src/vm/Interpreter.cpp:578 (Diff revision 2) > + sharedGroup->data); Nit: should fit on one line. ::: js/src/vm/Interpreter.cpp:583 (Diff revision 2) > + applyDeltas(userTimeDelta, systemTimeDelta, CPOWTimeDelta, And here. ::: js/src/jsapi.h:5582 (Diff revision 2) > private: Pre-existing nit: indent with two spaces ::: js/src/jsapi.cpp:348 (Diff revision 2) > + closure)) { Nit: this should fit on one line. ::: js/src/jsapi.cpp:379 (Diff revision 2) > + closure)) { And here. ::: js/src/vm/Interpreter.cpp:464 (Diff revision 2) > + JSRuntime* runtime = JS_GetRuntime(cx_); cx_->runtime() ::: js/src/vm/Interpreter.cpp:490 (Diff revision 2) > + private: Nit: indent with one more space (two spaces). ::: js/src/vm/Interpreter.cpp:617 (Diff revision 2) > ++i, duration *= 2) { Nit: { on its own line here. for (a; b; c) { ...; } ::: js/src/vm/Interpreter.cpp:702 (Diff revision 2) > - // An indication of the number of times we have entered the event > +private: Nit: indent with two spaces. ::: js/src/vm/Interpreter.cpp:508 (Diff revision 2) > + JSRuntime* runtime = JS_GetRuntime(cx_); cx_->runtime() ::: js/src/vm/Interpreter.cpp:406 (Diff revision 2) > + JSRuntime* runtime = JS_GetRuntime(cx_); Can use cx_->runtime() here. It's also faster because inlinable. ::: js/src/vm/Runtime.cpp:984 (Diff revision 2) > - runtime_->stopwatch.groups_.add(ptr, key, group_); > + runtime_->stopwatch.groups().add(ptr, key, sharedGroup_); Both the new_ and the add() here can fail I think so we'd need two of these: if (!...) return nullptr; And if the add() fails we should js_delete the sharedGroup_ and set it to nullptr? Please make sure this case is handled correctly.
Attachment #8629303 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 7•9 years ago
|
||
https://reviewboard.mozilla.org/r/12553/#review11071 > Nit: this should fit on one line. Well, it's at least 120 chars. If the JSAPI limit is 99 chars, it doesn't nearly fit.
Assignee | ||
Comment 8•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Attachment #8629303 -
Flags: review+ → review?(jdemooij)
Assignee | ||
Comment 9•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Attachment #8629303 -
Flags: review?(jdemooij)
Assignee | ||
Comment 10•9 years ago
|
||
Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 11•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 13•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 14•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 15•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 16•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 17•9 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d73077ffec
Keywords: checkin-needed
Comment 18•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/19bdfb755eeb https://hg.mozilla.org/integration/fx-team/rev/9eb2e0c7b864
Keywords: checkin-needed
Comment 19•9 years ago
|
||
Backed out for various test failures. https://treeherder.mozilla.org/logviewer.html#?job_id=3880265&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=3879857&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/68596c2c0496
Assignee | ||
Comment 20•9 years ago
|
||
Thanks, I'll investigate.
Assignee | ||
Comment 21•9 years ago
|
||
Er... syntax errors in the code? That looks like a merge error. I'll see if I can find what's going on.
Assignee | ||
Comment 22•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Attachment #8629303 -
Flags: review?(jdemooij)
Assignee | ||
Comment 23•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Attachment #8632534 -
Flags: review?(dtownsend)
Assignee | ||
Updated•9 years ago
|
Attachment #8629303 -
Flags: review?(jdemooij)
Assignee | ||
Comment 24•9 years ago
|
||
Comment on attachment 8629303 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Assignee | ||
Comment 25•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Attachment #8632534 -
Flags: review?(dtownsend)
Assignee | ||
Comment 26•9 years ago
|
||
Rebased. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf8e928aee9
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 27•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/83fa8af8140b https://hg.mozilla.org/integration/fx-team/rev/49667763013b
Keywords: checkin-needed
Assignee | ||
Comment 28•9 years ago
|
||
As mentioned by Mossop, the reviewer for the patch is actually wrong. Changeset 49667763013b was reviewed by felipe.
Comment 29•9 years ago
|
||
Backed out for test_compartments.js xpcshell failures. Please verify that this is green on all platforms before requesting checkin again. https://treeherder.mozilla.org/logviewer.html#?job_id=3896382&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/9dc4d5172957
Comment 30•9 years ago
|
||
And https://treeherder.mozilla.org/logviewer.html#?job_id=3896423&repo=fx-team
Comment 31•9 years ago
|
||
And https://treeherder.mozilla.org/logviewer.html#?job_id=3897360&repo=fx-team
Assignee | ||
Comment 32•9 years ago
|
||
Not again... The patch that you have landed is not the patch that shows up in my MozReview. /me is a bit tired of that.
Assignee | ||
Comment 33•9 years ago
|
||
Gps, do you have any idea what can be causing that?
Flags: needinfo?(gps)
Comment 34•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/cace9a3246c1 https://hg.mozilla.org/integration/fx-team/rev/cfd27d5ffc58
I had to back this out for xpcshell failures like https://treeherder.mozilla.org/logviewer.html#?job_id=3903436&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/a823c9da1ab2
Flags: needinfo?(dteller)
Assignee | ||
Comment 36•9 years ago
|
||
Ok, that error at least makes some sense. KWierso, can you tell me on which platforms this fails? Is this only on the emulator?
Flags: needinfo?(dteller) → needinfo?(wkocher)
Seems just b2g emulator opt: https://treeherder.mozilla.org/#/jobs?repo=fx-team&filter-searchStr=b2g%20xpcshell&fromchange=5f1d69c03e3d
Flags: needinfo?(wkocher)
Assignee | ||
Comment 38•9 years ago
|
||
Ok, I don't want to spend too much time on making sure that a Desktop-oriented feature works on B2G Emulator.
Assignee | ||
Comment 39•9 years ago
|
||
Comment on attachment 8632534 [details] MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Assignee | ||
Comment 40•9 years ago
|
||
Deactivated the test on B2G Emulator. Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a89e5f7a7e
Comment 42•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/60d809c185ad https://hg.mozilla.org/integration/fx-team/rev/1c0413eaf9f3
Keywords: checkin-needed
Comment 43•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/60d809c185ad https://hg.mozilla.org/mozilla-central/rev/1c0413eaf9f3 \m/
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Assignee | ||
Comment 44•9 years ago
|
||
\o/ Thanks everyone.
You need to log in
before you can comment on or make changes to this bug.
Description
•