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)

Product:
Toolkit
Component:
Performance Monitoring
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla42
Tracking Flags:
Tracking Status
firefox42 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(3 files, 1 obsolete file)

Detailed mode
10.39 KB, patch
Details | Diff | Splinter Review
MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
40 bytes, text/x-review-board-request
Details
MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
40 bytes, text/x-review-board-request
Details 
      No description provided.
Component: General → Performance Monitoring
Attached patch Detailed mode — — Splinter Review 
Attaching a prototype. Manual testing works, I'll add unit tests asap.
Assignee: nobody → dteller
Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem
Attachment #8629303 - Flags: review?(jdemooij)
Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop
Attachment #8629304 - Flags: review?(felipc)
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+
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 #8629304 - Attachment is obsolete: true
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+
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.
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)
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)
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
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
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
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
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
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
Thanks, I'll investigate.
Er... syntax errors in the code?
That looks like a merge error. I'll see if I can find what's going on.
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)
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)
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
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)
As mentioned by Mossop, the reviewer for the patch is actually wrong. Changeset 49667763013b was reviewed by felipe.
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
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.
Gps, do you have any idea what can be causing that?
Flags: needinfo?(gps)
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)
Ok, I don't want to spend too much time on making sure that a Desktop-oriented feature works on B2G Emulator.
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
Here we go again.
Flags: needinfo?(gps)
Keywords: checkin-needed
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
\o/

Thanks everyone.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: