925111 - Enable Profiler to support config options during runtime

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Mason Chang [Inactive] [:mchang]]

Currently, the gecko profiler only reads profiler config options, such as MOZ_PROFILER_INTERVAL, during a clean reboot of the device. If the user starts profiling a single process, all config parameters are ignored other than a thread number. This bug tracks the work needed so that the profiler reads the config parameters without a clean reboot.

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: profilerEnv.patch

Modifies the b2g profiler to read profiler configuration from profiler.options everytime we start the profiler. Tested using mochitest, which passes, but probably doesn't run the b2g profiler. Are there other tests we can run?

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 815677 [details] [diff] [review]
profilerEnv.patch

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

Several comments but they should all be fairly minor, you're on the right track!

::: tools/profiler/platform-linux.cc
@@ +457,5 @@
>  const int SIGSTART = SIGUSR2;
>  
> +// Currently support only the env variables
> +// reported in read_profiler_env
> +static void ReadProfilerVars(const char* fileName, const char** features, 

nit: several trailing whitespace in this patch.

@@ +460,5 @@
> +// reported in read_profiler_env
> +static void ReadProfilerVars(const char* fileName, const char** features, 
> +                            uint32_t* featureCount, char* threadName, uint32_t* threadCount) {
> +  FILE* file = fopen(fileName, "r");
> +  const int bufferSize = 64;

64 is a bit small especially if you add support for features and thread names. How about 1024?

@@ +468,5 @@
> +
> +  if (file) {
> +    while (fgets(line, bufferSize, file) != NULL) {
> +      feature = strtok(line, "=");
> +      value = strtok(NULL, "");

strtok isn't threadsafe. Please use 'strtok_s'. There's some info here for you:
http://stackoverflow.com/questions/5999418/why-is-strtok-considered-unsafe

@@ +470,5 @@
> +    while (fgets(line, bufferSize, file) != NULL) {
> +      feature = strtok(line, "=");
> +      value = strtok(NULL, "");
> +
> +      if (0 == strncmp(feature, PROFILER_MODE, bufferSize)) {

Generally we prefer (func() == 0) even if this manner guards against accidental assignment.

@@ +479,5 @@
> +        set_profiler_entries(value);
> +      } else if (0 == strncmp(feature, PROFILER_STACK, bufferSize)) {
> +        set_profiler_scan(value);
> +      } else {
> +        // We just assume it's a thread now

I don't like that assumption. We also still have features to support. How about we support:
features=js,stackwalk
threads=GeckoMain,Compositor

where features & threads is an optional comma separated list.

::: tools/profiler/platform.cpp
@@ +265,5 @@
> +      return true;
> +    }
> +  }
> +
> +  return false; 

To preserve the semantics this should be return true, the other returns can just fall through here and on line 266 you need else { return false; }.

Similarly for the other functions. Right now you're printing the usage if they are null.

@@ +333,2 @@
>  
> +  // TODO: Get rid of gotos

It should be easy to do now:
if (!a || !b ...) {
  //usage
}

@@ +578,5 @@
>                             const char** aFeatures, uint32_t aFeatureCount,
>                             const char** aThreadNameFilters, uint32_t aFilterCount)
>  
>  {
> +  if (!stack_key_initialized) 

this change hunk should go away.

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: profilerEnv.patch

Updated patch, thanks for the comments! We pass on the env variables from the local host into the file, which is then read when we start the profiler. We read both features, and threads as CSV lists. This also incorporates Jed's work of defaulting js,leaf features enabled. I cleared all the trailing whitespace. Made the buffers 1024 char size. Used strtok_r (I saw that strtok_s is Windows specific). Fixed all the other comments you made :)

Comment 4

[Benoit Girard (:BenWa)]

Comment on attachment 816660 [details] [diff] [review]
profilerEnv.patch

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

Nice :D

::: tools/profiler/platform-linux.cc
@@ +457,5 @@
>  const int SIGSTART = SIGUSR2;
>  
> +static void freeArray(const char** array, int size) {
> +  for (int i = 0; i < size; i++) {
> +    free((void*) array[i]);

shouldn't need this check if we're const correct.

@@ +470,5 @@
> +    csvList[newlinePos] = '\0';
> +  }
> +  char* item = strtok_r(csvList, ",", &savePtr);
> +
> +  do {

Use the 'for' style instead:
for (uint32_t count = 0; item; item = strtok_r(csvList, ",", &savePtr)) {

@@ +526,3 @@
>    uint32_t threadCount = 0;
> +  // Assume we won't turn on more than 10 features
> +  // or profile more than 10 threads at a time

You assume this but don't check it.

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: profilerEnv.patch

Fixed last remaining issues in Comment 4. Keeping the arrays are const because we don't want to touch profiler_start() declaration all over the place.

Comment 6

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=09a93e132b0b

Comment 7

[Mason Chang [Inactive] [:mchang]]

https://tbpl.mozilla.org/?tree=Try&rev=314eafe9d90e

Comment 8

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

Comment on attachment 819217 [details] [diff] [review]
profilerEnv.patch

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

Just a couple of minor nits.  Looks good otherwise.

::: tools/profiler/platform-linux.cc
@@ +505,5 @@
> +      } else if (strncmp(feature, PROFILER_ENTRIES, bufferSize) == 0) {
> +        set_profiler_entries(value);
> +      } else if (strncmp(feature, PROFILER_STACK, bufferSize) == 0) {
> +        set_profiler_scan(value);
> +      } else if (strncmp(feature, "features", bufferSize) == 0) {

If we're also going to expose this as an environment variable later, maybe we want to pick a more environment-variable-like name?  Or do we not mind having the env vars and the option file keys be different?

::: tools/profiler/platform.cpp
@@ +320,5 @@
> +  return false;
> +#endif
> +}
> +
> +void usage() {

This function should be static and/or have a less ambiguous name.

@@ +371,5 @@
> +  const char* interval = PR_GetEnv(PROFILER_INTERVAL);
> +  const char* entries = PR_GetEnv(PROFILER_ENTRIES);
> +  const char* scanCount = PR_GetEnv(PROFILER_STACK);
> +
> +  // TODO: Get rid of gotos

It looks like this TODO is already done.

::: tools/profiler/platform.h
@@ +250,5 @@
> +bool set_profiler_mode(const char*);
> +bool set_profiler_interval(const char*);
> +bool set_profiler_entries(const char*);
> +bool set_profiler_scan(const char*);
> +bool is_native_avail();

This name is a little vague, given that it's globally visible.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: bug925111.patch

Updated with jlds comments. Only minor refactoring, r+ from previous patch.

Comment 10

[Mason Chang [Inactive] [:mchang]]

Try with new patch in comment 9

https://tbpl.mozilla.org/?tree=Try&rev=9d965a4325e1

Comment 11

[Mason Chang [Inactive] [:mchang]]

Attachment: bug925111.patch

Carrying reviews from other r+. Previous bug had a typo which failed the build. New tinderbox:

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

Comment 12

[Mason Chang [Inactive] [:mchang]]

Attachment: bug925111.patch

Carrying over r+, minor typo which failed builds. Successful try server:

https://tbpl.mozilla.org/?tree=Try&rev=264df0e7a03e

Comment 13

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/b2g-inbound/rev/c4d7092d2e3c

Comment 14

[Mason Chang [Inactive] [:mchang]]

Landed:

http://hg.mozilla.org/mozilla-central/rev/c4d7092d2e3c

Comment 15

[Ed Morley [:emorley]]

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