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)
Tracking
()
RESOLVED
WORKSFORME
People
(Reporter: BenWa, Assigned: BenWa)
References
Details
Attachments
(1 file, 4 obsolete files)
9.55 KB,
patch
|
Details | Diff | 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)
Assignee | ||
Updated•12 years ago
|
Attachment #751547 -
Attachment is patch: true
Comment 1•12 years ago
|
||
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+
Assignee | ||
Comment 2•12 years ago
|
||
(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.
Assignee | ||
Comment 3•12 years ago
|
||
Push to try (along with several other pathes):
https://tbpl.mozilla.org/?tree=Try&rev=0e4f1c30b845
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bgirard
Assignee | ||
Comment 4•12 years ago
|
||
Attachment #751547 -
Attachment is obsolete: true
Attachment #756130 -
Flags: review+
Assignee | ||
Comment 5•11 years ago
|
||
rebased, fix some errors:
https://tbpl.mozilla.org/?tree=Try&rev=289a8eb08df2
Attachment #756130 -
Attachment is obsolete: true
Attachment #769146 -
Flags: review+
Comment 6•10 years ago
|
||
This never landed, which is why the thread environment variables don't have any effect.
Assignee | ||
Comment 7•10 years ago
|
||
ni? so I don't forget to land this again.
Flags: needinfo?(bgirard)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8535411 -
Attachment is patch: true
Assignee | ||
Comment 9•10 years ago
|
||
Attachment #8535411 -
Attachment is obsolete: true
Attachment #8535411 -
Flags: review?(mstange)
Attachment #8535413 -
Flags: review?(mstange)
Comment 10•10 years ago
|
||
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)
Comment 11•10 years ago
|
||
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?
Assignee | ||
Comment 12•8 years ago
|
||
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.
Description
•