Allow specifying a sub-millisecond interval for startup profiling

RESOLVED FIXED in Firefox 55

Status

()

Core
Gecko Profiler
RESOLVED FIXED
8 months ago
6 months ago

People

(Reporter: mstange, Assigned: kmag)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

8 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 4

6 months ago
mozreview-review
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)
(Reporter)

Comment 5

6 months ago
mozreview-review
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+
(Reporter)

Comment 6

6 months ago
mozreview-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.
(Assignee)

Comment 7

6 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 10

6 months ago
mozreview-review
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+

Comment 11

6 months ago
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
Blocks: 1364477

Comment 12

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c24d57d10d71
https://hg.mozilla.org/mozilla-central/rev/422e974844b3
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.