681535 - XUL reflow telemetry

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[(dormant account)]

Attachment: 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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

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)]

(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-4, no longer working on Mozilla)]

Initial Frame Construction?

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: 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 [:bzbarsky]]

  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]]

Attachment: v3: only accumulate time for XUL reflows

Updated with checks for XULness.  Feeling good enough to ask for review.

Comment 8

[(dormant account)]

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]]

Attachment: 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-4, no longer working on Mozilla)]

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)]

(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-4, no longer working on Mozilla)]

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 14

[Nathan Froyd [:froydnj]]

Attachment: v5: use AccumulateTimeDelta, update against trunk

Updated patch with new API usage, proper patch format, etc.  Carrying over r+.

Comment 15

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/9819261d7930

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9819261d7930

Comment 17

[Johnathan Nightingale [:johnath]]

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 [:bzbarsky]]

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