707308 - Support dynamic stack labels for profile

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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)]

Attachment: WIP

Comment 2

[Benoit Girard (:BenWa)]

Attachment: 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)]

Attachment: Part 1: Add dynamic labels

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Part 2: Show JS URL

Comment 5

[Benoit Girard (:BenWa)]

Example:
file:///Users/bgirard/ben/sps/cleopatra/index.html?report=AMIfv97liONaWJ92kkACmY6SAW_PmAYsAy-kifO-4AmTWBXeyRbtrA9OVYzg8ND9r5fOJcN7sjk9tLjUIga3hSLP4TTXYahYBcs9umdN-9M1p9f_-DHw0EduYg6_-sjisyJavzsrN3I2URCBc3A1_W4oGngVApzwLA

Comment 6

[(no longer active)]

avoid gotos, and hard coded array sizes. also avoid incrementing variables on one line,and decrementing them on the next!

Comment 7

[(no longer active)]

please use default arguments.

Comment 8

[(no longer active)]

r+ with comments in the bug and over lunch.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add dynamic labels (v2)

Jeff asked to review this.

Comment 10

[Benoit Girard (:BenWa)]

Attachment: 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)]

Attachment: Part 4: Annotate plugin mimetype + url during loading

Comment 12

[Josh Aas]

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

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

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

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)]

(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)]

Attachment: Part 1: Add dynamic labels (v3)

Comment 18

[Benoit Girard (:BenWa)]

Attachment: Part 3: Support JSON responsiveness

Fixed a verbal review comment. (resp->responsiveness)
Carrying forward r+

Comment 19

[Benoit Girard (:BenWa)]

Attachment: Part 1: Add dynamic labels (v3)

This was missing a change

Comment 20

[Benoit Girard (:BenWa)]

(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]]

(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]]

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)]

Attachment: Part 1: Add dynamic labels (v4)

Carry forward r=jrmuizel

Comment 24

[Benoit Girard (:BenWa)]

Attachment: 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)]

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

https://hg.mozilla.org/mozilla-central/rev/0e8522d513c2

Comment 27

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b9eabf51c50a

Comment 28

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/b9eabf51c50a