If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Support dynamic markers

RESOLVED FIXED in mozilla20

Status

()

Core
Gecko Profiler
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

unspecified
mozilla20
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Assignee)

Description

5 years ago
Dynamic markers will let us insert custom marker strings in the profile. This is particularly helpful for console.profile.
(Assignee)

Comment 1

5 years ago
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.
(Assignee)

Comment 2

5 years ago
Created attachment 658364 [details] [diff] [review]
patch

Approach is exactly as described.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #658364 - Flags: review?(ehsan)
(Assignee)

Comment 3

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 788400
(Assignee)

Comment 4

5 years ago
Created attachment 658472 [details] [diff] [review]
Part 2: Expose AddMarker via IDL
Attachment #658472 - Flags: review?(ehsan)
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.
Attachment #658364 - Flags: review?(ehsan)
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.
Attachment #658472 - Flags: review?(ehsan) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 689951 [details] [diff] [review]
rebase
Attachment #658364 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #689951 - Flags: review?(ehsan)
(Assignee)

Comment 8

5 years ago
(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 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?
Attachment #689951 - Flags: review?(ehsan) → review-
(Assignee)

Comment 10

5 years ago
(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.
(Assignee)

Comment 11

5 years ago
Created attachment 690443 [details] [diff] [review]
patch
Attachment #689951 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #690443 - Flags: review?(ehsan)
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.
Attachment #690443 - Flags: review?(ehsan) → review+
(Assignee)

Comment 13

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=305aaf3bb1d4
Flags: needinfo?(bgirard)
(Assignee)

Comment 14

5 years ago
Created attachment 690685 [details] [diff] [review]
patch

Carry forward r=ehsan. Wait on try to land. Meanwhile feel free to drive-by review the revisions.
Attachment #690685 - Flags: review+
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.)
(Assignee)

Comment 16

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

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

Comment 17

5 years ago
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d75e7bfff554
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0b7e244b7ac8
Flags: needinfo?(bgirard)
https://hg.mozilla.org/mozilla-central/rev/0b7e244b7ac8
https://hg.mozilla.org/mozilla-central/rev/973a98b6265f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Assignee)

Updated

5 years ago
Duplicate of this bug: 771102
You need to log in before you can comment on or make changes to this bug.