Closed Bug 1211838 Opened 4 years ago Closed 4 years ago

Add docs for Composite markers

Categories

(DevTools :: Performance Tools (Profiler/Timeline), defect)

defect
Not set

Tracking

(firefox44 fixed)

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- fixed

People

(Reporter: vporof, Assigned: vporof)

Details

(Whiteboard: [polish-backlog] [difficulty=easy])

Attachments

(1 file, 4 obsolete files)

No description provided.
Attached patch v1 (obsolete) — Splinter Review
Assignee: nobody → vporof
Status: NEW → ASSIGNED
Attachment #8670179 - Flags: review?(jsantell)
Comment on attachment 8670179 [details] [diff] [review]
v1

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

::: devtools/client/performance/docs/markers.md
@@ +153,5 @@
>    serializing/deserializing data off the main thread.
> +
> +## Composite
> +
> +* Composite markers are emitted when a compositing-related operation occurs.

This doesn't really follow the format of the other entries -- each section is mapped to a marker name and its properties (so Composite and CompositeForwardTransaction), and any other properties and their types below (look at how nsCycleCollector::{Collect|ForgetSkippable} are two different entries, and the possible properties for other markers (like Paint's rectangle sequence). These are describing the TimelineMarkers emitted, rather than what users ultimately see after we massage data, so someone working on the front end that doesn't know about the underlying markers.

It looks like the Worker docs above in this patch also need the same consistency
Attachment #8670179 - Flags: review?(jsantell)
(In reply to Jordan Santell [:jsantell] [@jsantell] from comment #2)
> Comment on attachment 8670179 [details] [diff] [review]
> v1
> 
> Review of attachment 8670179 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: devtools/client/performance/docs/markers.md
> @@ +153,5 @@
> >    serializing/deserializing data off the main thread.
> > +
> > +## Composite
> > +
> > +* Composite markers are emitted when a compositing-related operation occurs.
> 
> This doesn't really follow the format of the other entries -- each section
> is mapped to a marker name and its properties (so Composite and
> CompositeForwardTransaction), and any other properties and their types below
> (look at how nsCycleCollector::{Collect|ForgetSkippable} are two different
> entries, and the possible properties for other markers (like Paint's
> rectangle sequence). These are describing the TimelineMarkers emitted,
> rather than what users ultimately see after we massage data, so someone
> working on the front end that doesn't know about the underlying markers.
> 

Okay, will reorg the docs.

> It looks like the Worker docs above in this patch also need the same
> consistency

I think the Worker docs are ok the way they are right now. Only "Worker" appears in the sidebar, and the type of operation is detailed in the details pane, so the structure above mirrors this exactly.
Will file a separate bug for the marker docs just to make sure you take a closer look though.
Attached patch v2 (obsolete) — Splinter Review
Addressed comments.
Attachment #8670179 - Attachment is obsolete: true
Attachment #8670773 - Flags: review?(jsantell)
Comment on attachment 8670773 [details] [diff] [review]
v2

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

::: devtools/client/performance/docs/markers.md
@@ +158,5 @@
> +Currently, these markers are only especially interesting for Gecko platform
> +developers, and thus disabled by default. On a high-level, there are 3 types
> +of compsoite markers:
> +
> +# Composite Layers - traces the actual time an inner composite operation

I think we're misunderstanding -- the h2 entries are names of markers received from the actor -- right now these are "Composite" and "CompositeForwardTransaction", not the L10N'd form of these. To look up what markers are available to me as a developer hacking on the performance tool, I would have to save a profile.json or log out markers and inspect, since this doesn't tell me anything about the actual structure of the markers received.

These don't have any extra properties, so most likely just something like

```

## Composite

Composite markers are ...

## CompositeForwardTransaction

CompositeForwardTransaction markers are for...
```

I see "Composite Request Sent" (CompositeForwardTransaction) markers, but not any for "Composite Request Received", so don't know what the marker name for that is, which we should add here if it's implemented

@@ +161,5 @@
> +
> +# Composite Layers - traces the actual time an inner composite operation
> +took on the compositor thread
> +
> +# Composite Request Sent - when the IPC request was made to the compositor from

# is related to h1, ## to h2 -- each marker type is a ## entry

@@ +167,5 @@
> +
> +# Composite Request Received - when the IPC request was received by the
> +compositor thread in the parent process
> +
> +# requestAnimationFrame - when an animation frame is requested by content

AFAICT this isn't a marker -- it's a "Javascript" marker with a specific `causeName`, unless something changed in the recent compositor marker patches
Attachment #8670773 - Flags: review?(jsantell)
Attached patch v3 (obsolete) — Splinter Review
Attachment #8670773 - Attachment is obsolete: true
Attachment #8671885 - Flags: review?(jsantell)
Attachment #8671885 - Flags: review?(jsantell) → review+
Whiteboard: [polish-backlog] [difficulty=easy]
Attached patch v4 (obsolete) — Splinter Review
Updated docs as per changes in bug 1202657.
Attachment #8671885 - Attachment is obsolete: true
Attachment #8677441 - Flags: review+
Attached patch v5Splinter Review
Rebased.
Attachment #8677441 - Attachment is obsolete: true
Attachment #8678144 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/626ba108adf2
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.