Last Comment Bug 681535 - XUL reflow telemetry
: XUL reflow telemetry
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Nathan Froyd [:froydnj]
:
Mentors:
: 673249 (view as bug list)
Depends on:
Blocks: 683180 709971
  Show dependency treegraph
 
Reported: 2011-08-23 17:29 PDT by (dormant account)
Modified: 2011-12-12 14:27 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
wip (1.76 KB, patch)
2011-08-23 17:29 PDT, (dormant account)
no flags Details | Diff | Splinter Review
wip v2: updated timer for PresShell::InitialReflow (2.90 KB, patch)
2011-08-25 13:30 PDT, Nathan Froyd [:froydnj]
no flags Details | Diff | Splinter Review
v3: only accumulate time for XUL reflows (4.57 KB, patch)
2011-08-30 11:46 PDT, Nathan Froyd [:froydnj]
taras.mozilla: review-
Details | Diff | Splinter Review
v4: use static_cast, fix logic thinko (4.63 KB, patch)
2011-08-30 12:59 PDT, Nathan Froyd [:froydnj]
dbaron: review+
Details | Diff | Splinter Review
v5: use AccumulateTimeDelta, update against trunk (4.33 KB, patch)
2011-10-17 09:51 PDT, Nathan Froyd [:froydnj]
nfroyd: review+
Details | Diff | Splinter Review

Description (dormant account) 2011-08-23 17:29:10 PDT
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?
Comment 1 Boris Zbarsky [:bz] 2011-08-23 19:20:53 PDT
One simple way is to see whether the root element is in the XUL namespace, right?  At least good enough for our purposes.
Comment 2 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-08-23 20:22:57 PDT
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.
Comment 3 (dormant account) 2011-08-24 15:46:45 PDT
(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?
Comment 4 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-08-24 18:35:15 PDT
Initial Frame Construction?
Comment 5 Nathan Froyd [:froydnj] 2011-08-25 13:30:41 PDT
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.
Comment 6 Boris Zbarsky [:bz] 2011-08-25 18:54:48 PDT
  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.
Comment 7 Nathan Froyd [:froydnj] 2011-08-30 11:46:00 PDT
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.
Comment 8 (dormant account) 2011-08-30 11:48:45 PDT
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<>
Comment 9 Nathan Froyd [:froydnj] 2011-08-30 12:59:54 PDT
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.
Comment 10 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-08-30 17:38:35 PDT
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?)
Comment 11 (dormant account) 2011-08-30 18:35:02 PDT
(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?
Comment 12 David Baron :dbaron: ⌚️UTC+1 (mostly busy through August 4; review requests must explain patch) 2011-08-30 20:40:00 PDT
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()),
Comment 13 (dormant account) 2011-09-30 11:38:36 PDT
*** Bug 673249 has been marked as a duplicate of this bug. ***
Comment 14 Nathan Froyd [:froydnj] 2011-10-17 09:51:31 PDT
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+.
Comment 16 Ed Morley [:emorley] 2011-10-23 09:29:51 PDT
https://hg.mozilla.org/mozilla-central/rev/9819261d7930
Comment 17 Johnathan Nightingale [:johnath] 2011-12-12 08:27:16 PST
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?
Comment 18 Boris Zbarsky [:bz] 2011-12-12 08:36:26 PST
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

Note You need to log in before you can comment on or make changes to this bug.