Closed
Bug 1328385
Opened 8 years ago
Closed 8 years ago
Replace the profile entry tag with an enum
Categories
(Core :: Gecko Profiler, defect)
Core
Gecko Profiler
Tracking
()
RESOLVED
FIXED
mozilla54
People
(Reporter: mstange, Assigned: jseward)
References
Details
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
djvj
:
review+
|
Details | Diff | Splinter Review |
This is a patch that Kannan wrote in bug 1135236 but didn't finish.
Assignee | ||
Comment 1•8 years ago
|
||
* 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
Attachment #8823402 -
Attachment is obsolete: true
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → jseward
Assignee | ||
Updated•8 years ago
|
Attachment #8834031 -
Flags: review?(kvijayan)
Comment 2•8 years ago
|
||
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.
Attachment #8834031 -
Flags: review?(kvijayan) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/206f2c7479ad
Replace the profile entry tag with an enum. r=kvijayan.
Assignee | ||
Comment 4•8 years ago
|
||
(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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in
before you can comment on or make changes to this bug.
Description
•