The default bug view has changed. See this FAQ.

XUL reflow telemetry

RESOLVED FIXED in mozilla10

Status

()

Core
XUL
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: (dormant account), Assigned: froydnj)

Tracking

(Blocks: 1 bug)

unspecified
mozilla10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

6 years ago
Created attachment 555259 [details] [diff] [review]
wip

I would like to measure xul reflow overhead. Unfortunately I don't know how to distinguish xul reflows from html ones. Any suggestions?
One simple way is to see whether the root element is in the XUL namespace, right?  At least good enough for our purposes.
You shouldn't be instrumenting PresShell::InitialReflow if you want to measure reflow; ProcessReflowCommands is the correct point to instrument.  There isn't even any reflow at *all* inside InitialReflow, and hasn't been since bug 378975.
(Reporter)

Comment 3

6 years ago
(In reply to David Baron [:dbaron] from comment #2)
> You shouldn't be instrumenting PresShell::InitialReflow if you want to
> measure reflow; ProcessReflowCommands is the correct point to instrument. 
> There isn't even any reflow at *all* inside InitialReflow, and hasn't been
> since bug 378975.

Ok, I still want to have a probe in there to measure *something* xul-ish happening. What should we call it instead?
Initial Frame Construction?
(Assignee)

Comment 5

6 years ago
Created attachment 555822 [details] [diff] [review]
wip v2: updated timer for PresShell::InitialReflow

This patch addresses dbaron's point, I think.  I haven't figured out how to address bz's idea of checking the root element; the layout code is opaque to me.
Attachment #555259 - Attachment is obsolete: true
  if (mDocument->GetRootElement() && mDocument->GetRootElement()->IsXUL())

(note that you can also check |root->IsXUL()| in InitialReflow, if you move the getting of |root| higher up in the function.
(Reporter)

Updated

6 years ago
Blocks: 683180
(Assignee)

Comment 7

6 years ago
Created attachment 556937 [details] [diff] [review]
v3: only accumulate time for XUL reflows

Updated with checks for XULness.  Feeling good enough to ask for review.
Assignee: nobody → nfroyd
Attachment #555822 - Attachment is obsolete: true
Attachment #556937 - Flags: review?(dbaron)
(Reporter)

Comment 8

6 years ago
Comment on attachment 556937 [details] [diff] [review]
v3: only accumulate time for XUL reflows

Make sure you .hgrc has something like 
[diff]
git = 1
showfunc=true

Use static_cast<>
Attachment #556937 - Flags: review?(dbaron) → review-
(Assignee)

Comment 9

6 years ago
Created attachment 556964 [details] [diff] [review]
v4: use static_cast, fix logic thinko

As Taras pointed out, we should be using static_cast; we should also be accumulating (end-start) durations, not (start-end) durations.
Attachment #556937 - Attachment is obsolete: true
Attachment #556964 - Flags: review?
Comment on attachment 556964 [details] [diff] [review]
v4: use static_cast, fix logic thinko

Please use construction-style cast, i.e., PRUint32(foo) rather than static_cast<PRUint32>(foo).  r=dbaron with that.

(You've complied and tested this, right?)
Attachment #556964 - Flags: review? → review+
(Reporter)

Comment 11

6 years ago
(In reply to David Baron [:dbaron] from comment #10)
> Comment on attachment 556964 [details] [diff] [review]
> v4: use static_cast, fix logic thinko
> 
> Please use construction-style cast, i.e., PRUint32(foo) rather than
> static_cast<PRUint32>(foo).  r=dbaron with that.

David, I've never been asked to use this style of cast before in mozilla. When is constuctor style preferable?
I generally prefer it for primitive types; I think others do too.  In all of layout/base I see only one use of static_cast to a primitive type (and I reviewed it, aargh -- though it's in #ifdef'd out code):
nsDisplayList.cpp:    printf("  Hit! Time: %f, first frame: %p\n", static_cast<double>(clock()),
(Reporter)

Updated

6 years ago
Duplicate of this bug: 673249
(Assignee)

Comment 14

6 years ago
Created attachment 567480 [details] [diff] [review]
v5: use AccumulateTimeDelta, update against trunk

Updated patch with new API usage, proper patch format, etc.  Carrying over r+.
Attachment #556964 - Attachment is obsolete: true
Attachment #567480 - Flags: review+
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/9819261d7930
Keywords: checkin-needed
Target Milestone: --- → mozilla10

Updated

6 years ago
OS: Linux → All
Hardware: x86 → All
https://hg.mozilla.org/mozilla-central/rev/9819261d7930
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
The telemetry dashboard for this histogram (XUL_REFLOW_MS) shows all samples at 0. Is this expected because we're waiting for another piece, or is this a bug?
This probe measures the time spent in ProcessReflowCommands even in cases when there are no reflow commands to process. The fraction of times when this is true completely swamps all other cases, I bet.

My changes in bug 709256 will make this probe measure something sane, at which point we can see how things look
(Reporter)

Updated

5 years ago
Blocks: 709971
You need to log in before you can comment on or make changes to this bug.