918825 - Add paint markers

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Currently markers denote a single point event.

We want to extend so that markers can have relationships. For example 'Paint Start' caused by [DOM Mod1, DOM Mod2, DOM Mod3].

We want to extend markers to denote time intervals. For example 'Paint Start' -> 'Paint End'.

We want these relationship to work across process. 'Composition start' caused by 'Paint end'.

This means that markers will form small relationship graphs.

Comment 1

[Benoit Girard (:BenWa)]

Attachment: patch - WIP

Ideally this would wait for bug 867757 to land.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: Trace drawing

Example profile collected with this patch:
http://people.mozilla.org/~bgirard/cleopatra/?report=1305aa31f417005934020cd7181d8331691945d1

Frame information will only be shown if you're profiling the main thread & the compositor.

Comment 3

[(no longer active)]

Comment on attachment 809664 [details] [diff] [review]
Trace drawing

Review of attachment 809664 [details] [diff] [review]:
-----------------------------------------------------------------

r- because of the ProfilerMarkerPayload thing.  We need a better implementation there.

::: tools/profiler/GeckoProfiler.h
@@ +88,5 @@
>  #define PROFILER_MAIN_THREAD_LABEL(name_space, info) do {} while (0)
>  #define PROFILER_MAIN_THREAD_LABEL_PRINTF(name_space, info, format, ...) do {} while (0)
>  
> +static inline void profiler_tracing(const char* aCategory, const char* aInfo,
> +                                    const TracingMetadata& metaData = TRACING_DEFAULT) {}

Just pass in the type itself, not a reference to it.

::: tools/profiler/GeckoProfilerFunc.h
@@ +73,5 @@
>  /* Returns true if env var SPS_NEW is set to anything, else false. */
>  extern bool sps_version2();
>  
> +void mozilla_sampler_tracing(const char* aCategory, const char* aInfo,
> +                             const TracingMetadata& aMetaData);

Here and elsewhere.

::: tools/profiler/GeckoProfilerImpl.h
@@ +211,5 @@
>  #define PROFILER_MARKER(info) mozilla_sampler_add_marker(info)
>  #define PROFILER_MARKER_PAYLOAD(info, payload) mozilla_sampler_add_marker(info, payload)
> +#define PROFILER_MAIN_THREAD_MARKER(info)  MOZ_ASSERT(NS_IsMainThread(), "This can only be called on the main thread"); mozilla_sampler_add_marker(info)
> +#define PROFILER_TRACING(category, info) mozilla_sampler_add_marker(info, )
> +#define PROFILER_TRACING_PAYLOAD(category, info, payload) do {} while (0)

You're not using these two macros.  What's the point in defining them?

::: tools/profiler/ProfilerMarkers.h
@@ +24,5 @@
>   * is called from the main thread.
>   */
>  class ProfilerMarkerPayload {
>  public:
>    ProfilerMarkerPayload() {}

You should initialize all of the members in both constructors!  Or better yet, just make aCategory default to null or something and remove this ctor.

@@ +58,5 @@
>    /**
>     * Called from the main thread
>     */
>    virtual JSObjectBuilder::Object
> +  preparePayload(JSObjectBuilder& b) { return preparePayloadImp(b); }

I don't understand the goal behind this change (and the preparePayloadImp function.)  Firstly, that doesn't need to be a template -- you just need two overrides.  Also, why make these functions virtual? Isn't the "imp" function the only one that should be virtual?

Comment 4

[Benoit Girard (:BenWa)]

Attachment: trace drawing v2

This patch needs to be rebased on top of inbound so I'm going to wait until inbound is merged to central before continuing this patch.

Comment 5

[Benoit Girard (:BenWa)]

Attachment: trace drawing v3

rebased

Comment 6

[Benoit Girard (:BenWa)]

Attachment: trace drawing v3

Forgot to qref

Comment 7

[Benoit Girard (:BenWa)]

Attachment: trace drawing v3

... and one more time:
https://tbpl.mozilla.org/?tree=Try&rev=55187edb33f6

Comment 8

[(no longer active)]

Comment on attachment 810935 [details] [diff] [review]
trace drawing v3

Review of attachment 810935 [details] [diff] [review]:
-----------------------------------------------------------------

This is great, thanks!

::: tools/profiler/ProfilerMarkers.cpp
@@ +11,4 @@
>  
>  ProfilerMarkerPayload::ProfilerMarkerPayload(ProfilerBacktrace* aStack)
>    : mStack(aStack)
>  {}

I still insist that you should initialize all of the members of this class.  :-)

::: tools/profiler/ProfilerMarkers.h
@@ +93,5 @@
> +  typename Builder::Object preparePayloadImp(Builder& b);
> +
> +private:
> +  const char *mCategory;
> +  TracingMetadata mMeta;

Nit: mMetaData.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: trace drawing v4

https://tbpl.mozilla.org/?tree=Try&rev=982fe2cd5dcf

Comment 10

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2eb7e93512b1

Comment 11

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e19ed60eaffa - browser-chrome was crashing all over the place:

https://tbpl.mozilla.org/php/getParsedLog.php?id=28656253&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28656489&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28656484&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28655777&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28655889&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28654900&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28654753&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=28654820&tree=Mozilla-Inbound

Comment 12

[Benoit Girard (:BenWa)]

Attachment: log

Here's a log in case it expires.

Comment 13

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=a9faf0aa22ba

Comment 14

[Benoit Girard (:BenWa)]

Attachment: trace drawing v5

Comment 15

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2390cb215cc4

Comment 16

[Benoit Girard (:BenWa)]

BTW failure was caused by mPendingGenerationFlush not being set to 0 during construction.

Comment 17

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

https://hg.mozilla.org/mozilla-central/rev/2390cb215cc4