Allow specifying a sub-millisecond interval for startup profiling

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: mstange, Assigned: kmag)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(2 attachments)

The environment variable MOZ_PROFILER_INTERVAL value is parsed using strtol, so it can only handle integers at the moment. It would be better to handle floating point millisecond values.
I was just about to file a bug for this, and for the talos command line arguments (and have already fixed it locally). 1ms is way too long an interval to get useful profiling data for the things I'm looking at.
Assignee: nobody → kmaglione+bmo
Comment on attachment 8867859 [details]
Bug 1348031: Part 1 - Allow specifying fractional-millisecond startup profiling interval.

https://reviewboard.mozilla.org/r/139400/#review142726

::: tools/profiler/core/platform.cpp:2081
(Diff revision 1)
>  
> -    int interval = PROFILER_DEFAULT_INTERVAL;
> +    double interval = PROFILER_DEFAULT_INTERVAL;
>      const char* startupInterval = getenv("MOZ_PROFILER_STARTUP_INTERVAL");
>      if (startupInterval) {
>        errno = 0;
> -      interval = strtol(startupInterval, nullptr, 10);
> +      interval = strtod(startupInterval, nullptr);

Is this going to work on systems with a French / German OS locale where the decimal separator is a comma? I'd expect not.

Can you use Decimal::fromString(stdString).toDouble() instead? That seems to be the simplest way of using StringToDoubleConverter.

::: tools/profiler/core/platform.cpp:2082
(Diff revision 1)
> -    int interval = PROFILER_DEFAULT_INTERVAL;
> +    double interval = PROFILER_DEFAULT_INTERVAL;
>      const char* startupInterval = getenv("MOZ_PROFILER_STARTUP_INTERVAL");
>      if (startupInterval) {
>        errno = 0;
> -      interval = strtol(startupInterval, nullptr, 10);
> -      if (errno == 0 && 1 <= interval && interval <= 1000) {
> +      interval = strtod(startupInterval, nullptr);
> +      if (errno == 0 && interval > 0. && interval <= 1000.) {

I'd prefer 0.0 and 1000.0
Attachment #8867859 - Flags: review?(mstange)
Comment on attachment 8867860 [details]
Bug 1348031: Part 2 - Accept fractional-millisecond profiling intervals in talos.

https://reviewboard.mozilla.org/r/139402/#review142736
Attachment #8867860 - Flags: review?(mstange) → review+
Comment on attachment 8867859 [details]
Bug 1348031: Part 1 - Allow specifying fractional-millisecond startup profiling interval.

https://reviewboard.mozilla.org/r/139400/#review142738

::: tools/profiler/core/platform.cpp:2081
(Diff revision 1)
>  
> -    int interval = PROFILER_DEFAULT_INTERVAL;
> +    double interval = PROFILER_DEFAULT_INTERVAL;
>      const char* startupInterval = getenv("MOZ_PROFILER_STARTUP_INTERVAL");
>      if (startupInterval) {
>        errno = 0;
> -      interval = strtol(startupInterval, nullptr, 10);
> +      interval = strtod(startupInterval, nullptr);

Actually, just calling PR_strtod is probably the better way to go.
Comment on attachment 8867859 [details]
Bug 1348031: Part 1 - Allow specifying fractional-millisecond startup profiling interval.

https://reviewboard.mozilla.org/r/139400/#review142726

> Is this going to work on systems with a French / German OS locale where the decimal separator is a comma? I'd expect not.
> 
> Can you use Decimal::fromString(stdString).toDouble() instead? That seems to be the simplest way of using StringToDoubleConverter.

Yes, strtod is locale-aware.
Comment on attachment 8867859 [details]
Bug 1348031: Part 1 - Allow specifying fractional-millisecond startup profiling interval.

https://reviewboard.mozilla.org/r/139400/#review142766

Thanks!
Attachment #8867859 - Flags: review?(mstange) → review+
Pushed by maglione.k@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c24d57d10d71
Part 1 - Allow specifying fractional-millisecond startup profiling interval. r=mstange
https://hg.mozilla.org/integration/autoland/rev/422e974844b3
Part 2 - Accept fractional-millisecond profiling intervals in talos. r=mstange
https://hg.mozilla.org/mozilla-central/rev/c24d57d10d71
https://hg.mozilla.org/mozilla-central/rev/422e974844b3
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.