Closed Bug 1348031 Opened 6 years ago Closed 6 years ago

Allow specifying a sub-millisecond interval for startup profiling

Categories

(Core :: Gecko Profiler, enhancement)

Product:
Core
Component:
Gecko Profiler
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: mstange, Assigned: kmag)

References

Details

Attachments

(2 files)

Bug 1348031: Part 1 - Allow specifying fractional-millisecond startup profiling interval.
59 bytes, text/x-review-board-request
mstange
: review+
Details
Bug 1348031: Part 2 - Accept fractional-millisecond profiling intervals in talos.
59 bytes, text/x-review-board-request
mstange
: review+
Details 
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
Blocks: 1364477
https://hg.mozilla.org/mozilla-central/rev/c24d57d10d71
https://hg.mozilla.org/mozilla-central/rev/422e974844b3
Status: NEW → RESOLVED
Closed: 6 years 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.