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

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

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

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

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

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

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

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?

This landed in another bug several years ago.