Closed Bug 918825 Opened 11 years ago Closed 11 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
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
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.
Status: ASSIGNED → RESOLVED
Closed: 11 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.

Attachment

General

Created:
Updated:
Size: