Last Comment Bug 707308 - Support dynamic stack labels for profile
: Support dynamic stack labels for profile
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Gecko Profiler (show other bugs)
: unspecified
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks: 713227 761261 766579 785287
  Show dependency treegraph
 
Reported: 2011-12-02 13:57 PST by Jeff Muizelaar [:jrmuizel]
Modified: 2013-01-17 08:30 PST (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (9.94 KB, patch)
2012-06-18 11:29 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
WIP (v2) works but needs clean up (17.34 KB, text/plain)
2012-06-18 22:17 PDT, Benoit Girard (:BenWa)
no flags Details
Part 1: Add dynamic labels (14.23 KB, patch)
2012-06-19 08:47 PDT, Benoit Girard (:BenWa)
ehsan: review+
Details | Diff | Splinter Review
Part 2: Show JS URL (4.34 KB, patch)
2012-06-19 08:49 PDT, Benoit Girard (:BenWa)
ehsan: feedback+
Details | Diff | Splinter Review
Part 1: Add dynamic labels (v2) (14.12 KB, patch)
2012-06-19 12:22 PDT, Benoit Girard (:BenWa)
jmuizelaar: review-
Details | Diff | Splinter Review
Part 3: Support JSON responsiveness (1.12 KB, patch)
2012-06-19 13:02 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 4: Annotate plugin mimetype + url during loading (1.26 KB, patch)
2012-06-19 13:36 PDT, Benoit Girard (:BenWa)
john: review+
Details | Diff | Splinter Review
Part 1: Add dynamic labels (v3) (15.45 KB, patch)
2012-06-20 14:21 PDT, Benoit Girard (:BenWa)
no flags Details | Diff | Splinter Review
Part 3: Support JSON responsiveness (1.18 KB, patch)
2012-06-20 16:23 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review
Part 1: Add dynamic labels (v3) (15.50 KB, patch)
2012-06-20 16:31 PDT, Benoit Girard (:BenWa)
jmuizelaar: review+
Details | Diff | Splinter Review
Part 1: Add dynamic labels (v4) (15.55 KB, patch)
2012-06-26 14:41 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review
Part 1: Add dynamic labels (v5) (16.06 KB, patch)
2012-06-26 20:26 PDT, Benoit Girard (:BenWa)
bgirard: review+
Details | Diff | Splinter Review

Description Jeff Muizelaar [:jrmuizel] 2011-12-02 13:57:58 PST
We'll want to be able to add some dynamic information like the page that we're painting or the javascript script that we're running.
Comment 1 Benoit Girard (:BenWa) 2012-06-18 11:29:22 PDT
Created attachment 634122 [details] [diff] [review]
WIP
Comment 2 Benoit Girard (:BenWa) 2012-06-18 22:17:17 PDT
Created attachment 634298 [details]
WIP (v2) works but needs clean up

right now we always copy stack entries. The tagged pointers didn't work since pointers don't always have any particular alignment.
Comment 3 Benoit Girard (:BenWa) 2012-06-19 08:47:28 PDT
Created attachment 634448 [details] [diff] [review]
Part 1: Add dynamic labels
Comment 4 Benoit Girard (:BenWa) 2012-06-19 08:49:59 PDT
Created attachment 634451 [details] [diff] [review]
Part 2: Show JS URL
Comment 5 Benoit Girard (:BenWa) 2012-06-19 09:06:55 PDT
Example:
file:///Users/bgirard/ben/sps/cleopatra/index.html?report=AMIfv97liONaWJ92kkACmY6SAW_PmAYsAy-kifO-4AmTWBXeyRbtrA9OVYzg8ND9r5fOJcN7sjk9tLjUIga3hSLP4TTXYahYBcs9umdN-9M1p9f_-DHw0EduYg6_-sjisyJavzsrN3I2URCBc3A1_W4oGngVApzwLA
Comment 6 :Ehsan Akhgari (away Aug 1-5) 2012-06-19 09:42:52 PDT
avoid gotos, and hard coded array sizes. also avoid incrementing variables on one line,and decrementing them on the next!
Comment 7 :Ehsan Akhgari (away Aug 1-5) 2012-06-19 09:57:51 PDT
please use default arguments.
Comment 8 :Ehsan Akhgari (away Aug 1-5) 2012-06-19 10:07:44 PDT
r+ with comments in the bug and over lunch.
Comment 9 Benoit Girard (:BenWa) 2012-06-19 12:22:18 PDT
Created attachment 634539 [details] [diff] [review]
Part 1: Add dynamic labels (v2)

Jeff asked to review this.
Comment 10 Benoit Girard (:BenWa) 2012-06-19 13:02:37 PDT
Created attachment 634557 [details] [diff] [review]
Part 3: Support JSON responsiveness

We need to enable JSON format support and this patch should solve the last regression of the JSON format (not supporting responsiveness).

I've already pushed the required changes to the Add-on and cleopatra.
Comment 11 Benoit Girard (:BenWa) 2012-06-19 13:36:50 PDT
Created attachment 634571 [details] [diff] [review]
Part 4: Annotate plugin mimetype + url during loading
Comment 12 Josh Aas 2012-06-19 13:39:35 PDT
Comment on attachment 634571 [details] [diff] [review]
Part 4: Annotate plugin mimetype + url during loading

I'd like John Schoenick to do this review.
Comment 13 John Schoenick [:johns] 2012-06-19 15:16:29 PDT
Comment on attachment 634571 [details] [diff] [review]
Part 4: Annotate plugin mimetype + url during loading

This looks good to me -- with the caveat that the aURI = baseURI block right above that may be going away in the future (I believe it is unnecessary after bug 745030 lands), so aURI may be null (and will always be null for java, for instance)
Comment 14 Jeff Muizelaar [:jrmuizel] 2012-06-20 07:58:16 PDT
Comment on attachment 634539 [details] [diff] [review]
Part 1: Add dynamic labels (v2)

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

::: tools/profiler/TableTicker.cpp
@@ +249,5 @@
> +      const char* tagStringData = entry.mTagData;
> +      int readAheadPos = (readPos + 1) % mEntrySize;
> +      char tagBuff[SAMPLER_MAX_STRING];
> +      tagBuff[0] = '\0';
> +      tagBuff[SAMPLER_MAX_STRING-1] = '\0';

Why do we need to '\0' these two particular values?

@@ +259,5 @@
> +        while (readAheadPos != mLastFlushPos && !seenNullByte) {
> +          incBy++;
> +          ProfileEntry readAheadEntry = mEntries[readAheadPos];
> +          for (size_t pos = 0; pos < sizeof(void*)/sizeof(char); pos++) {
> +            tagBuff[tagBuffPos] = ((char*)(&readAheadEntry.mTagData))[pos];

Can you use a union here instead of type punning?

@@ +270,5 @@
> +          if (!seenNullByte)
> +            readAheadPos = (readAheadPos + 1) % mEntrySize;
> +        }
> +        tagStringData = tagBuff;
> +      }

Can this be a separate function? It adds a lot of bulk to this function. It would also be nice if there were some tests. As I read through it, I can't tell whether it is correct or not.

@@ +599,5 @@
> +    // First entry has tagName 's' (start)
> +    // Check for magic pointer bit 1 to indicate copy
> +    bool isCopyString = aStack->mStack[i].isCopyLabel();
> +    const char* sampleLabel = aStack->mStack[i].mLabel;
> +    if (isCopyString) {

You could just check isCopyLabel() directly instead of giving it a name.

@@ +610,5 @@
> +      for (size_t j = 0; j < strLen;) {
> +        // Store as many characters in the void* as the platform allows
> +        void* text = 0;
> +        for (size_t pos = 0; pos < sizeof(void*)/sizeof(char) && j+pos < strLen; pos++) {
> +          ((char*)&text)[pos] = sampleLabel[j+pos];

union vs type punning again.

@@ +711,5 @@
>      snprintf(tagBuff, 1024, "l-%#llx\n", pc);
>      stream << tagBuff;
> +  } else if (entry.mTagName == 'd') {
> +    void* text = 0;
> +    printf("d tag\n");

You probably don't want this.

::: tools/profiler/sampler.h
@@ +71,5 @@
>  #define SAMPLER_GET_RESPONSIVENESS() NULL
>  #define SAMPLER_GET_FEATURES() NULL
>  #define SAMPLE_LABEL(name_space, info)
> +// Provide a default literal string to use if profiling is disabled
> +// and a printf argument to be computed if profiling is enabled.

Might be worth adding a note about stack usage.

::: tools/profiler/sps_sampler.h
@@ +169,5 @@
> +      _vsnprintf(buff, SAMPLER_MAX_STRING, aFormat, args);
> +      _snprintf(mDest, SAMPLER_MAX_STRING, "%s %s", aDefault, buff);
> +#else
> +      vsnprintf(buff, SAMPLER_MAX_STRING, aFormat, args);
> +      snprintf(mDest, SAMPLER_MAX_STRING, "%s %s", aDefault, buff);

The double printing here is sort of crappy, but at least it is safe and easy to read. You may want to add a comment about why you're doing it.

@@ +181,5 @@
> +  ~SamplerStackFramePrintfRAII() {
> +    mozilla_sampler_call_exit(mHandle);
> +  }
> +private:
> +  char mDest[SAMPLER_MAX_STRING];

I don't really like the fact that we pay this this stack size penalty regardless of whether we're profiling or not. I have no suggestions for how to avoid that though.

@@ +190,5 @@
>  
> +class StackEntry
> +{
> +public:
> +  static const void* EncodeStackAddress(const void* aStackAddress, bool aCopy) {

This could probably use a description of what you're doing
and why it's safe. Though I suppose some of that is by the definition of mStackAddress

@@ +191,5 @@
> +class StackEntry
> +{
> +public:
> +  static const void* EncodeStackAddress(const void* aStackAddress, bool aCopy) {
> +    aStackAddress = (const void*)((uintptr_t)aStackAddress & ~0x1);

You should also probably use c++ casts
Comment 15 Jeff Muizelaar [:jrmuizel] 2012-06-20 08:11:56 PDT
Another thing I thought of was that it is probably a good idea to split the formatted string from the the other part of the label in the json'd data so that filtering is easier in the front end.
Comment 16 Benoit Girard (:BenWa) 2012-06-20 08:29:30 PDT
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> @@ +181,5 @@
> > +  ~SamplerStackFramePrintfRAII() {
> > +    mozilla_sampler_call_exit(mHandle);
> > +  }
> > +private:
> > +  char mDest[SAMPLER_MAX_STRING];
> 
> I don't really like the fact that we pay this this stack size penalty
> regardless of whether we're profiling or not. I have no suggestions for how
> to avoid that though.
> 

We could solve this by processing the printf at sample time. This has the benefit of eliminating the stack size penalty and the run time over head. The drawback is that we now have to guarantee that the parameters are constant throughout the scope and we need to either find a signal safe implementation of printf or roll out a custom limited variant.
Comment 17 Benoit Girard (:BenWa) 2012-06-20 14:21:58 PDT
Created attachment 635065 [details] [diff] [review]
Part 1: Add dynamic labels (v3)
Comment 18 Benoit Girard (:BenWa) 2012-06-20 16:23:18 PDT
Created attachment 635114 [details] [diff] [review]
Part 3: Support JSON responsiveness

Fixed a verbal review comment. (resp->responsiveness)
Carrying forward r+
Comment 19 Benoit Girard (:BenWa) 2012-06-20 16:31:18 PDT
Created attachment 635115 [details] [diff] [review]
Part 1: Add dynamic labels (v3)

This was missing a change
Comment 20 Benoit Girard (:BenWa) 2012-06-20 16:41:49 PDT
(In reply to John Schoenick [:johns] from comment #13)
> Comment on attachment 634571 [details] [diff] [review]
> Part 4: Annotate plugin mimetype + url during loading
> 
> This looks good to me -- with the caveat that the aURI = baseURI block right
> above that may be going away in the future (I believe it is unnecessary
> after bug 745030 lands), so aURI may be null (and will always be null for
> java, for instance)

What's the best way to resolve that? To keep that block there, or have a null check and only annotate the plugin URI if it's not null?
Comment 21 John Schoenick [:johns] 2012-06-21 00:43:19 PDT
(In reply to Benoit Girard (:BenWa) from comment #20)
> What's the best way to resolve that? To keep that block there, or have a
> null check and only annotate the plugin URI if it's not null?

A comment that we intend to be logging the baseURI if aURI is null would probably be sufficient, so whoever ends up fixing up that function can preserve it (we just don't want to be assigning it into aURI as that's passed onwards to pluginHost)
Comment 22 Jeff Muizelaar [:jrmuizel] 2012-06-26 14:23:36 PDT
Comment on attachment 635115 [details] [diff] [review]
Part 1: Add dynamic labels (v3)

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

::: tools/profiler/TableTicker.cpp
@@ +616,5 @@
> +      size_t strLen = strlen(sampleLabel) + 1;
> +      for (size_t j = 0; j < strLen;) {
> +        // Store as many characters in the void* as the platform allows
> +        char text[sizeof(void*)/sizeof(char)];
> +        for (size_t pos = 0; pos < sizeof(void*)/sizeof(char) && j+pos < strLen; pos++) {

sizeof(char) is defined as 1 so you can remove all of them.
Comment 23 Benoit Girard (:BenWa) 2012-06-26 14:41:49 PDT
Created attachment 636879 [details] [diff] [review]
Part 1: Add dynamic labels (v4)

Carry forward r=jrmuizel
Comment 24 Benoit Girard (:BenWa) 2012-06-26 20:26:21 PDT
Created attachment 636981 [details] [diff] [review]
Part 1: Add dynamic labels (v5)

Increase the size of dynamic labels to 512 to support big JS strings. Note: I kept the size of the RAII helper to 128.
Comment 25 Benoit Girard (:BenWa) 2012-06-27 12:40:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e8522d513c2

Going to land the other parts once we figure how to shutdown JS properly and have Js and non Js dynamic label co-exists.
Comment 26 Ed Morley [:emorley] 2012-06-28 01:09:22 PDT
https://hg.mozilla.org/mozilla-central/rev/0e8522d513c2
Comment 28 Ed Morley [:emorley] 2012-06-29 00:48:04 PDT
https://hg.mozilla.org/mozilla-central/rev/b9eabf51c50a

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