1147664 - PerformanceStatsService should offer a detailed mode that doesn't collapse the platform in a single group

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Detailed mode

Attaching a prototype. Manual testing works, I'll add unit tests asap.

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem

Bug 1147664 - Detailed mode for PerformanceStats (low-level);r=jandem

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop

Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop

Comment 4

[:Felipe Gomes (needinfo for replies!)]

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

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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 6

[Jan de Mooij [:jandem]]

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.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

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 8

[David Teller [:Yoric] - still alive but not very active]

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 9

[David Teller [:Yoric] - still alive but not very active]

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 10

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop

Bug 1147664 - Detailed mode for PerformanceStats (high-level);r=mossop

Comment 11

[David Teller [:Yoric] - still alive but not very active]

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 12

[David Teller [:Yoric] - still alive but not very active]

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 13

[David Teller [:Yoric] - still alive but not very active]

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 14

[David Teller [:Yoric] - still alive but not very active]

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 15

[David Teller [:Yoric] - still alive but not very active]

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 16

[David Teller [:Yoric] - still alive but not very active]

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 17

[David Teller [:Yoric] - still alive but not very active]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=93d73077ffec

Comment 18

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/19bdfb755eeb
https://hg.mozilla.org/integration/fx-team/rev/9eb2e0c7b864

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[David Teller [:Yoric] - still alive but not very active]

Thanks, I'll investigate.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

Er... syntax errors in the code?
That looks like a merge error. I'll see if I can find what's going on.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

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 23

[David Teller [:Yoric] - still alive but not very active]

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 24

[David Teller [:Yoric] - still alive but not very active]

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 25

[David Teller [:Yoric] - still alive but not very active]

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 26

[David Teller [:Yoric] - still alive but not very active]

Rebased.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7cf8e928aee9

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/83fa8af8140b
https://hg.mozilla.org/integration/fx-team/rev/49667763013b

Comment 28

[David Teller [:Yoric] - still alive but not very active]

As mentioned by Mossop, the reviewer for the patch is actually wrong. Changeset 49667763013b was reviewed by felipe.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

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

[Ryan VanderMeulen [:RyanVM]]

And https://treeherder.mozilla.org/logviewer.html#?job_id=3896423&repo=fx-team

Comment 31

[Ryan VanderMeulen [:RyanVM]]

And https://treeherder.mozilla.org/logviewer.html#?job_id=3897360&repo=fx-team

Comment 32

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Gps, do you have any idea what can be causing that?

Comment 34

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/cace9a3246c1
https://hg.mozilla.org/integration/fx-team/rev/cfd27d5ffc58

Comment 35

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Ok, that error at least makes some sense.
KWierso, can you tell me on which platforms this fails? Is this only on the emulator?

Comment 37

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Seems just b2g emulator opt: https://treeherder.mozilla.org/#/jobs?repo=fx-team&filter-searchStr=b2g%20xpcshell&fromchange=5f1d69c03e3d

Comment 38

[David Teller [:Yoric] - still alive but not very active]

Ok, I don't want to spend too much time on making sure that a Desktop-oriented feature works on B2G Emulator.

Comment 39

[David Teller [:Yoric] - still alive but not very active]

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 40

[David Teller [:Yoric] - still alive but not very active]

Deactivated the test on B2G Emulator.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=52a89e5f7a7e

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Here we go again.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/60d809c185ad
https://hg.mozilla.org/integration/fx-team/rev/1c0413eaf9f3

Comment 43

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60d809c185ad
https://hg.mozilla.org/mozilla-central/rev/1c0413eaf9f3

\m/

Comment 44

[David Teller [:Yoric] - still alive but not very active]

\o/

Thanks everyone.