Closed
Bug 792302
Opened 8 years ago
Closed 8 years ago
Add a more useful pseudo stack entry for flushing to FlushPendingNotifications
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: ehsan, Assigned: ehsan)
Details
Attachments
(1 file, 3 obsolete files)
1.38 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
because that's way more useful than DoReflow!
Comment 1•8 years ago
|
||
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.
Assignee | ||
Comment 2•8 years ago
|
||
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)
Assignee | ||
Comment 3•8 years ago
|
||
(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.
Assignee | ||
Comment 4•8 years ago
|
||
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)
Comment 5•8 years ago
|
||
Why are you removing the reflow label? It seems useful to know what portion of the time is spent in reflow.
Assignee | ||
Comment 6•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Summary: Move the profiler pseudo stack entry for flushing to FlushPendingNotifications → Add a more useful pseudo stack entry for flushing to FlushPendingNotifications
Assignee | ||
Comment 7•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #662408 -
Flags: review?(bgirard) → review+
Comment 8•8 years ago
|
||
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
Assignee | ||
Comment 9•8 years ago
|
||
(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•8 years ago
|
||
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•8 years ago
|
||
(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.)
Assignee | ||
Comment 12•8 years ago
|
||
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+
Assignee | ||
Comment 14•8 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/78ff52eb09d9
Target Milestone: --- → mozilla18
Depends on: 793052
Comment 15•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/78ff52eb09d9
Status: ASSIGNED → RESOLVED
Closed: 8 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.
Description
•