Closed Bug 918825 Opened 8 years ago Closed 8 years ago

Add paint markers

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

(Keywords: perf, Whiteboard: [c=profiling p= s= u=])

Attachments

(2 files, 7 obsolete files)

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.
Attached patch patch - WIP (obsolete) — Splinter Review
Ideally this would wait for bug 867757 to land.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attached patch Trace drawing (obsolete) — Splinter Review
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.
Attachment #808583 - Attachment is obsolete: true
Attachment #809664 - Flags: review?(ehsan)
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?
Attachment #809664 - Flags: review?(ehsan) → review-
Attached patch trace drawing v2 (obsolete) — Splinter Review
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.
Attachment #809664 - Attachment is obsolete: true
Keywords: perf
Whiteboard: [c=profiling p= s= u=]
Attached patch trace drawing v3 (obsolete) — Splinter Review
rebased
Attachment #810066 - Attachment is obsolete: true
Attachment #810932 - Flags: review?(ehsan)
Attached patch trace drawing v3 (obsolete) — Splinter Review
Forgot to qref
Attachment #810932 - Attachment is obsolete: true
Attachment #810932 - Flags: review?(ehsan)
Attachment #810933 - Flags: review?(ehsan)
Attached patch trace drawing v3 (obsolete) — Splinter Review
... and one more time:
https://tbpl.mozilla.org/?tree=Try&rev=55187edb33f6
Attachment #810933 - Attachment is obsolete: true
Attachment #810933 - Flags: review?(ehsan)
Attachment #810935 - Flags: review?(ehsan)
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.
Attachment #810935 - Flags: review?(ehsan) → review+
Attached patch trace drawing v4 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=982fe2cd5dcf
Attachment #810935 - Attachment is obsolete: true
Attachment #811185 - Flags: review+
Attached file log
Here's a log in case it expires.
Attached patch trace drawing v5Splinter Review
Attachment #811185 - Attachment is obsolete: true
Attachment #815139 - Flags: review+
BTW failure was caused by mPendingGenerationFlush not being set to 0 during construction.
https://hg.mozilla.org/mozilla-central/rev/2390cb215cc4
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Summary: Create marker relationships → Add paint markers
Whiteboard: [c=profiling p= s= u=] → [c=profiling p= s=2013.10.18 u=]
Whiteboard: [c=profiling p= s=2013.10.18 u=] → [c=profiling p= s= u=]
Blocks: 926922
Blocks: 930476
You need to log in before you can comment on or make changes to this bug.