Last Comment Bug 718440 - Add more sampler labels to various places
: Add more sampler labels to various places
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: General (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: mozilla12
Assigned To: Jeff Muizelaar [:jrmuizel]
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-01-16 08:38 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2012-01-19 10:53 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Some more labels (5.31 KB, patch)
2012-01-16 08:45 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
labels around network io (4.81 KB, patch)
2012-01-16 08:46 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
Labels for debugging the github problem (7.52 KB, patch)
2012-01-16 08:48 PST, Jeff Muizelaar [:jrmuizel]
no flags Details | Diff | Splinter Review
All in one patch (17.62 KB, patch)
2012-01-17 12:59 PST, Jeff Muizelaar [:jrmuizel]
b56girard: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2012-01-16 08:38:40 PST

    
Comment 1 Jeff Muizelaar [:jrmuizel] 2012-01-16 08:45:48 PST
Created attachment 588899 [details] [diff] [review]
Some more labels
Comment 2 Jeff Muizelaar [:jrmuizel] 2012-01-16 08:46:53 PST
Created attachment 588901 [details] [diff] [review]
labels around network io
Comment 3 Jeff Muizelaar [:jrmuizel] 2012-01-16 08:48:30 PST
Created attachment 588903 [details] [diff] [review]
Labels for debugging the github problem
Comment 4 Benoit Girard (:BenWa) 2012-01-16 11:44:03 PST
Comment on attachment 588903 [details] [diff] [review]
Labels for debugging the github problem

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

The patches looks good. It doesn't look like we're adding sampling to any performance critical code. I can r+ the cocoa changes but maybe we need someone else to review a patch that touch other modules. They just need to 'ok' adding a few cycles to these functions.

::: tools/profiler/sampler.h
@@ +106,4 @@
>  #define SAMPLER_GET_RESPONSIVENESS() NULL
>  #define SAMPLER_GET_FEATURES() NULL
>  #define SAMPLE_LABEL(name_space, info)
> +#define SAMPLE_LABEL_FN(name_space, info)

We should just pick a single standard and use it everywhere for all 3 patches. I think I would vote for that to be:
SAMPLE_LABEL_FN

The info parameter here isn't needed.

Do you think PRETTY_FUNCTION is to verbose? Maybe using __func__ is enough. To be honest I think PRETTY_FUNCTION is too verbose because it include parameters and return type, and that __func__ is ambiguous because it doesn't include namespace.
Comment 5 Jeff Muizelaar [:jrmuizel] 2012-01-17 12:59:01 PST
Created attachment 589273 [details] [diff] [review]
All in one patch

This also gets rid of the function macros because they don't provide what we want.

The naming is still pretty adhoc throughout as I'm not really sure what a good convention should be. I think we can figure that out as we add more labels and use them more.
Comment 8 Matt Brubeck (:mbrubeck) 2012-01-19 10:53:04 PST
https://hg.mozilla.org/mozilla-central/rev/147c00d2ca3b

Please comment in bugs when they land on inbound, especially if they've been backed out and/or re-landed.  It takes some of the pain out of merging.

Note You need to log in before you can comment on or make changes to this bug.