Closed
Bug 1341255
Opened 8 years ago
Closed 8 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•8 years ago
|
||
Assignee | ||
Comment 2•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #8839468 -
Attachment is patch: true
Assignee | ||
Updated•8 years ago
|
Attachment #8839467 -
Flags: review?(n.nethercote)
Assignee | ||
Updated•8 years ago
|
Attachment #8839468 -
Flags: review?(n.nethercote)
![]() |
||
Comment 3•8 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•8 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•8 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•8 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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5cb0e7cf804b
https://hg.mozilla.org/mozilla-central/rev/6ef5dd99b45c
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.
Description
•