Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
56973012:
Implement Time::ThreadNow() on Mac. (Closed)
|
Created:
7 years, 1 month ago by fmeawad Modified:
7 years, 1 month ago CC:
chromium-reviews, erikwright+watch_chromium.org, nduca, tonyg Base URL:
https://src.chromium.org/chrome/trunk/src/ Visibility:
Public. |
DescriptionImplement thread-specific CPU time on Mac OS X
This is different from https://code.google.com/p/chromium/codesearch#chromium/src/base/process/process_metrics_mac.cc&l=216 as it gets the per-thread CPU time while process metrics gets all-thread CPU usage.
BUG=280744
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=233450
Patch Set 1 #Patch Set 2 : #Patch Set 3 : Add checks for invlalid ThreadNow return times #Patch Set 4 : add checks to make sure that ThreadNow returns a non-zero value #
Total comments: 21
Patch Set 5 : Remove an extra include #Patch Set 6 : #Patch Set 7 : #
Total comments: 4
Patch Set 8 : exclude OS_IOS from the IsThreadNowSupported check #
Total comments: 1
Messages
Total messages: 20 (0 generated)
As part of the effort to enhance TraceViewer, we are adding CPU time to all platforms to be used by TRACE_EVENT. PTAL.
https://codereview.chromium.org/56973012/diff/130001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time.h#newcode552 base/time/time.h:552: defined(OS_MACOSX) OS_IOS implies OS_MACOSX https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:9: #include <mach/mach_init.h> Include mach/mach.h instead. It will also include mach_port.h. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:13: nit: remove this blank line https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:73: NOTREACHED(); NOTREACHED() or NOTIMPLEMENTED()? Can this be implemented? https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:76: mach_port_t thread; Why break up declaration from initialization? https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:80: thread = mach_thread_self(); This is leaked. Store this in a ScopedMachPort. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:84: kern_return_t kr; Same. No need to break up declaration from initialization. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:92: return 0; No logging or DCHECKing for failure? https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:97: thread_info_data.user_time.microseconds; nit: align this to underneath the (
Thank you for the quick response. I have address all of your issues except regarding OS_IOS and NOTREACHED, currently the implementation is only for OS_MACOS, and any call to ThreadNow should be guarded by IsThreadNowSupported check first. So IsThreadNowSupported should return only the supported platforms for ThreadNow (i.e. only OS_MACOS), and ThreadNow "OS_IOS" should be NOTREACHED and not NOTIMPLEMENTED. I would be happy to change both, if you think my logic is not correct. Again thanks for the quick review. PTAL. https://codereview.chromium.org/56973012/diff/130001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time.h#newcode552 base/time/time.h:552: defined(OS_MACOSX) On 2013/11/05 18:32:07, rsesek wrote: > OS_IOS implies OS_MACOSX But ThreadNow is not supported for IOS yet, what am I missing? https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:9: #include <mach/mach_init.h> On 2013/11/05 18:32:07, rsesek wrote: > Include mach/mach.h instead. It will also include mach_port.h. Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:13: On 2013/11/05 18:32:07, rsesek wrote: > nit: remove this blank line Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:73: NOTREACHED(); On 2013/11/05 18:32:07, rsesek wrote: > NOTREACHED() or NOTIMPLEMENTED()? Can this be implemented? It can be implemented, but it should not be reached at the moment, since each call of ThreadNow is guarded by a IsThreadNowSupported check. I would be happy to change it. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:76: mach_port_t thread; On 2013/11/05 18:32:07, rsesek wrote: > Why break up declaration from initialization? Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:80: thread = mach_thread_self(); On 2013/11/05 18:32:07, rsesek wrote: > This is leaked. Store this in a ScopedMachPort. Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:84: kern_return_t kr; On 2013/11/05 18:32:07, rsesek wrote: > Same. No need to break up declaration from initialization. Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:92: return 0; On 2013/11/05 18:32:07, rsesek wrote: > No logging or DCHECKing for failure? Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:97: thread_info_data.user_time.microseconds; On 2013/11/05 18:32:07, rsesek wrote: > nit: align this to underneath the ( Done.
https://codereview.chromium.org/56973012/diff/130001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time.h#newcode552 base/time/time.h:552: defined(OS_MACOSX) On 2013/11/05 19:34:25, fmeawad-cr wrote: > On 2013/11/05 18:32:07, rsesek wrote: > > OS_IOS implies OS_MACOSX > > But ThreadNow is not supported for IOS yet, what am I missing? When you compile with OS_IOS, it sets OS_MACOSX=1 as well. https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc#ne... base/time/time_mac.cc:79: DLOG(ERROR) << "Invalid thread"; "Failed to get mach_thread_self()" https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc#ne... base/time/time_mac.cc:84: thread, nit: Only indent 4 spaces from the previous line.
Sorry for the misunderstanding, issues addressed. PTAL. https://codereview.chromium.org/56973012/diff/130001/base/time/time.h File base/time/time.h (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time.h#newcode552 base/time/time.h:552: defined(OS_MACOSX) On 2013/11/05 19:49:01, rsesek wrote: > On 2013/11/05 19:34:25, fmeawad-cr wrote: > > On 2013/11/05 18:32:07, rsesek wrote: > > > OS_IOS implies OS_MACOSX > > > > But ThreadNow is not supported for IOS yet, what am I missing? > > When you compile with OS_IOS, it sets OS_MACOSX=1 as well. Done. https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/56973012/diff/130001/base/time/time_mac.cc#ne... base/time/time_mac.cc:73: NOTREACHED(); On 2013/11/05 19:34:25, fmeawad-cr wrote: > On 2013/11/05 18:32:07, rsesek wrote: > > NOTREACHED() or NOTIMPLEMENTED()? Can this be implemented? > > It can be implemented, but it should not be reached at the moment, since each > call of ThreadNow is guarded by a IsThreadNowSupported check. I would be happy > to change it. Done. https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc File base/time/time_mac.cc (right): https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc#ne... base/time/time_mac.cc:79: DLOG(ERROR) << "Invalid thread"; On 2013/11/05 19:49:02, rsesek wrote: > "Failed to get mach_thread_self()" Done. https://codereview.chromium.org/56973012/diff/300001/base/time/time_mac.cc#ne... base/time/time_mac.cc:84: thread, On 2013/11/05 19:49:02, rsesek wrote: > nit: Only indent 4 spaces from the previous line. Done.
LGTM. Please also update the CL description to be a little more complete. Something like: "Implement Time::ThreadNow() on Mac. BUG=XXXX"
This looks different from what https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce... does. I suppose this does something different so this is probably intentional, but the CL description could be a bit longer (like rsesek suggested), explaining what exactly this is trying to do and why the call it does is actually the right call. https://codereview.chromium.org/56973012/diff/370001/base/time/time_unittest.cc File base/time/time_unittest.cc (right): https://codereview.chromium.org/56973012/diff/370001/base/time/time_unittest.... base/time/time_unittest.cc:650: // Make sure that some thread time have elapsed. s/have/has/
On 2013/11/05 23:20:05, Nico wrote: > This looks different from what > https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce... > does. I suppose this does something different so this is probably intentional, > but the CL description could be a bit longer (like rsesek suggested), explaining > what exactly this is trying to do and why the call it does is actually the right > call. This is getting per-thread CPU usage, while that's getting all-thread CPU usage.
On 2013/11/05 23:39:05, rsesek wrote: > On 2013/11/05 23:20:05, Nico wrote: > > This looks different from what > > > https://code.google.com/p/chromium/codesearch#chromium/src/base/process/proce... > > does. I suppose this does something different so this is probably intentional, > > but the CL description could be a bit longer (like rsesek suggested), > explaining > > what exactly this is trying to do and why the call it does is actually the > right > > call. > > This is getting per-thread CPU usage, while that's getting all-thread CPU usage. Right, I'm just saying the CL description should explicitly mention that. Right now it's a bit terse.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/56973012/370001
Retried try job too often on chromium_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/56973012/370001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/56973012/370001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/56973012/370001
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/fmeawad@chromium.org/56973012/370001
Message was sent while issue was closed.
Change committed as 233450 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
