Closed Bug 873915 Opened 12 years ago Closed 8 years ago

Add env variables to control profiler feature and selected threads

Categories

(Core :: Gecko Profiler, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: BenWa, Assigned: BenWa)

References

Details

Attachments

(1 file, 4 obsolete files)

Attached patch patch (obsolete) — Splinter Review
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.
Attachment #751547 - Flags: review?(jseward)
Attachment #751547 - Attachment is patch: true
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.)
Attachment #751547 - Flags: review?(jseward) → review+
(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
Assignee: nobody → bgirard
Attached patch patch v2 (obsolete) — Splinter Review
Attachment #751547 - Attachment is obsolete: true
Attachment #756130 - Flags: review+
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #756130 - Attachment is obsolete: true
Attachment #769146 - Flags: review+
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.
Flags: needinfo?(bgirard)
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.
Attachment #769146 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Attachment #8535411 - Flags: review?(mstange)
Attachment #8535411 - Attachment is patch: true
Attachment #8535411 - Attachment is obsolete: true
Attachment #8535411 - Flags: review?(mstange)
Attachment #8535413 - Flags: review?(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?
Attachment #8535413 - Flags: review?(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?
This landed in another bug several years ago.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: