Closed Bug 792302 Opened 7 years ago Closed 7 years ago

Add a more useful pseudo stack entry for flushing to FlushPendingNotifications

Categories

(Core :: Layout, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: ehsan, Assigned: ehsan)

Details

Attachments

(1 file, 3 obsolete files)

because that's way more useful than DoReflow!
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.
Attached patch Patch (v1) (obsolete) — Splinter Review
As a bonus, this also includes the flush type!  :-)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #662405 - Flags: review?(roc)
Attachment #662405 - Flags: review?(bgirard)
(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.
Attached patch Patch (v2) (obsolete) — Splinter Review
Attachment #662405 - Attachment is obsolete: true
Attachment #662405 - Flags: review?(roc)
Attachment #662405 - Flags: review?(bgirard)
Attachment #662406 - Flags: review?(roc)
Attachment #662406 - Flags: review?(bgirard)
Why are you removing the reflow label?  It seems useful to know what portion of the time is spent in reflow.
(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.
Summary: Move the profiler pseudo stack entry for flushing to FlushPendingNotifications → Add a more useful pseudo stack entry for flushing to FlushPendingNotifications
Attached patch Patch (v3) (obsolete) — Splinter Review
Attachment #662406 - Attachment is obsolete: true
Attachment #662406 - Flags: review?(roc)
Attachment #662406 - Flags: review?(bgirard)
Attachment #662408 - Flags: review?(roc)
Attachment #662408 - Flags: review?(bgirard)
Attachment #662408 - Flags: review?(bgirard) → review+
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
(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.
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.)
(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.)
Attached patch Patch (v4)Splinter Review
Let's just drop the docURL altogether.  We get it in DoReflow in most cases.
Attachment #662408 - Attachment is obsolete: true
Attachment #662408 - Flags: review?(roc)
Attachment #663138 - Flags: review?(roc)
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
Attachment #663138 - Flags: review?(roc) → review+
http://hg.mozilla.org/integration/mozilla-inbound/rev/78ff52eb09d9
Target Milestone: --- → mozilla18
Depends on: 793052
https://hg.mozilla.org/mozilla-central/rev/78ff52eb09d9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
No longer depends on: 793052
You need to log in before you can comment on or make changes to this bug.