773428 - Support dynamic markers

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Dynamic markers will let us insert custom marker strings in the profile. This is particularly helpful for console.profile.

Comment 1

[Benoit Girard (:BenWa)]

I have a plan for implementing this.
1) strdup() all inputs, let the caller free the string
2) When adding a marker set a lock bool. During which any sample that come in will skip reading markers. This means markers wont appear until the next sample or so. Note this means we could infinitely postpone a marker but this is very unlikely and not critical.
3) Modify the marker stack, if there's no room remaining release the lock and sleep until the markers have been cleared.

Comment 2

[Benoit Girard (:BenWa)]

Attachment: patch

Approach is exactly as described.

Comment 3

[Benoit Girard (:BenWa)]

Comment on attachment 658364 [details] [diff] [review]
patch

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

::: tools/profiler/shared-libraries-linux.cc
@@ +41,5 @@
>  #include <dlfcn.h>
>  #include <sys/types.h>
>  
>  #ifdef ANDROID
> +extern "C" __attribute__((weak))

Ignore this, it's already on trunk anyways.

Comment 4

[Benoit Girard (:BenWa)]

Attachment: Part 2: Expose AddMarker via IDL

Comment 5

[(no longer active)]

Comment on attachment 658364 [details] [diff] [review]
patch

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

I think you want to implement this using a circular buffer.

Comment 6

[(no longer active)]

Comment on attachment 658472 [details] [diff] [review]
Part 2: Expose AddMarker via IDL

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

Nit: rev the iid please.

Comment 7

[Benoit Girard (:BenWa)]

Attachment: rebase

Comment 8

[Benoit Girard (:BenWa)]

(In reply to Ehsan Akhgari [:ehsan] from comment #5)
> Comment on attachment 658364 [details] [diff] [review]
> patch
> 
> Review of attachment 658364 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I think you want to implement this using a circular buffer.

We discussed this. A circular buffer would be useful because while we're adding a marker it's possible for a sample to fire, which can keep happening and keep delaying the marker from being logged. This is extremely unlikely and would show up in the profile as a sample in add_marker. Because of this we're going to land the patch without a circular buffer since it's impossibly likely and a concurrent circular buffer has a big potential for bugs.

Comment 9

[(no longer active)]

Comment on attachment 689951 [details] [diff] [review]
rebase

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

r- for the comment about strdup, but at least I need answers to all of my other questions for the next round.

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

Now that you're refactoring this, can you please rewrite this using memcpy?  I never liked this explicit loop...

::: tools/profiler/sps_sampler.h
@@ +285,5 @@
>      }
>      if (size_t(mMarkerPointer) == mozilla::ArrayLength(mMarkers)) {
>        return; //array full, silently drop
>      }
> +    mMarkers[mMarkerPointer] = strdup(aMarker);

Please move this before the write barrier.

@@ +295,5 @@
>  
>    // called within signal. Function must be reentrant
>    const char* getMarker(int aMarkerId)
>    {
> +    if (mSignalLock || aMarkerId < 0 || mQueueClearMarker ||

Why are you changing the behavior of mQueueClearMarker here?

@@ +306,5 @@
>    // called within signal. Function must be reentrant
>    void clearMarkers()
>    {
> +    for (size_t i = 0; i < mMarkerPointer; i++) {
> +      free(mMarkers[i]);

Can we free all of the markers, even if they're not dynamic?  (I guess they're all strdup'ed string now, but I'm not 100% sure.)

@@ +374,5 @@
>  
>    // Keep a list of active checkpoints
>    StackEntry volatile mStack[1024];
>    // Keep a list of active markers to be applied to the next sample taken
> +  char* mMarkers[1024];

Why are you removing the volatile?

@@ +379,4 @@
>   private:
>    // This may exceed the length of mStack, so instead use the stackSize() method
>    // to determine the number of valid samples in mStack
> +  mozilla::sig_safe_t mStackPointer;

Why are you removing the volatile?

@@ +441,5 @@
>      return;
>  
> +  if (!mozilla_sampler_is_active()) {
> +    return;
> +  }

Why was this not needed before?

Comment 10

[Benoit Girard (:BenWa)]

(In reply to Ehsan Akhgari [:ehsan] from comment #9)
> Comment on attachment 689951 [details] [diff] [review]
> rebase
> 
> Review of attachment 689951 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r- for the comment about strdup, but at least I need answers to all of my
> other questions for the next round.
> 
> ::: tools/profiler/TableTicker.cpp
> @@ +721,5 @@
> > +    // Store as many characters in the void* as the platform allows
> > +    char text[sizeof(void*)];
> > +    for (size_t pos = 0; pos < sizeof(void*) && j+pos < strLen; pos++) {
> > +      text[pos] = aStr[j+pos];
> > +    }
> 
> Now that you're refactoring this, can you please rewrite this using memcpy? 
> I never liked this explicit loop...

We can only do a memcpy for this inner loop, not the other. Which will only move a single word. Are you sure you want that as a memcpy?

> 
> @@ +295,5 @@
> >  
> >    // called within signal. Function must be reentrant
> >    const char* getMarker(int aMarkerId)
> >    {
> > +    if (mSignalLock || aMarkerId < 0 || mQueueClearMarker ||
> 
> Why are you changing the behavior of mQueueClearMarker here?

Because the clear+free needs to happen on the profiled thread. If the markers are queued for clear it means that they shouldn't be read. Before a new marker is pushed mQueueClearMarker will be reset the and the marker will be read.

> 
> @@ +306,5 @@
> >    // called within signal. Function must be reentrant
> >    void clearMarkers()
> >    {
> > +    for (size_t i = 0; i < mMarkerPointer; i++) {
> > +      free(mMarkers[i]);
> 
> Can we free all of the markers, even if they're not dynamic?  (I guess
> they're all strdup'ed string now, but I'm not 100% sure.)

They are all dynamic. We could avoid this by using tag pointers or increasing the storage and keeping a flag. But it just slightly adds to the overhead of adding a marker.

> 
> @@ +374,5 @@
> >  
> >    // Keep a list of active checkpoints
> >    StackEntry volatile mStack[1024];
> >    // Keep a list of active markers to be applied to the next sample taken
> > +  char* mMarkers[1024];
> 
> Why are you removing the volatile?
> 

because we can't free 'volatile char*' and it's not needed with the signal lock.
> @@ +379,4 @@
> >   private:
> >    // This may exceed the length of mStack, so instead use the stackSize() method
> >    // to determine the number of valid samples in mStack
> > +  mozilla::sig_safe_t mStackPointer;
> 
> Why are you removing the volatile?
> 

Because it's not needed with the signal lock.

> @@ +441,5 @@
> >      return;
> >  
> > +  if (!mozilla_sampler_is_active()) {
> > +    return;
> > +  }
> 
> Why was this not needed before?

Because we didn't allocate memory for labels. Now I only want them to get allocated and tracked if the profiler is running.

Comment 11

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 12

[(no longer active)]

Comment on attachment 690443 [details] [diff] [review]
patch

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

Please say in the commit message why you're removing the volatile's here when landing.  r=me with the comments.

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

Yeah, I think it's worth using memcpy here since without that the person reading this code thinks that this loop is doing something special.

::: tools/profiler/sps_sampler.h
@@ +296,5 @@
>  
>    // called within signal. Function must be reentrant
>    const char* getMarker(int aMarkerId)
>    {
> +    if (mSignalLock || aMarkerId < 0 || mQueueClearMarker ||

Please add a comment here explaining what this code is doing.

@@ +442,5 @@
>      return;
>  
> +  if (!mozilla_sampler_is_active()) {
> +    return;
> +  }

Please add a comment explaining this.

Comment 13

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=305aaf3bb1d4

Comment 14

[Benoit Girard (:BenWa)]

Attachment: patch

Carry forward r=ehsan. Wait on try to land. Meanwhile feel free to drive-by review the revisions.

Comment 15

[(no longer active)]

You're adding some new warnings, which can be fatal.

(Tip: to catch this type of problem locally, add ac_add_options --enable-warnings-as-error to your mozconfig.  I've found it quite useful.)

Comment 16

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=5dca57eb3b19

One more time. Also note to self not to forget to fix the rev id.

Comment 17

[Benoit Girard (:BenWa)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d75e7bfff554
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7e244b7ac8

Comment 18

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0b7e244b7ac8
https://hg.mozilla.org/mozilla-central/rev/973a98b6265f