Closed
Bug 918825
Opened 11 years ago
Closed 11 years ago
Add paint markers
Categories
(Core :: Gecko Profiler, defect)
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)
441.53 KB,
application/x-gzip
|
Details | |
22.37 KB,
patch
|
BenWa
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Ideally this would wait for bug 867757 to land.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•11 years ago
|
||
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 3•11 years ago
|
||
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-
Assignee | ||
Comment 4•11 years ago
|
||
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
Assignee | ||
Comment 5•11 years ago
|
||
rebased
Attachment #810066 -
Attachment is obsolete: true
Attachment #810932 -
Flags: review?(ehsan)
Assignee | ||
Comment 6•11 years ago
|
||
Forgot to qref
Attachment #810932 -
Attachment is obsolete: true
Attachment #810932 -
Flags: review?(ehsan)
Attachment #810933 -
Flags: review?(ehsan)
Assignee | ||
Comment 7•11 years ago
|
||
... 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 8•11 years ago
|
||
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+
Assignee | ||
Comment 9•11 years ago
|
||
Attachment #810935 -
Attachment is obsolete: true
Attachment #811185 -
Flags: review+
Assignee | ||
Comment 10•11 years ago
|
||
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
Here's a log in case it expires.
Assignee | ||
Comment 13•11 years ago
|
||
Assignee | ||
Comment 14•11 years ago
|
||
Attachment #811185 -
Attachment is obsolete: true
Attachment #815139 -
Flags: review+
Assignee | ||
Comment 15•11 years ago
|
||
Assignee | ||
Comment 16•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Summary: Create marker relationships → Add paint markers
Updated•11 years ago
|
Whiteboard: [c=profiling p= s= u=] → [c=profiling p= s=2013.10.18 u=]
Updated•11 years ago
|
Whiteboard: [c=profiling p= s=2013.10.18 u=] → [c=profiling p= s= u=]
Blocks: 1590700
You need to log in
before you can comment on or make changes to this bug.
Description
•