Record compositing operations

RESOLVED FIXED in Firefox 43

Status

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: paul, Assigned: vporof)

Tracking

Trunk
Firefox 43
Dependency tree / graph

Firefox Tracking Flags

(firefox43 fixed)

Details

(Whiteboard: [webvr-noted])

Attachments

(1 attachment, 1 obsolete attachment)

v2
42.92 KB, patch
tromey
: review+
smaug
: review+
jsantell
: review+
mattwoodrow
: review+
Details | Diff | Splinter Review
Reporter

Description

5 years ago
No description provided.
Reporter

Updated

5 years ago
Summary: [timeline] record compositing operatoins → [timeline] record compositing operations
Reporter

Updated

5 years ago
Component: Developer Tools → Developer Tools: Timeline
Reporter

Comment 1

5 years ago
2 things can be done here:
- register layer transaction (main thread)
- add a timeline for the compositing thread
Assignee

Comment 2

4 years ago
Moving into the Profiler component. Filter on GUTHRIE'S WAVY CAKES.
Component: Developer Tools: Timeline → Developer Tools: Profiler
Assignee

Updated

4 years ago
Hardware: x86_64 → All
Summary: [timeline] record compositing operations → Record compositing operations
Assignee

Comment 3

4 years ago
Don't necessarily need bug 1183219 for recording compositing operations, since mozAfterPaints know about docshells and happen on the main thread, and are caused by a composite.
Assignee

Updated

4 years ago
Blocks: perf-tools-fx42
No longer blocks: timeline
No longer depends on: otmt-markers
Assignee: nobody → vporof
Blocks: perf-tools-fx43
No longer blocks: perf-tools-fx42
Assignee

Comment 4

4 years ago
Posted patch composite.patch (obsolete) — Splinter Review
No idea who to ask for review.
Attachment #8645352 - Flags: review?(ttromey)
Comment on attachment 8645352 [details] [diff] [review]
composite.patch

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

See the particular note for a suggestion on a different approach to the timeline change.

I can't review the compositor bits.

::: docshell/base/timeline/TimelineMarker.h
@@ +60,5 @@
>    TracingMetadata GetMetaData() const { return mMetaData; }
>    DOMHighResTimeStamp GetTime() const { return mTime; }
>    const nsString& GetCause() const { return mCause; }
>  
> +  void SetTime(const mozilla::TimeStamp& time) {

I think it would be better to add an optional time argument to TimelineMarker constructors (and in its wrappers, like AddTimelineMarker).  This would have two benefits.

First, the current TimelineMarker object is essentially readonly after construction.  This makes it simpler to reason about; and such a change would preserve this property.

Second, this approach required a change to AddTimelineMarker to return a pointer to the newly constructed marker.  However, much of the API was designed to avoid passing around raw pointers; making it simpler to reason about lifetimes and harder to introduce bugs.  Changing the TimelineMarker constructor would preserve this property as well.
Attachment #8645352 - Flags: review?(ttromey) → review-
Assignee

Comment 6

4 years ago
Good suggestions, will make the fix.
Assignee

Comment 7

4 years ago
Posted patch v2Splinter Review
Tom for the platform timeline/ method/constructors changes, Jordan for the very small frontend changes, Smaug for everything else.
Attachment #8648677 - Flags: review?(ttromey)
Attachment #8648677 - Flags: review?(jsantell)
Attachment #8648677 - Flags: review?(bugs)
Assignee

Updated

4 years ago
Attachment #8645352 - Attachment is obsolete: true
Comment on attachment 8648677 [details] [diff] [review]
v2

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

I think the markers should be green for this one -- if you strongly disagree, let's discuss. The name "Composite" as opposed to alternatives I don't feel strongly about, I think it's probably fine.

::: browser/devtools/performance/modules/markers.js
@@ +74,5 @@
>      label: L10N.getStr("marker.label.paint"),
>    },
> +  "Composite": {
> +    group: 0,
> +    colorName: "graphs-blue",

Should this be green as well, to match paint operations, and what chrome uses for this marker? I think sticking with green for graphics operations makes sense.

::: browser/locales/en-US/chrome/browser/devtools/markers.properties
@@ +16,5 @@
>  # We want to use the same wording as Google Chrome when appropriate.
>  marker.label.styles=Recalculate Style
>  marker.label.reflow=Layout
>  marker.label.paint=Paint
> +marker.label.composite=Composite

Wonder if there's a better name we can use -- chrome is "Composite Layers", not sure if this covers the same operation as them, though.
Attachment #8648677 - Flags: review?(jsantell) → review+
Comment on attachment 8648677 [details] [diff] [review]
v2

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

Looks good, thanks.  I made a few comments but basically just trivia.

::: docshell/base/timeline/TimelineConsumers.h
@@ +71,5 @@
>    static void AddMarkerForDocShellsList(Vector<nsRefPtr<nsDocShell>>& aDocShells,
> +                                        const char* aName,
> +                                        TracingMetadata aMetaData);
> +
> +  // This methodc creates custom markers, none of which have to be tied to a

Typo, "methodc"

::: view/nsView.cpp
@@ +1082,5 @@
> +    nsPresContext* context = presShell->GetPresContext();
> +    context->GetDisplayRootPresContext()->GetRootPresContext()->NotifyDidPaintForSubtree(nsIPresShell::PAINT_COMPOSITE);
> +
> +    // If the two timestamps are identical, this was likely a fake composite
> +    // event which wouldn't be terribly useful to display. XXX: Is this true?

Probably should find out now, before landing; but it seems to me that a zero-duration marker isn't all that interesting in any case, so just removing the XXX bit would be appropriate.
Attachment #8648677 - Flags: review?(ttromey) → review+
Assignee

Updated

4 years ago
Status: NEW → ASSIGNED
Comment on attachment 8648677 [details] [diff] [review]
v2

>+  // Methods for adding markers relevant for particular docshells, or generic
>+  // (meaning that they either can't be tied to a particular docshell, or one
>+  // wasn't accessible in the part of the codebase where they're instantiated).

I don't understand the comment inside ( ).
>+  // This methodc creates custom markers, none of which have to be tied to a
method

> TimelineMarker::TimelineMarker(nsDocShell* aDocShell, const char* aName,
>+                               const mozilla::TimeStamp& time,
aTime

>+                               TracingMetadata aMetaData)
>+  : TimelineMarker(aDocShell, aName, aMetaData)
>+{
>+  bool isInconsistent = false;
>+  mozilla::TimeStamp epoch = mozilla::TimeStamp::ProcessCreation(isInconsistent);
Not sure I'd call this epoch 
I'd just do
  bool ignore;
  mTime = (time - mozilla::TimeStamp::ProcessCreation(ignore)).ToMilliseconds();


>   TimelineMarker(nsDocShell* aDocShell, const char* aName,
>+                 const mozilla::TimeStamp& time,
aTime

> EXPORTS.mozilla += [
>     'AutoGlobalTimelineMarker.h',
>     'AutoTimelineMarker.h',
>+    'ObservedDocShell.h',
>     'TimelineConsumers.h',
>+    'TimelineMarker.h',
oh, do we have a mistake here. TimelineMarker should be in mozilla namespace.
Please put the class to mozilla namespace and also ObservedDocShell.



>+++ b/gfx/layers/client/ClientLayerManager.cpp
Someone familiar with compositing should review this and other compositing specific stuff
(I have no idea whether TimeStamp::Now() is too slow here, or whether we catch all the interesting cases)
Attachment #8648677 - Flags: review?(bugs) → review+
Assignee

Comment 11

4 years ago
(In reply to Olli Pettay [:smaug] from comment #10)
> Comment on attachment 8648677 [details] [diff] [review]

Thanks!

> 
> > EXPORTS.mozilla += [
> >     'AutoGlobalTimelineMarker.h',
> >     'AutoTimelineMarker.h',
> >+    'ObservedDocShell.h',
> >     'TimelineConsumers.h',
> >+    'TimelineMarker.h',
> oh, do we have a mistake here. TimelineMarker should be in mozilla namespace.
> Please put the class to mozilla namespace and also ObservedDocShell.
> 

Changed this in bug 1195838.
 
> 
> >+++ b/gfx/layers/client/ClientLayerManager.cpp
> Someone familiar with compositing should review this and other compositing
> specific stuff
> (I have no idea whether TimeStamp::Now() is too slow here, or whether we
> catch all the interesting cases)

Okay.
Assignee

Comment 12

4 years ago
Comment on attachment 8648677 [details] [diff] [review]
v2

Matt, can you please check the compositor bits?
Attachment #8648677 - Flags: review?(matt.woodrow)
Whiteboard: [webvr-noted]
Attachment #8648677 - Flags: review?(matt.woodrow) → review+
Assignee

Comment 13

4 years ago
Thanks Matt!
https://hg.mozilla.org/mozilla-central/rev/b9ae6b3addaf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
I reproduce this bug on firefox nightly windows 8.1 32bit

BuildID	        20140807030202
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:34.0) Gecko/20100101 Firefox/34.0

It's oke on Latest Firefox Latest Nightly

Build ID 	20150826030211
User Agent 	Mozilla/5.0 (Windows NT 6.3; rv:43.0) Gecko/20100101 Firefox/43.0
QA Whiteboard: [bugday-20150826]
Assignee

Updated

4 years ago
No longer blocks: perf-gaming

Updated

Last year
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.