1328385 - Replace the profile entry tag with an enum

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Markus Stange [:mstange]]

Attachment: 03-replace-profile-entry-tag-with-enum.patch

This is a patch that Kannan wrote in bug 1135236 but didn't finish.

Comment 1

[Julian Seward [:jseward]]

Attachment: bug1328385-1.cset

* rebased

* compared to the original, with all the static_cast<> removed as they
  are unnecessary.  We can just tell the compiler that the enum is to
  be stored in 8 bits.

* with OptInfoAddr renamed to JitReturnAddr as that is how it seems
  to be used

* with the mapping below, which I've checked for consistency

  y  ->  Category
  c  ->  CodeLocation
  d  ->  EmbeddedString
  f  ->  FrameNumber
  J  ->  JitReturnAddr
  n  ->  LineNumber
  l  ->  NativeLeafAddr
  m  ->  Marker
  R  ->  ResidentMemory
  r  ->  Responsiveness
  s  ->  Sample
  T  ->  ThreadId
  t  ->  Time
  U  ->  UnsharedMemory

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8834031 [details] [diff] [review]
bug1328385-1.cset

Review of attachment 8834031 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good.  Have some minor comments and suggestions, but mostly to get you to consider whether you want to incorporate them or not.  Thanks for landing this!

::: tools/profiler/core/ProfileEntry.cpp
@@ +35,4 @@
>  { }
>  
>  // aTagData must not need release (i.e. be a string from the text segment)
> +ProfileEntry::ProfileEntry(Kind aKind, const char *aTagData)

Just a suggestion, but couldn't you use the same generator-macro method you used to declare the constructors, to implement them?

If you move the relevant class field (e.g. mTagData) into the generator-macro as another parameter, these constructor implementations could all be macro-ified.

@@ +708,5 @@
>            sample->mStack = stack.GetOrAddIndex();
>            break;
>          }
> +      default:
> +        break;

Should there be a warning here?  The old code elides the default.  Is this a style thing to explicitly tell the reader "yes, we've handled all the cases we care about, ignore the rest"?

::: tools/profiler/core/ProfileEntry.h
@@ +38,5 @@
> +  _(Responsiveness,         float)            /* r */          \
> +  _(Sample,                 const char *)     /* s */          \
> +  _(ThreadId,               int)              /* T */          \
> +  _(Time,                   float)            /* t */          \
> +  _(UnsharedMemory,         float)            /* U */

I'm guessing the comments relating the new names to the older letters are unnecessary now?  It seems like they'd be confusing for somebody looking at the code not aware of the history of changes from char => enum.  Might be good to remove these if they no longer serve any purpose.

::: tools/profiler/core/Sampler.cpp
@@ +766,2 @@
>  {
> +  aInfo.addTag(ProfileEntry::CodeLocation(""));

Suggestion: calling this function addDynamicCodeLocationTag or something might be useful to readers of the call site code.

Took me a dxr search to figure out why you were able to eliminate the aTagName from the args.  Anyway, the previous code would hint at the call site what tag was being added (because "c").  Now it might be useful to move the hint into the method name.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/206f2c7479ad
Replace the profile entry tag with an enum.  r=kvijayan.

Comment 4

[Julian Seward [:jseward]]

(In reply to Kannan Vijayan [:djvj] from comment #2)
Thanks for the review.

> Just a suggestion, but couldn't you use the same generator-macro method you
> used to declare the constructors, to implement them?

Not easily, because the generator-macro is a once-per-sample-kind
thing whereas the constructors on a per-type basis, so there'd be
duplication for eg

  +  _(Time,                   float)            /* t */          \
  +  _(UnsharedMemory,         float)            /* U */


> @@ +708,5 @@
> >            sample->mStack = stack.GetOrAddIndex();
> >            break;
> >          }
> > +      default:
> > +        break;
> 
> Should there be a warning here?  The old code elides the default.  Is this a
> style thing to explicitly tell the reader "yes, we've handled all the cases
> we care about, ignore the rest"?

We're adding an enum class here, and g++ checks it more carefully than
a plain enum.  In particular it complains if not all values are
handled in a switch, so I had to add the default so as to avoid a
bunch of warnings.


> ::: tools/profiler/core/ProfileEntry.h
> @@ +38,5 @@
> > +  _(Responsiveness,         float)            /* r */          \
> > +  _(Sample,                 const char *)     /* s */          \
> > +  _(ThreadId,               int)              /* T */          \
> 
> I'm guessing the comments relating the new names to the older letters are
> unnecessary now?

Yes; rm'd.


> ::: tools/profiler/core/Sampler.cpp
> @@ +766,2 @@
> >  {
> > +  aInfo.addTag(ProfileEntry::CodeLocation(""));
> 
> Suggestion: calling this function addDynamicCodeLocationTag or something
> might be useful to readers of the call site code.

Done.


> Took me a dxr search to figure out why you were able to eliminate the
> aTagName from the args.

The removal was already done in your version of the patch!

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/206f2c7479ad