792302 - Add a more useful pseudo stack entry for flushing to FlushPendingNotifications

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[:ehsan akhgari]

because that's way more useful than DoReflow!

Comment 1

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

There's already a SAMPLE_LABEL for FlushPendingNotifications in PresShell::FlushPendingNotifications (and there's a SAMPLE_LABEL_PRINTF in PresShell::DoReflow for reflow, too).  I'm not sure what you're suggesting here.

Comment 2

[:ehsan akhgari]

Attachment: Patch (v1)

As a bonus, this also includes the flush type!  :-)

Comment 3

[:ehsan akhgari]

(In reply to David Baron [:dbaron] from comment #1)
> There's already a SAMPLE_LABEL for FlushPendingNotifications in
> PresShell::FlushPendingNotifications (and there's a SAMPLE_LABEL_PRINTF in
> PresShell::DoReflow for reflow, too).  I'm not sure what you're suggesting
> here.

I should take the existing pseduo-stack entry out.  The function timer stuff sort of made that skip my eyes.

Comment 4

[:ehsan akhgari]

Attachment: Patch (v2)

Comment 5

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

Why are you removing the reflow label?  It seems useful to know what portion of the time is spent in reflow.

Comment 6

[:ehsan akhgari]

(In reply to comment #5)
> Why are you removing the reflow label?  It seems useful to know what portion of
> the time is spent in reflow.

Hmm, yeah I guess you're right... Let me attach another patch which just makes the Flush label better.

Comment 7

[:ehsan akhgari]

Attachment: Patch (v3)

Comment 8

[Benoit Girard (:BenWa)]

Actually assuming that FlushPendingNotifications will always precede DoReflow let's take out the URL there since it will be redundant:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7362

Comment 9

[:ehsan akhgari]

(In reply to comment #8)
> Actually assuming that FlushPendingNotifications will always precede DoReflow
> let's take out the URL there since it will be redundant:
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7362

I'm not sure if all of the call paths to DoReflow() pass through FlushPendingNotifications.

Comment 10

[Mats Palmgren (:mats)]

It seems unnecessarily costly do all that work when not sampling.
How about something like:

#ifdef MOZ_ENABLE_PROFILER_SPS
  nsCString docURL;
  if (mozilla_sampler_is_active()) {
    nsIURI* uri = mDocument->GetDocumentURI();
    if (uri)
      uri->GetSpec(docURL);
  }
  SAMPLE_LABEL_PRINTF("layout", "Flush", "(%s - %s)",
                      flushTypeNames[aType - 1], docURL.get());
#endif

(Specifically, I want docURL to use the shared
empty buffer when not sampling.)

Comment 11

[David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-8]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> (In reply to comment #8)
> > Actually assuming that FlushPendingNotifications will always precede DoReflow
> > let's take out the URL there since it will be redundant:
> > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#7362
> 
> I'm not sure if all of the call paths to DoReflow() pass through
> FlushPendingNotifications.

They don't, though it's getting close.  (ResizeReflow doesn't go through FlushPendingNotifications->ProcessReflowCommands.)

Comment 12

[:ehsan akhgari]

Attachment: Patch (v4)

Let's just drop the docURL altogether.  We get it in DoReflow in most cases.

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 663138 [details] [diff] [review]
Patch (v4)

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

::: layout/base/nsPresShell.cpp
@@ +3714,5 @@
>     * SetNeedStyleFlush calls on the document.
>     */
>  
> +#ifdef MOZ_ENABLE_PROFILER_SPS
> +  static const char *flushTypeNames[] = {

static const char flushTypeNames[][NNN]

@@ +3725,5 @@
> +  };
> +  // Make sure that we don't miss things added to mozFlushType!
> +  MOZ_ASSERT(aType <= ArrayLength(flushTypeNames));
> +
> +  SAMPLE_LABEL_PRINTF("layout", "Flush", "(%s)",

Drop the Flush_ prefixes and use "(Flush_%s)" here

Comment 14

[:ehsan akhgari]

http://hg.mozilla.org/integration/mozilla-inbound/rev/78ff52eb09d9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/78ff52eb09d9