Closed
Bug 1142457
Opened 10 years ago
Closed 10 years ago
The stopwatch should count resources per thread on MacOS, too
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: Yoric, Assigned: Yoric)
References
Details
Attachments
(1 file, 2 obsolete files)
5.29 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Updated•10 years ago
|
Summary: The stopwatch should counter per-thread resources on MacOS, too → The stopwatch should count resources per thread on MacOS, too
Assignee | ||
Comment 1•10 years ago
|
||
André, as one of the resident MacOS X experts, can you review this bug?
Assignee: nobody → dteller
Attachment #8587358 -
Flags: review?(areinald)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee | ||
Comment 3•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8587867 -
Flags: review?(jdemooij)
Comment 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
(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.
Comment 6•10 years ago
|
||
(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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
Applied feedback.
Attachment #8587867 -
Attachment is obsolete: true
Attachment #8590244 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 10•10 years ago
|
||
Keywords: checkin-needed
Comment 11•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Comment 12•10 years ago
|
||
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.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Description
•