1240786 - The BHR should be able to inject Timeline events

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

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

We are attempting to find out whether BHR is behaving correctly (with strong suspicion that it's not). It would be useful if BHR was able to inject Timeline markers at the start/end of each period of suspected hang. This would help us investigate these periods using the profiler.

Comment 1

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

If I read TimelineMarker.h correctly, I think that this should not be too difficult, assuming that we don't need a stack.

Comment 2

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

Well, it looks like the main difficulty will be porting the code from PRIntervalTime to TimeStamp. Might take a little finesse.

Comment 3

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

BenWa, what's the best way to inject a profiler Timeline event from xpcom/? Something like that? https://dxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp#2839

Comment 4

[Benoit Girard (:BenWa)]

Call 'PROFILER_MARKER' form the thread that owns the timeline you want to post it on.

Comment 5

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

Attachment: MozReview Request: Bug 1240786 - Converting BHR to TimeStamp/TimeDuration;r?ehsan

Review commit: https://reviewboard.mozilla.org/r/31917/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31917/

Comment 6

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

Attachment: MozReview Request: Bug 1240786 - Letting BHR inject timeline events;r?benwa

Review commit: https://reviewboard.mozilla.org/r/31919/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/31919/

Comment 7

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

Comment on attachment 8710935 [details]
MozReview Request: Bug 1240786 - Converting BHR to TimeStamp/TimeDuration;r?ehsan

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31917/diff/1-2/

Comment 8

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

Comment on attachment 8710936 [details]
MozReview Request: Bug 1240786 - Letting BHR inject timeline events;r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31919/diff/1-2/

Comment 9

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

Comment on attachment 8710936 [details]
MozReview Request: Bug 1240786 - Letting BHR inject timeline events;r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31919/diff/2-3/

Comment 10

[Benoit Girard (:BenWa)]

Comment on attachment 8710936 [details]
MozReview Request: Bug 1240786 - Letting BHR inject timeline events;r?benwa

https://reviewboard.mozilla.org/r/31919/#review28745

r+ with the following super minor stuff fixed.

::: xpcom/threads/BackgroundHangMonitor.cpp:76
(Diff revision 3)
> +    if (profiler_is_active()) {

You don't need profiler_is_active again here. If it's inactive the marker will just be free'ed.

::: xpcom/threads/BackgroundHangMonitor.cpp:455
(Diff revision 3)
> +#ifdef MOZ_ENABLE_PROFILER_SPS

You don't need an ifdef here. It's not saving a lot of work and with the stub implementation of the profiler you'll be fine.

the if() branch here is fine cause it will save a non trivial ammount of work when the profiler is inactive.

Comment 11

[Victor Porof [:vporof][:vp]]

TimelineMarker.h is used by the devtools performance/timeline. 

PROFILER_MARKER calls add markers to the Gecko profiler add-on.

Yoric, which one exactly do you want to use?

Comment 12

[Victor Porof [:vporof][:vp]]

Relevant: https://dxr.mozilla.org/mozilla-central/source/docshell/base/timeline/readme.md

Comment 13

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

(In reply to Victor Porof [:vporof][:vp] from comment #11)
> TimelineMarker.h is used by the devtools performance/timeline. 
> 
> PROFILER_MARKER calls add markers to the Gecko profiler add-on.
> 
> Yoric, which one exactly do you want to use?

Gecko Profiler.

Comment 14

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

BenWa, with these patches, I don't see my markers in Cleopatra, even though I have checked that the code is executed and the timestamps are correct. Am I forgetting something, either in code or in Cleopatra?

Comment 15

[(no longer active)]

Comment on attachment 8710935 [details]
MozReview Request: Bug 1240786 - Converting BHR to TimeStamp/TimeDuration;r?ehsan

https://reviewboard.mozilla.org/r/31917/#review28773

Comment 16

[Benoit Girard (:BenWa)]

IRC: I think custom events might be filtered by default
IRC: most of them have their own custom presentation code in cleopatra

If you just want text marker then remove the payload. If you care about denoting the start/end point then you'll need to modify the cleopatra DOM to display the start/end in some way.

Comment 17

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

(In reply to Benoit Girard (:BenWa) from comment #16)
> If you just want text marker then remove the payload. If you care about
> denoting the start/end point then you'll need to modify the cleopatra DOM to
> display the start/end in some way.

I can't find a good entry point for this in Cleopatra. Who could give me a hand?

Comment 18

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

Comment on attachment 8710936 [details]
MozReview Request: Bug 1240786 - Letting BHR inject timeline events;r?benwa

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31919/diff/3-4/

Comment 19

[Benoit Girard (:BenWa)]

Already answered this in: https://github.com/bgirard/Gecko-Profiler-Addon/issues/107#issuecomment-174421331

Comment 20

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

Too many communication channels involved here, we are losing messages.

As mentioned in https://github.com/bgirard/cleopatra/issues/66, it looks like my markers never actually make it to Cleopatra. Moreover, the C++ json serialization code in this patch is never triggered, which is weird. What am I missing?

Comment 21

[Benoit Girard (:BenWa)]

I'm not sure? Make sure the marker is still in the circular buffer when you're saving the data.

Comment 22

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

How do I do that?

Comment 23

[Benoit Girard (:BenWa)]

set profiler.interval to 100
Then cause something to insert the marker and save the profile immediately.

Comment 24

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

The profiler marker API has changed a lot since 2016, and I believe should work from almost anywhere, including BHR, if that's still needed now.
Please reopen/file if you do have issues.