Closed Bug 681535 Opened 9 years ago Closed 8 years ago
XUL reflow telemetry
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.
(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?
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.
Updated with checks for XULness. Feeling good enough to ask for review.
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-
As Taras pointed out, we should be using static_cast; we should also be accumulating (end-start) durations, not (start-end) durations.
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+
(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()),
Updated patch with new API usage, proper patch format, etc. Carrying over r+.
Target Milestone: --- → mozilla10
Status: NEW → RESOLVED
Closed: 8 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
You need to log in before you can comment on or make changes to this bug.