Closed Bug 1341255 Opened 8 years ago Closed 8 years ago

Profiler tidyups: remove StackEntry, rename ProfileEntry to ProfileBufferEntry

Categories

(Core :: Gecko Profiler, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: jseward, Assigned: jseward)

References

Details

Attachments

(2 files)

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.
Attachment #8839468 - Attachment is patch: true
Attachment #8839467 - Flags: review?(n.nethercote)
Attachment #8839468 - Flags: review?(n.nethercote)
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 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+
(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.
> 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.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: