Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1959103004:
Implement CPU time for OS X and POSIX. (Closed)
DescriptionImplement CPU time for OS X and POSIX.
V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch
implements CPU time for OS X and POSIX to provide more accurate
accounting of CPU time used by each thread.
BUG=v8:4984
LOG=n
Committed: https://crrev.com/025f3d262bab2748362374f1b90ac723a9655ee4
Cr-Commit-Position: refs/heads/master@{#36188}
Patch Set 1 #
Total comments: 8
Patch Set 2 : #
Total comments: 12
Patch Set 3 : #Patch Set 4 : #
Total comments: 4
Patch Set 5 : Address fadi's comments. #Patch Set 6 : #
Total comments: 2
Patch Set 7 : #Patch Set 8 : #
Total comments: 4
Patch Set 9 : Use pthread_mach_thread_np #Patch Set 10 : #
Total comments: 1
Messages
Total messages: 31 (9 generated)
lpy@chromium.org changed reviewers: + fmeawad@chromium.org
PTAL.
https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (left): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#o... src/base/platform/time.cc:22: #include "src/base/win32-headers.h" This line should be untouched in this CL. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:46: USE(kr); You are loosing the check here that kr == KERN_SUCCESS https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:66: int64_t ClockNow(clockid_t clk_id) { If this method is only used for CLOCK_THREAD_CPU, then maybe it should be inlined. if we are using clock_gettime somewhere else, then they should all call this method instead. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.h File src/base/platform/time.h (left): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.h#ol... src/base/platform/time.h:14: #include "src/base/safe_math.h" Unrelated to this CL?
PTAL. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (left): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#o... src/base/platform/time.cc:22: #include "src/base/win32-headers.h" On 2016/05/09 21:50:32, fmeawad wrote: > This line should be untouched in this CL. Done. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:46: USE(kr); On 2016/05/09 21:50:32, fmeawad wrote: > You are loosing the check here that kr == KERN_SUCCESS Done. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.cc#n... src/base/platform/time.cc:66: int64_t ClockNow(clockid_t clk_id) { On 2016/05/09 21:50:32, fmeawad wrote: > If this method is only used for CLOCK_THREAD_CPU, then maybe it should be > inlined. > > if we are using clock_gettime somewhere else, then they should all call this > method instead. Done. https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.h File src/base/platform/time.h (left): https://codereview.chromium.org/1959103004/diff/1/src/base/platform/time.h#ol... src/base/platform/time.h:14: #include "src/base/safe_math.h" On 2016/05/09 21:50:32, fmeawad wrote: > Unrelated to this CL? Done.
It looks good in general, some more minor comments. https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.cc:55: int64_t ConvertTimespecToMicros(const struct timespec& ts) { I would inline this method as well since it is only called from ClockNow https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.cc:616: #if (defined(_POSIX_THREAD_CPUTIME) && (_POSIX_THREAD_CPUTIME >= 0)) || \ Add comment with the bug number that tracks Windows implementation. https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.h File src/base/platform/time.h (right): https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.h:392: // Please use Now() or GetForThread() to create a new object. This is for I cannot find a GetForThread method in the API?
a couple more :) https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... File test/unittests/base/platform/time-unittest.cc (right): https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... test/unittests/base/platform/time-unittest.cc:188: #if V8_OS_ANDROID || V8_OS_WIN Why are you disabling the test on Android? https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... test/unittests/base/platform/time-unittest.cc:194: if (ThreadTicks::IsSupported()) { How about testing IsSupported itself?
I updated the CL. PTAL. https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.cc:55: int64_t ConvertTimespecToMicros(const struct timespec& ts) { On 2016/05/09 23:22:33, fmeawad wrote: > I would inline this method as well since it is only called from ClockNow Done. https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.cc:616: #if (defined(_POSIX_THREAD_CPUTIME) && (_POSIX_THREAD_CPUTIME >= 0)) || \ On 2016/05/09 23:22:33, fmeawad wrote: > Add comment with the bug number that tracks Windows implementation. Done. https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.h File src/base/platform/time.h (right): https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.h:392: // Please use Now() or GetForThread() to create a new object. This is for On 2016/05/09 23:22:33, fmeawad wrote: > I cannot find a GetForThread method in the API? Done. https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... File test/unittests/base/platform/time-unittest.cc (right): https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... test/unittests/base/platform/time-unittest.cc:188: #if V8_OS_ANDROID || V8_OS_WIN On 2016/05/09 23:24:56, fmeawad wrote: > Why are you disabling the test on Android? This test is ported from Chrome, and Chrome disables it on Android. https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... test/unittests/base/platform/time-unittest.cc:194: if (ThreadTicks::IsSupported()) { On 2016/05/09 23:24:56, fmeawad wrote: > How about testing IsSupported itself? Is it necessary? ThreadTicks::IsSupported doesn't do any calculation, just returns true when some macros are defined and false otherwise.
https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/20001/src/base/platform/time.... src/base/platform/time.cc:55: int64_t ConvertTimespecToMicros(const struct timespec& ts) { On 2016/05/09 23:51:15, lpy wrote: > On 2016/05/09 23:22:33, fmeawad wrote: > > I would inline this method as well since it is only called from ClockNow > > Done. I meant move the code to where it is called. https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... File test/unittests/base/platform/time-unittest.cc (right): https://codereview.chromium.org/1959103004/diff/20001/test/unittests/base/pla... test/unittests/base/platform/time-unittest.cc:194: if (ThreadTicks::IsSupported()) { On 2016/05/09 23:51:15, lpy wrote: > On 2016/05/09 23:24:56, fmeawad wrote: > > How about testing IsSupported itself? > > Is it necessary? ThreadTicks::IsSupported doesn't do any calculation, just > returns true when some macros are defined and false otherwise. It is very necessary, but I cannot think of a good testing strategy. The problem with this test as it stands is that if IsSupported is buggy and always returns false we also loose coverage for ::Now() https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.... src/base/platform/time.cc:615: // TODO(lpy): See issue 5000 for windows implementation. Instead of issue #, but a url: http://crbug.com/v8/5000 I would change this comment to // TODO(lpy): For windows ThreadTicks implementation see http://crbug.com/v8/5000 (lucky bug number :) ) https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.h File src/base/platform/time.h (right): https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.... src/base/platform/time.h:392: // This is for internal use and testing. Ticks is in microseconds. nit: Ticks "are" ?
I addressed the comments, PTAL. https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.... src/base/platform/time.cc:615: // TODO(lpy): See issue 5000 for windows implementation. On 2016/05/10 18:12:48, fmeawad wrote: > Instead of issue #, but a url: http://crbug.com/v8/5000 > I would change this comment to > // TODO(lpy): For windows ThreadTicks implementation see > http://crbug.com/v8/5000 > > (lucky bug number :) ) Done. https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.h File src/base/platform/time.h (right): https://codereview.chromium.org/1959103004/diff/60001/src/base/platform/time.... src/base/platform/time.h:392: // This is for internal use and testing. Ticks is in microseconds. On 2016/05/10 18:12:48, fmeawad wrote: > nit: Ticks "are" ? Done.
LGTM make sure to use Ticks are in microseconds everywhere.
lpy@chromium.org changed reviewers: + bmeurer@chromium.org, jochen@chromium.org
Thanks. + bmeurer@ and jochen@ for review. PTAL.
Description was changed from ========== Implement CPU time for OS X and other Posix OSs. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and other Posix OSs to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ========== to ========== Implement CPU time for OS X and other Posix OSs. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ==========
Description was changed from ========== Implement CPU time for OS X and other Posix OSs. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ========== to ========== Implement CPU time for OS X and POSIX. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ==========
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
Drive-by comment. https://codereview.chromium.org/1959103004/diff/100001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/100001/src/base/platform/time... src/base/platform/time.cc:33: mach_port_t thread = mach_thread_self(); The thread port is getting leaked here.
Thanks for pointing it out. https://codereview.chromium.org/1959103004/diff/100001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/100001/src/base/platform/time... src/base/platform/time.cc:33: mach_port_t thread = mach_thread_self(); On 2016/05/10 18:58:07, Robert Sesek wrote: > The thread port is getting leaked here. Done.
https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time... src/base/platform/time.cc:53: kr = mach_port_deallocate(mach_task_self(), thread); If this is performance critical, you can also use the pthread_mach_thread_np(pthread_self()) to avoid two kernel traps: one into mach_thread_self() and one into mach_port_deallocate(). Libpthread caches the thread port in a TSD slot. https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time... src/base/platform/time.cc:54: DCHECK(kr = KERN_SUCCESS); should be ==
Thanks. https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time.cc File src/base/platform/time.cc (right): https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time... src/base/platform/time.cc:53: kr = mach_port_deallocate(mach_task_self(), thread); On 2016/05/10 21:24:58, Robert Sesek wrote: > If this is performance critical, you can also use the > pthread_mach_thread_np(pthread_self()) to avoid two kernel traps: one into > mach_thread_self() and one into mach_port_deallocate(). Libpthread caches the > thread port in a TSD slot. Done. https://codereview.chromium.org/1959103004/diff/140001/src/base/platform/time... src/base/platform/time.cc:54: DCHECK(kr = KERN_SUCCESS); On 2016/05/10 21:24:58, Robert Sesek wrote: > should be == Done.
lgtm
lgtm https://codereview.chromium.org/1959103004/diff/180001/src/base/platform/time.h File src/base/platform/time.h (right): https://codereview.chromium.org/1959103004/diff/180001/src/base/platform/time... src/base/platform/time.h:394: }; DISALLOW_COPY_AND_ASSIGN()
On 2016/05/11 08:58:07, jochen (soon OOO) wrote: > lgtm > > https://codereview.chromium.org/1959103004/diff/180001/src/base/platform/time.h > File src/base/platform/time.h (right): > > https://codereview.chromium.org/1959103004/diff/180001/src/base/platform/time... > src/base/platform/time.h:394: }; > DISALLOW_COPY_AND_ASSIGN() Please see the definition of ThreadTicks::Now, we need to allow assignment.
The CQ bit was checked by lpy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fmeawad@chromium.org Link to the patchset: https://codereview.chromium.org/1959103004/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1959103004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1959103004/180001
Message was sent while issue was closed.
Description was changed from ========== Implement CPU time for OS X and POSIX. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ========== to ========== Implement CPU time for OS X and POSIX. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ==========
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== Implement CPU time for OS X and POSIX. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n ========== to ========== Implement CPU time for OS X and POSIX. V8 tracing controller uses 2 clocks: wall clock and cpu clock. This patch implements CPU time for OS X and POSIX to provide more accurate accounting of CPU time used by each thread. BUG=v8:4984 LOG=n Committed: https://crrev.com/025f3d262bab2748362374f1b90ac723a9655ee4 Cr-Commit-Position: refs/heads/master@{#36188} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/025f3d262bab2748362374f1b90ac723a9655ee4 Cr-Commit-Position: refs/heads/master@{#36188}
Message was sent while issue was closed.
A revert of this CL (patchset #10 id:180001) has been created in https://codereview.chromium.org/1966173003/ by lpy@chromium.org. The reason for reverting is: Buildbot is failing on Mac release build..
Message was sent while issue was closed.
will reland the CL in https://codereview.chromium.org/1966183003/ |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
