1341255 - Profiler tidyups: remove StackEntry, rename ProfileEntry to ProfileBufferEntry

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Julian Seward [:jseward]]

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.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1341255-1-remove-StackEntry.cset

Comment 2

[Julian Seward [:jseward]]

Attachment: bug1341255-1-rename-ProfileEntry.cset

Comment 3

[Nicholas Nethercote [inactive]]

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?

Comment 4

[Nicholas Nethercote [inactive]]

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.

Comment 5

[Julian Seward [:jseward]]

(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

[Nicholas Nethercote [inactive]]

> 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.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5cb0e7cf804b
https://hg.mozilla.org/mozilla-central/rev/6ef5dd99b45c