Add env variables to control profiler feature and selected threads

RESOLVED WORKSFORME

Status

()

RESOLVED WORKSFORME
6 years ago
2 years ago

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

6 years ago
Created attachment 751547 [details] [diff] [review]
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.
Attachment #751547 - Flags: review?(jseward)
(Assignee)

Updated

6 years ago
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+
(Assignee)

Comment 2

6 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

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

Updated

6 years ago
Assignee: nobody → bgirard
(Assignee)

Comment 4

6 years ago
Created attachment 756130 [details] [diff] [review]
patch v2
Attachment #751547 - Attachment is obsolete: true
Attachment #756130 - Flags: review+
(Assignee)

Comment 5

5 years ago
Created attachment 769146 [details] [diff] [review]
patch v3

rebased, fix some errors:
https://tbpl.mozilla.org/?tree=Try&rev=289a8eb08df2
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.
(Assignee)

Comment 7

4 years ago
ni? so I don't forget to land this again.
Flags: needinfo?(bgirard)
(Assignee)

Comment 8

4 years ago
Created attachment 8535411 [details] [diff] [review]
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.
Attachment #769146 - Attachment is obsolete: true
Flags: needinfo?(bgirard)
Attachment #8535411 - Flags: review?(mstange)
(Assignee)

Updated

4 years ago
Attachment #8535411 - Attachment is patch: true
(Assignee)

Comment 9

4 years ago
Created attachment 8535413 [details] [diff] [review]
patch v4 (rebased, redid split function)

https://tbpl.mozilla.org/?tree=Try&rev=05189ccb534b
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?
(Assignee)

Comment 12

2 years ago
This landed in another bug several years ago.
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.