Closed Bug 1142457 Opened 9 years ago Closed 9 years ago

The stopwatch should count resources per thread on MacOS, too

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

Details

Attachments

(1 file, 2 obsolete files)

Bug 674779 introduces per-component CPU accounting. For the moment, on Linux and Windows, only the resources used by the current thread are accounted for. However, on other OSes, including Mac OS X, this is not the case.

On MacOS X, we should use `thread_info` to get that same information. See http://www.gnu.org/software/hurd/gnumach-doc/Thread-Information.html#Thread-Information for more details.
Summary: The stopwatch should counter per-thread resources on MacOS, too → The stopwatch should count resources per thread on MacOS, too
Attached patch Count CPU usage per thread (obsolete) — Splinter Review
André, as one of the resident MacOS X experts, can you review this bug?
Assignee: nobody → dteller
Attachment #8587358 - Flags: review?(areinald)
Attached patch Count CPU usage per thread, v2 (obsolete) — Splinter Review
Fixed wrong include.
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69b3c71a0b3d
Attachment #8587358 - Attachment is obsolete: true
Attachment #8587358 - Flags: review?(areinald)
Attachment #8587867 - Flags: review?(areinald)
Comment on attachment 8587867 [details] [diff] [review]
Count CPU usage per thread, v2

Review of attachment 8587867 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/vm/Interpreter.cpp
@@ +530,5 @@
>          MOZ_ASSERT(userTime);
>          MOZ_ASSERT(systemTime);
>  
> +#if defined(XP_MACOSX)
> +        // On MacOS X, to get we per-thread data, we need to

to get (we?) per-thread

@@ +544,5 @@
> +                        /* [inout] number of items */   &count);
> +
> +        // We do not need ability to communicate with the thread, so
> +        // let's release the port.
> +        mach_port_deallocate(mach_task_self(), port);

Maybe it would make sense to have port as a thread static variable, in order to avoid alloc/dealloc. Just a suggestion.
Attachment #8587867 - Flags: review?(areinald) → review+
(In reply to André Reinald from comment #4)
> @@ +544,5 @@
> > +                        /* [inout] number of items */   &count);
> > +
> > +        // We do not need ability to communicate with the thread, so
> > +        // let's release the port.
> > +        mach_port_deallocate(mach_task_self(), port);
> 
> Maybe it would make sense to have port as a thread static variable, in order
> to avoid alloc/dealloc. Just a suggestion.

The problem is that there is a finite number of ports, and caching `mach_task_self()` would occupy a port for the duration of the thread. Unless we find out that this alloc/dealloc is causing a slowdown, I'd rather keep it.
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #5)
> The problem is that there is a finite number of ports, and caching
> `mach_task_self()` would occupy a port for the duration of the thread.
> Unless we find out that this alloc/dealloc is causing a slowdown, I'd rather
> keep it.

Both ways it's neither a significant performance hit, nor a significant resource consumption, so r+ anyway.
Comment on attachment 8587867 [details] [diff] [review]
Count CPU usage per thread, v2

Review of attachment 8587867 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.

::: js/src/vm/Interpreter.cpp
@@ +529,1 @@
>      bool getTimes(uint64_t *userTime, uint64_t *systemTime) const {

Pre-existing nit: this should be uint64_t* userTime (and same for systemTime) now.

@@ +548,5 @@
> +        mach_port_deallocate(mach_task_self(), port);
> +
> +        MOZ_ASSERT(err == KERN_SUCCESS);
> +        if (err != KERN_SUCCESS)
> +            return false;

I'd prefer having either the assert or the |if|. If we're not 200% sure it can never fail we should just check for failures IMO.

@@ +551,5 @@
> +        if (err != KERN_SUCCESS)
> +            return false;
> +
> +        *userTime = info.user_time.microseconds
> +            + info.user_time.seconds * 1000000;

Nit: this fits on one line, same for systemTime below.
Attachment #8587867 - Flags: review?(jdemooij) → review+
Applied feedback.
Attachment #8587867 - Attachment is obsolete: true
Attachment #8590244 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/6ac33b20b2dd
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
A regression/improvement popped up on awfy. This is the only revision I can think of that can remotely be possible.
There is a nice improvement on kraken-dft. Also a small regression on the more noisy asmjsapps-bullet-throughput benchmark.

AWFY detected a regression/improvement on:
- slave: Mac OS X 10.10 32-bit (Mac Pro, shell)
- mode: Ion

Regression(s)/Improvement(s):
- asmjs-apps: bullet-throughput: 1.54% (regression)
- kraken: dft: -3.83% (improvement)

Recorded range:
- http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=50d48228fd8c&tochange=f3a27b508519

More details: http://arewefastyet.com/regressions/#/regression/198435

Giving the patch is that a possible to influence some benchmarks and how? Given the small change and low severity I don't think it is worthy to look deeper into it, though.
Yes, I suspected a tiny impact on benchmarks, as we use a direct kernel call for stopwatch monitoring instead of going the longer way. I'm a bit surprised by the regression, but it really looks tiny, so I'm not really worried.
You need to log in before you can comment on or make changes to this bug.