873915 - Add env variables to control profiler feature and selected threads

Status: RESOLVED WORKSFORME

(Core :: Gecko Profiler, defect)

Description

[Benoit Girard (:BenWa)]

Attachment: patch

Right now we need to hardcore which features and threads we want to profile by default. If we control these using an environment variable we can control this when profiling from startup which includes the b2g profiling script.

Comment 1

[Julian Seward [:jseward]]

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

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

r+ provided you address all the comments except for the one about
reimplementing a C stdlib function (fixing that would be a 
nice-to-have but not essential.)

::: tools/profiler/platform.cpp
@@ +157,5 @@
>    }
>  }
>  
> +static
> +char** StringSplit(const char* aStr, char aSeperator)

Add a comment to this saying (1) what it does and (2) what the ownership of the resulting memory is [afaics, the caller owns both the top level array and all the strings hanging off it.]

I have the nagging feeling that you're basically reimplementing a C standard library function, or something pretty close to that.  But I can't figure out which one.  strtok_r?  Not exactly the same, but maybe similar enough to be of some help?

@@ +198,5 @@
> +  currPart++;
> +
> +  for (uint32_t i = 0; i < strParts; i++) {
> +    printf("Found part %s\n", parts[i]);
> +  }

Should this loop be here?  Looks like debugging code.

@@ +267,3 @@
>    goto out;
>  
>   usage:

Pls update the usage message accordingly (if that's appropriate; but I get the impression it is.)

Comment 2

[Benoit Girard (:BenWa)]

(In reply to Julian Seward from comment #1)
> I have the nagging feeling that you're basically reimplementing a C standard
> library function, or something pretty close to that.  But I can't figure out
> which one.  strtok_r?  Not exactly the same, but maybe similar enough to be
> of some help?

Ohh, I didn't know there was a sane variant of strtok. I look into using it.

Comment 3

[Benoit Girard (:BenWa)]

Push to try (along with several other pathes):
https://tbpl.mozilla.org/?tree=Try&rev=0e4f1c30b845

Comment 4

[Benoit Girard (:BenWa)]

Attachment: patch v2

Comment 5

[Benoit Girard (:BenWa)]

Attachment: patch v3

rebased, fix some errors:
https://tbpl.mozilla.org/?tree=Try&rev=289a8eb08df2

Comment 6

[Markus Stange [:mstange]]

This never landed, which is why the thread environment variables don't have any effect.

Comment 7

[Benoit Girard (:BenWa)]

ni? so I don't forget to land this again.

Comment 8

[Benoit Girard (:BenWa)]

Attachment: patch v4 (rebased, redid split function)

I really didn't like my old split function. I think I was a bit ashamed to land it before :). This one is much simpler.

Comment 9

[Benoit Girard (:BenWa)]

Attachment: patch v4 (rebased, redid split function)

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

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8535413 [details] [diff] [review]
patch v4 (rebased, redid split function)

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

I don't really like all the code needed for the string array memory management. Can we instead do something like this?
(I know, I'm asking for quite a big change here, but I think it's worth it.)

struct ProfilerEnvSettings
{
  int unwindInterval;
  int profileEntries;
  std::vector<std::string> features;
  std::vector<std::string> threadFilters;
};

static ProfilerEnvSettings* sProfilerEnvSettings = nullptr;

[...]

sProfilerEnvSettings = read_profiler_env_vars();

[...]

In read_profiler_env_vars():
ProfilerEnvSettings* settings = new ProfilerEnvSetting();
settings->features = split(features, ",");

[...]

somewhere:
if (!settings || settings->features->empty()) {
  set default features
}

[...]

delete sProfilerEnvSettings;
sProfilerEnvSettings = nullptr;

::: tools/profiler/platform.cpp
@@ +365,5 @@
> +
> +static
> +std::vector<std::string> split(const std::string &s, char delim) {
> +  std::vector<std::string> elems;
> +  split(s, delim, elems);

Why not combine the two split functions? The way the first is written (returning a reference to a parameter) seems very strange to me.

@@ +639,5 @@
> +  const char** selectedFeature = defaultFeatures;
> +  uint32_t featureCount = sizeof(defaultFeatures)/sizeof(const char*);
> +
> +  if (sStartupFeatures) {
> +    selectedFeature = const_cast<const char**>(sStartupFeatures);

I thought you only needed const_cast when putting something const into a non-const variable, not the other way round. Why is it needed here?

Comment 11

[Markus Stange [:mstange]]

Oh, and I guess we'd need to change a few of the profiler starting functions to accept std::vector<std::string> instead of const char**, and then add a "const char** -> std::vector<std::string> conversion pass to the existing functions. Hmm... ideas?

Comment 12

[Benoit Girard (:BenWa)]

This landed in another bug several years ago.