Closed
Bug 1341255
Opened 7 years ago
Closed 7 years ago
Profiler tidyups: remove StackEntry, rename ProfileEntry to ProfileBufferEntry
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox54 | --- | fixed |
People
(Reporter: jseward, Assigned: jseward)
References
Details
Attachments
(2 files)
11.36 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
33.27 KB,
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
Two renamings: (1) We now have class StackEntry : public js::ProfileEntry { }; so remove StackEntry and use js::ProfileEntry instead. (2) The code mentions both js::ProfileEntry from js/public/ProfilingStack.h and ProfileEntry from tools/profiler/core/ProfileEntry.h which is confusing, and is made worse by (1). Rename, therefore, ProfileEntry to ProfileBufferEntry.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Comment 2•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8839468 -
Attachment is patch: true
Assignee | ||
Updated•7 years ago
|
Attachment #8839467 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•7 years ago
|
Attachment #8839468 -
Flags: review?(n.nethercote)
![]() |
||
Comment 3•7 years ago
|
||
Comment on attachment 8839467 [details] [diff] [review] bug1341255-1-remove-StackEntry.cset Review of attachment 8839467 [details] [diff] [review]: ----------------------------------------------------------------- It would be nice to see the log message that you're planning to land with at review time. How are you creating and uploading your patches? If you use |hg mq| I strongly recommend |hg bzexport| from version-control-tools repo. IIRC, `mach bootstrap` installs automatically if you let it. I wonder what all the |volatile|s in this code are supposed to be doing :/ ::: tools/profiler/public/PseudoStack.h @@ -48,5 @@ > #else > # error "Memory clobber not supported for your platform." > #endif > > -// A stack entry exists to allow the JS engine to inform the Gecko Profiler of #include ProfilingStack.h in this file? ::: xpcom/threads/ThreadStackHelper.h @@ -8,5 @@ > #define mozilla_ThreadStackHelper_h > > #include "mozilla/ThreadHangStats.h" > > -#include "GeckoProfiler.h" I'm wondering where the definition of ProfileEntry is coming from in this file. Unified builds make it hard to know. Perhaps #include ProfilingStack.h? Or just forward declare ProfileEntry in this header and then #include ProfilingStack.h in ThreadStackHelper.cpp?
Attachment #8839467 -
Flags: review?(n.nethercote) → review+
![]() |
||
Comment 4•7 years ago
|
||
Comment on attachment 8839468 [details] [diff] [review] bug1341255-1-rename-ProfileEntry.cset Review of attachment 8839468 [details] [diff] [review]: ----------------------------------------------------------------- ::: tools/profiler/core/ProfileBufferEntry.h @@ +4,5 @@ > * License, v. 2.0. If a copy of the MPL was not distributed with this > * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ > > +#ifndef MOZ_PROFILE_BUFFER_ENTRY_H > +#define MOZ_PROFILE_BUFFER_ENTRY_H Might as well follow standard style and change this to ProfileBufferEntry_h, as per https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style (search for "include guards"). @@ +409,5 @@ > // ] > // } > // > > +#endif /* ndef MOZ_PROFILE_BUFFER_ENTRY_H */ Ditto. ::: tools/profiler/core/platform.cpp @@ +825,5 @@ > { > ThreadInfo& currThreadInfo = *aSample->threadInfo; > > + currThreadInfo > + .addTag(ProfileBufferEntry::ThreadId(currThreadInfo.ThreadId())); I would break the line after the first '('. @@ +859,5 @@ > mozilla::TimeDuration delta = > currThreadInfo.GetThreadResponsiveness()->GetUnresponsiveDuration( > aSample->timestamp); > + currThreadInfo > + .addTag(ProfileBufferEntry::Responsiveness(delta.ToMilliseconds())); Ditto.
Attachment #8839468 -
Flags: review?(n.nethercote) → review+
Assignee | ||
Comment 5•7 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #3) > It would be nice to see the log message that you're planning to land with at > review time. No special messages planned. > How are you creating and uploading your patches? If you use |hg mq| Yeah, |hg mq| and manual uploading. Time to upgrade, it sounds like. > I wonder what all the |volatile|s in this code are supposed to be doing :/ Let's nuke them (in a new bug). AFAIK |volatile| has no place in modern thinking on concurrency. It's for writing device drivers. What do you say? > ::: tools/profiler/public/PseudoStack.h > @@ -48,5 @@ > > #else > > # error "Memory clobber not supported for your platform." > > #endif > > > > -// A stack entry exists to allow the JS engine to inform the Gecko Profiler of > > #include ProfilingStack.h in this file? It's already there. > ::: xpcom/threads/ThreadStackHelper.h > @@ -8,5 @@ > > #define mozilla_ThreadStackHelper_h > > > > #include "mozilla/ThreadHangStats.h" > > > > -#include "GeckoProfiler.h" > > I'm wondering where the definition of ProfileEntry is coming from in this > file. Unified builds make it hard to know. > > Perhaps #include ProfilingStack.h? Or just forward declare ProfileEntry in > this header and then #include ProfilingStack.h in ThreadStackHelper.cpp? Added. Yes, seems the safe thing to do.
![]() |
||
Comment 6•7 years ago
|
||
> Yeah, |hg mq| and manual uploading. Time to upgrade, it sounds like. |hg bzexport| is much better. > > I wonder what all the |volatile|s in this code are supposed to be doing :/ > > Let's nuke them (in a new bug). AFAIK |volatile| has no place in modern > thinking on concurrency. It's for writing device drivers. What do you say? I'm inclined to leave them in for now as a form of documentation. I.e. they indicate code that may need fixes to avoid races.
Pushed by jseward@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/5cb0e7cf804b Profiler tidyups: remove StackEntry, rename ProfileEntry to ProfileBufferEntry (part 1: remove StackEntry). r=n.nethercote. https://hg.mozilla.org/integration/mozilla-inbound/rev/6ef5dd99b45c Profiler tidyups: remove StackEntry, rename ProfileEntry to ProfileBufferEntry (part 2: rename ProfileEntry). r=n.nethercote.
Comment 8•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5cb0e7cf804b https://hg.mozilla.org/mozilla-central/rev/6ef5dd99b45c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•