Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2695743003:
Added GetCpuTime to base/ to get total CPU time consumed by process for perf tests. (Closed)
DescriptionAdded GetCpuTime to base/ to get total CPU time consumed by process for perf tests.
BUG=webrtc:7095
Review-Url: https://codereview.webrtc.org/2695743003
Cr-Commit-Position: refs/heads/master@{#16665}
Committed: https://chromium.googlesource.com/external/webrtc/+/3ff474b72ba1b34027e8391b1014c2838af445d4
Patch Set 1 #Patch Set 2 : increased test tolerance and fixed typo #Patch Set 3 : Small improvements #Patch Set 4 : fix asan #Patch Set 5 : fixing address sanitizing #Patch Set 6 : fixing asan second attempt #
Total comments: 8
Patch Set 7 : Implemeted nisse@ comments #
Total comments: 8
Patch Set 8 : fixed compile error on win #Patch Set 9 : implemented Nisse@ comments part 2 #
Total comments: 13
Patch Set 10 : implemented Nisse comments part 3 #Patch Set 11 : Added GetThreadCpuTime(), implemented Nisse comments. #Patch Set 12 : typo fixed #Patch Set 13 : fixed CE on MAC #Patch Set 14 : fixed mach CE v2 #Patch Set 15 : fixed MAC CE v3 #Patch Set 16 : fixing MAC CE v4 #Patch Set 17 : Fixing MAC CE v5 #Patch Set 18 : Fixed tests so threads will do only specified amount of work #Patch Set 19 : Speed-up tests and cleanup #Patch Set 20 : refactor tests #
Total comments: 15
Patch Set 21 : implemented Sprang@ comments #
Total comments: 18
Patch Set 22 : implemented deadbeef@ comments. #
Total comments: 4
Patch Set 23 : deadbeef@ comments. #
Total comments: 2
Patch Set 24 : implemented Sprang@ comments #
Total comments: 1
Patch Set 25 : Reland of the CL #Patch Set 26 : fixing flaky tests #
Messages
Total messages: 106 (59 generated)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_arm64_rel/build...) win_clang_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/win_clang_dbg/builds/10931)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_rel/builds/21158)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/21960)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/22058) mac_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_asan/builds/21963)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_asan on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_asan/builds/22059) mac_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/mac_rel/builds/22787)
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
ilnik@webrtc.org changed reviewers: + deadbeef@webrtc.org, henrika@webrtc.org, mflodman@webrtc.org, sprang@webrtc.org
I have added cpu_time to base/ directory. Currently it's in the "rtc_base_approved" source set. Is it the correct place? Other possibilities are to put it to not approved source list or in system_wrappers/, however I heard that we are trying to move from system_wrappers/ to base/. Finally, if there are objections, it can be moved to test/ directory, as it's intended to be used in perf_tests only.
Description was changed from ========== Added GetCpuTime to base/ to get total CPU time consumed by process for perf tests. BUG=webrtc:7095 ========== to ========== Added GetCpuTime to base/ to get total CPU time consumed by process for perf tests. BUG=webrtc:7095 ==========
henrika@webrtc.org changed reviewers: - henrika@webrtc.org
henrika@webrtc.org changed reviewers: + nisse@webrtc.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
You may also want to have a look at std::chrono, maybe it provides some portable interface to cpu time. (I'm seen it mentioned a few times, but I'm not familiar with it). https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:32: static_cast<double>(ts.tv_nsec) / 1000000000.0; I'd prefer "* 1e-9", so I don't have to count the zeros. And 1e-6 below. (There are also integer constants, like rtc::kNumNanosecontsPerSecond). https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:49: SYSTEMTIME userSystemTime; After a quick look at the windows docs, I'd recommend accessing the FILETIME fields directly. (static_cast<uint64_t>(userTime.dwHighDateTime) << 32) + userTime.dwLowDateTime. or something like that. Converting to hours and seconds is a waste of cycles. And note that left shift of signed values is undefined behavior in C (not sure about C++), hence the use of an unsigned type. https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h File webrtc/base/cpu_time.h (right): https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:16: class CpuTime { Why a class? Could be just rtc::GetCpuTime. https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:20: static double GetCpuTime(); Any particular reason to use floating point, rather than int64_t with nanoseconds? If I get the numbers right, 2^63 ns is a few hundred years.
https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:32: static_cast<double>(ts.tv_nsec) / 1000000000.0; On 2017/02/15 12:18:16, nisse-webrtc wrote: > I'd prefer "* 1e-9", so I don't have to count the zeros. And 1e-6 below. (There > are also integer constants, like rtc::kNumNanosecontsPerSecond). Done. https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:49: SYSTEMTIME userSystemTime; On 2017/02/15 12:18:16, nisse-webrtc wrote: > After a quick look at the windows docs, I'd recommend accessing the FILETIME > fields directly. > > (static_cast<uint64_t>(userTime.dwHighDateTime) << 32) > + userTime.dwLowDateTime. > > or something like that. Converting to hours and seconds is a waste of cycles. > And note that left shift of signed values is undefined behavior in C (not sure > about C++), hence the use of an unsigned type. Done. https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h File webrtc/base/cpu_time.h (right): https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:16: class CpuTime { On 2017/02/15 12:18:17, nisse-webrtc wrote: > Why a class? Could be just rtc::GetCpuTime. No particular reason. I've seen it somewhere and thought it is a preferred style here. At the second look, it is rather an exception than a standard. Will do it as a function. https://codereview.webrtc.org/2695743003/diff/100001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:20: static double GetCpuTime(); On 2017/02/15 12:18:17, nisse-webrtc wrote: > Any particular reason to use floating point, rather than int64_t with > nanoseconds? If I get the numbers right, 2^63 ns is a few hundred years. For portability reasons. Initially I thought of using clock()/CLOCKS_PER_SECOND for POSIX platforms, but it may not align perfectly with nanoseconds. I believe float will be more logical as a common denominator. But int64_t will do as well.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:31: return ts.tv_sec * kNumNanosecsPerSec + ts.tv_nsec; I think you need a cast to int64_t on one of the factors to make sure you get a 64-bit rather than 32-bit multiply. And similarly for the rusage case below. https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:33: LOG(LS_ERROR) << "clock_gettime() failed."; I think you should use LOG_ERR when errno (or the corresponding thing on windows) contains a relevant error code. I think you should use it for all three cases, clock_gettime, getrusage, and GetProcessTimes. https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:49: &userTime) != -1) { The docs doesn't specify which non-zero value is returned on error. So use == 0. And perhaps change to == 0 for the the other cases too, for consistency. See https://msdn.microsoft.com/en-us/library/windows/desktop/ms683223(v=vs.85).aspx
https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:31: return ts.tv_sec * kNumNanosecsPerSec + ts.tv_nsec; On 2017/02/15 13:07:31, nisse-webrtc wrote: > I think you need a cast to int64_t on one of the factors to make sure you get a > 64-bit rather than 32-bit multiply. And similarly for the rusage case below. kNumNanosecsPerSec and other constants are already int64_t. For simplicity reasons I prefer no to add any additional casts. https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:33: LOG(LS_ERROR) << "clock_gettime() failed."; On 2017/02/15 13:07:31, nisse-webrtc wrote: > I think you should use LOG_ERR when errno (or the corresponding thing on > windows) contains a relevant error code. I think you should use it for all three > cases, clock_gettime, getrusage, and GetProcessTimes. Done. https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:49: &userTime) != -1) { On 2017/02/15 13:07:31, nisse-webrtc wrote: > The docs doesn't specify which non-zero value is returned on error. So use == 0. > And perhaps change to == 0 for the the other cases too, for consistency. > > See > https://msdn.microsoft.com/en-us/library/windows/desktop/ms683223(v=vs.85).aspx This was actually a mistake, because it returns non-zero value ON SUCCESS.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Some more comments. Sorry I didn't look closer at the tests in earlier iterations. (And I'm not OWNER of any of this, so you'll need some of the other reviewers to have a say too). https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:31: return ts.tv_sec * kNumNanosecsPerSec + ts.tv_nsec; On 2017/02/15 13:23:11, ilnik wrote: > On 2017/02/15 13:07:31, nisse-webrtc wrote: > > I think you need a cast to int64_t on one of the factors to make sure you get > a > > 64-bit rather than 32-bit multiply. And similarly for the rusage case below. > > kNumNanosecsPerSec and other constants are already int64_t. For simplicity > reasons I prefer no to add any additional casts. You're right, that's sufficient. https://codereview.webrtc.org/2695743003/diff/120001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:49: &userTime) != -1) { On 2017/02/15 13:23:11, ilnik wrote: > On 2017/02/15 13:07:31, nisse-webrtc wrote: > > The docs doesn't specify which non-zero value is returned on error. So use == > 0. > > And perhaps change to == 0 for the the other cases too, for consistency. > > > > See > > > https://msdn.microsoft.com/en-us/library/windows/desktop/ms683223(v=vs.85).aspx > > This was actually a mistake, because it returns non-zero value ON SUCCESS. Ooops. Good you found it. https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:16: namespace internal { Just use the unnamed namespace. My understanding is that internal (or rather, webrtc::internal) is mainly internal things which for some reason has to be put in otherwise public header file. https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); Is there anyway you can rearrange this test to not have a tolerance threshold? If you spin based on wallclock time (as you do above), then you should get the strict bound cputime <= wallclock time, with a possibly large difference if competing for cpu with other threads. (Maybe add a little margin, in case the two clocks for some reason aren't based on exactly the same frequency). A strict bound in the other direction is harder, but maybe it's sufficient for this test to check that the measured cputime is > 0? https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { This makes me wonder what the cputime is going to be used for. For the usecases I imagine, I'd expect a thread-specific cpu timer is more relevant. I.e., CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on windows, or if RUSAGE_THREAD is supported on Mac OS).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:16: namespace internal { On 2017/02/15 14:27:35, nisse-webrtc wrote: > Just use the unnamed namespace. My understanding is that internal (or rather, > webrtc::internal) is mainly internal things which for some reason has to be put > in otherwise public header file. Done. https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); On 2017/02/15 14:27:35, nisse-webrtc wrote: > Is there anyway you can rearrange this test to not have a tolerance threshold? > > If you spin based on wallclock time (as you do above), then you should get the > strict bound cputime <= wallclock time, with a possibly large difference if > competing for cpu with other threads. (Maybe add a little margin, in case the > two clocks for some reason aren't based on exactly the same frequency). > > A strict bound in the other direction is harder, but maybe it's sufficient for > this test to check that the measured cputime is > 0? Unfortunately, these CPU counters are not very precise. On my machine I get consistently 0.001-0.002s error. I could decrease the tolerance a bit. https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { On 2017/02/15 14:27:35, nisse-webrtc wrote: > This makes me wonder what the cputime is going to be used for. For the usecases > I imagine, I'd expect a thread-specific cpu timer is more relevant. I.e., > CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on windows, or > if RUSAGE_THREAD is supported on Mac OS). I am going to measure total CPU time consumed by test to infer total cpu usage.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); On 2017/02/15 14:40:45, ilnik wrote: > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > Is there anyway you can rearrange this test to not have a tolerance threshold? > > > > If you spin based on wallclock time (as you do above), then you should get the > > strict bound cputime <= wallclock time, with a possibly large difference if > > competing for cpu with other threads. (Maybe add a little margin, in case the > > two clocks for some reason aren't based on exactly the same frequency). > > > > A strict bound in the other direction is harder, but maybe it's sufficient for > > this test to check that the measured cputime is > 0? > > Unfortunately, these CPU counters are not very precise. On my machine I get > consistently 0.001-0.002s error. I could decrease the tolerance a bit. I'm more concerned that tests may be flaky, i.e., randomly fail if the machine running them happen to have high load. So if you need a threshold, please make include a pretty large margin. https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { On 2017/02/15 14:40:45, ilnik wrote: > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > This makes me wonder what the cputime is going to be used for. For the > usecases > > I imagine, I'd expect a thread-specific cpu timer is more relevant. I.e., > > CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on windows, or > > if RUSAGE_THREAD is supported on Mac OS). > > I am going to measure total CPU time consumed by test to infer total cpu usage. I see. Then I have two suggestions: 1. Rename it to include "Process" somewhere in the name. Otherwise, I think including other threads will be unexpected to anyone looking to using this for benchmarking. 2. If it's intended for tests only, move it to a test target in the BUILD.gn file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_tsan2 on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_tsan2/builds/19811)
https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); On 2017/02/15 14:48:33, nisse-webrtc wrote: > On 2017/02/15 14:40:45, ilnik wrote: > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > Is there anyway you can rearrange this test to not have a tolerance > threshold? > > > > > > If you spin based on wallclock time (as you do above), then you should get > the > > > strict bound cputime <= wallclock time, with a possibly large difference if > > > competing for cpu with other threads. (Maybe add a little margin, in case > the > > > two clocks for some reason aren't based on exactly the same frequency). > > > > > > A strict bound in the other direction is harder, but maybe it's sufficient > for > > > this test to check that the measured cputime is > 0? > > > > Unfortunately, these CPU counters are not very precise. On my machine I get > > consistently 0.001-0.002s error. I could decrease the tolerance a bit. > > I'm more concerned that tests may be flaky, i.e., randomly fail if the machine > running them happen to have high load. So if you need a threshold, please make > include a pretty large margin. That's the great thing about CPU time - it should not depend on load of the system as CPU time of only current process is measured. There are some issues with precision, but it doesn't change over time (on my linux workstation it's always 0.001-0.002s less than expected). https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { On 2017/02/15 14:48:33, nisse-webrtc wrote: > On 2017/02/15 14:40:45, ilnik wrote: > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > This makes me wonder what the cputime is going to be used for. For the > > usecases > > > I imagine, I'd expect a thread-specific cpu timer is more relevant. I.e., > > > CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on windows, > or > > > if RUSAGE_THREAD is supported on Mac OS). > > > > I am going to measure total CPU time consumed by test to infer total cpu > usage. > > I see. Then I have two suggestions: > > 1. Rename it to include "Process" somewhere in the name. Otherwise, I think > including other threads will be unexpected to anyone looking to using this for > benchmarking. > > 2. If it's intended for tests only, move it to a test target in the BUILD.gn > file. 1. is GetProcessCpuTime() ok? 2. I could just include it to the source set named "rtc_base_tests_utils". But none of the stuff there has any unittests. Should I purge my unittest. It seams to pass on every trybot and there is no changes for that function intended in the future.
https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); On 2017/02/15 15:07:10, ilnik wrote: > On 2017/02/15 14:48:33, nisse-webrtc wrote: > > On 2017/02/15 14:40:45, ilnik wrote: > > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > > Is there anyway you can rearrange this test to not have a tolerance > > threshold? > > > > > > > > If you spin based on wallclock time (as you do above), then you should get > > the > > > > strict bound cputime <= wallclock time, with a possibly large difference > if > > > > competing for cpu with other threads. (Maybe add a little margin, in case > > the > > > > two clocks for some reason aren't based on exactly the same frequency). > > > > > > > > A strict bound in the other direction is harder, but maybe it's sufficient > > for > > > > this test to check that the measured cputime is > 0? > > > > > > Unfortunately, these CPU counters are not very precise. On my machine I get > > > consistently 0.001-0.002s error. I could decrease the tolerance a bit. > > > > I'm more concerned that tests may be flaky, i.e., randomly fail if the machine > > running them happen to have high load. So if you need a threshold, please make > > include a pretty large margin. > > That's the great thing about CPU time - it should not depend on load of the > system as CPU time of only current process is measured. There are some issues > with precision, but it doesn't change over time (on my linux workstation it's > always 0.001-0.002s less than expected). Also, I just realized, that's the problem with the tests: they are affected by system load. These 0.001 lost seconds are other processes stealing work from my waiting function. Unfortunately, it's impossible to test CPU time without writing function which takes exactly fixed time to compute using wall timers. I think unittests here are even more complicated than the function itself and are not needed.
henrika@webrtc.org changed reviewers: + kjellander@webrtc.org
+kjellander since he has lots of experience of tests running on different bot configurations. To me it sounds like there is a large risk of creating flaky tests here especially in combination with swarming, ASAN, MSAN etc.
On 2017/02/15 15:26:26, henrika_webrtc wrote: > +kjellander since he has lots of experience of tests running on different bot > configurations. > > To me it sounds like there is a large risk of creating flaky tests here > especially in combination > with swarming, ASAN, MSAN etc. This will only be used as an additional metric in perf tests. Furthermore it will not be any more flaky than encoding_time or any similar timing metrics. The only issue here may be that drastic change to the testing code, like adding new metrics, will affect the CPU time. Anyways, if the metric will be too flaky we can always disable it later.
https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - ::internal::kAllowedErrorSec); On 2017/02/15 15:19:16, ilnik wrote: > On 2017/02/15 15:07:10, ilnik wrote: > > On 2017/02/15 14:48:33, nisse-webrtc wrote: > > > On 2017/02/15 14:40:45, ilnik wrote: > > > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > > > Is there anyway you can rearrange this test to not have a tolerance > > > threshold? > > > > > > > > > > If you spin based on wallclock time (as you do above), then you should > get > > > the > > > > > strict bound cputime <= wallclock time, with a possibly large difference > > if > > > > > competing for cpu with other threads. (Maybe add a little margin, in > case > > > the > > > > > two clocks for some reason aren't based on exactly the same frequency). > > > > > > > > > > A strict bound in the other direction is harder, but maybe it's > sufficient > > > for > > > > > this test to check that the measured cputime is > 0? > > > > > > > > Unfortunately, these CPU counters are not very precise. On my machine I > get > > > > consistently 0.001-0.002s error. I could decrease the tolerance a bit. > > > > > > I'm more concerned that tests may be flaky, i.e., randomly fail if the > machine > > > running them happen to have high load. So if you need a threshold, please > make > > > include a pretty large margin. > > > > That's the great thing about CPU time - it should not depend on load of the > > system as CPU time of only current process is measured. There are some issues > > with precision, but it doesn't change over time (on my linux workstation it's > > always 0.001-0.002s less than expected). > > Also, I just realized, that's the problem with the tests: they are affected by > system load. These 0.001 lost seconds are other processes stealing work from my > waiting function. Unfortunately, it's impossible to test CPU time without > writing function which takes exactly fixed time to compute using wall timers. > > I think unittests here are even more complicated than the function itself and > are not needed. I think you could do a meaningful test like this: const int64_t kTestTimeNs = 300 * rtc::kNumMillisecondsPerSecond; int64_t wall_clock_start = rtc::TimeNanos(); int64_t cpu_clock_start = rtc::GetProcessCpuTime(); while (rtc::TimeNanos() < wall_clock_start + kTestTimeNs) ; int64_t cpu_clock_duration = rtc::GetPRocesscpuTime() - cpu_clock_start; int64_t wall_clock_duration = rtc::TimeNanos() - wall_clock_start; EXPECT_GT(cpu_clock_duration, 0); EXPECT_LE(cpu_clock_duration, wall_clock_duration); https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { On 2017/02/15 15:07:10, ilnik wrote: > On 2017/02/15 14:48:33, nisse-webrtc wrote: > > On 2017/02/15 14:40:45, ilnik wrote: > > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > > This makes me wonder what the cputime is going to be used for. For the > > > usecases > > > > I imagine, I'd expect a thread-specific cpu timer is more relevant. I.e., > > > > CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on > windows, > > or > > > > if RUSAGE_THREAD is supported on Mac OS). > > > > > > I am going to measure total CPU time consumed by test to infer total cpu > > usage. > > > > I see. Then I have two suggestions: > > > > 1. Rename it to include "Process" somewhere in the name. Otherwise, I think > > including other threads will be unexpected to anyone looking to using this for > > benchmarking. > > > > 2. If it's intended for tests only, move it to a test target in the BUILD.gn > > file. > > 1. is GetProcessCpuTime() ok? I think so. > 2. I could just include it to the source set named "rtc_base_tests_utils". > But none of the stuff there has any unittests. Should I purge my unittest. It > seams to pass on every trybot and there is no changes for that function intended > in the future. I don't think unit tests are an obstacle to putting things in that target.
On 2017/02/15 15:34:35, nisse-webrtc wrote: > https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... > File webrtc/base/cpu_time_unittest.cc (right): > > https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... > webrtc/base/cpu_time_unittest.cc:42: ::internal::kProcessingTimeSec - > ::internal::kAllowedErrorSec); > On 2017/02/15 15:19:16, ilnik wrote: > > On 2017/02/15 15:07:10, ilnik wrote: > > > On 2017/02/15 14:48:33, nisse-webrtc wrote: > > > > On 2017/02/15 14:40:45, ilnik wrote: > > > > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > > > > Is there anyway you can rearrange this test to not have a tolerance > > > > threshold? > > > > > > > > > > > > If you spin based on wallclock time (as you do above), then you should > > get > > > > the > > > > > > strict bound cputime <= wallclock time, with a possibly large > difference > > > if > > > > > > competing for cpu with other threads. (Maybe add a little margin, in > > case > > > > the > > > > > > two clocks for some reason aren't based on exactly the same > frequency). > > > > > > > > > > > > A strict bound in the other direction is harder, but maybe it's > > sufficient > > > > for > > > > > > this test to check that the measured cputime is > 0? > > > > > > > > > > Unfortunately, these CPU counters are not very precise. On my machine I > > get > > > > > consistently 0.001-0.002s error. I could decrease the tolerance a bit. > > > > > > > > I'm more concerned that tests may be flaky, i.e., randomly fail if the > > machine > > > > running them happen to have high load. So if you need a threshold, please > > make > > > > include a pretty large margin. > > > > > > That's the great thing about CPU time - it should not depend on load of the > > > system as CPU time of only current process is measured. There are some > issues > > > with precision, but it doesn't change over time (on my linux workstation > it's > > > always 0.001-0.002s less than expected). > > > > Also, I just realized, that's the problem with the tests: they are affected by > > system load. These 0.001 lost seconds are other processes stealing work from > my > > waiting function. Unfortunately, it's impossible to test CPU time without > > writing function which takes exactly fixed time to compute using wall timers. > > > > I think unittests here are even more complicated than the function itself and > > are not needed. > > I think you could do a meaningful test like this: > > const int64_t kTestTimeNs = 300 * rtc::kNumMillisecondsPerSecond; > > int64_t wall_clock_start = rtc::TimeNanos(); > int64_t cpu_clock_start = rtc::GetProcessCpuTime(); > > while (rtc::TimeNanos() < wall_clock_start + kTestTimeNs) > ; > > int64_t cpu_clock_duration = rtc::GetPRocesscpuTime() - cpu_clock_start; > int64_t wall_clock_duration = rtc::TimeNanos() - wall_clock_start; > > EXPECT_GT(cpu_clock_duration, 0); > EXPECT_LE(cpu_clock_duration, wall_clock_duration); > No, the same issue here. For 300 seconds the test is running, the process may be using only 50% of CPU due to high load. And cpu_clock_duration will be around 150 seconds. Also, if using 2 threads, if them both are restricted to the single core, cpu time will be about equal to wall time. And double time will be only achieved if both threads are ran on different cores. > https://codereview.webrtc.org/2695743003/diff/160001/webrtc/base/cpu_time_uni... > webrtc/base/cpu_time_unittest.cc:47: TEST(GetCpuTimeTest, TwoThreads) { > On 2017/02/15 15:07:10, ilnik wrote: > > On 2017/02/15 14:48:33, nisse-webrtc wrote: > > > On 2017/02/15 14:40:45, ilnik wrote: > > > > On 2017/02/15 14:27:35, nisse-webrtc wrote: > > > > > This makes me wonder what the cputime is going to be used for. For the > > > > usecases > > > > > I imagine, I'd expect a thread-specific cpu timer is more relevant. > I.e., > > > > > CLOCK_THREAD_CPUTIME_ID on linux (no idea how these things work on > > windows, > > > or > > > > > if RUSAGE_THREAD is supported on Mac OS). > > > > > > > > I am going to measure total CPU time consumed by test to infer total cpu > > > usage. > > > > > > I see. Then I have two suggestions: > > > > > > 1. Rename it to include "Process" somewhere in the name. Otherwise, I think > > > including other threads will be unexpected to anyone looking to using this > for > > > benchmarking. > > > > > > 2. If it's intended for tests only, move it to a test target in the BUILD.gn > > > file. > > > > 1. is GetProcessCpuTime() ok? > > I think so. > > > 2. I could just include it to the source set named "rtc_base_tests_utils". > > But none of the stuff there has any unittests. Should I purge my unittest. It > > seams to pass on every trybot and there is no changes for that function > intended > > in the future. > > I don't think unit tests are an obstacle to putting things in that target. Yes, after some more investigation I see that unittest is correctly built. Still, my unittest is incorrect and I want to remove it, because it will fail under high load. But then GetProcessCpuTime will be uncovered by any tests.
On 2017/02/15 15:43:24, ilnik wrote: > On 2017/02/15 15:34:35, nisse-webrtc wrote: > > I think you could do a meaningful test like this: > > > > const int64_t kTestTimeNs = 300 * rtc::kNumMillisecondsPerSecond; > > > > int64_t wall_clock_start = rtc::TimeNanos(); > > int64_t cpu_clock_start = rtc::GetProcessCpuTime(); > > > > while (rtc::TimeNanos() < wall_clock_start + kTestTimeNs) > > ; > > > > int64_t cpu_clock_duration = rtc::GetPRocesscpuTime() - cpu_clock_start; > > int64_t wall_clock_duration = rtc::TimeNanos() - wall_clock_start; > > > > EXPECT_GT(cpu_clock_duration, 0); > > EXPECT_LE(cpu_clock_duration, wall_clock_duration); > > > > No, the same issue here. For 300 seconds the test is running, the process may be > using only 50% of CPU due to high load. And cpu_clock_duration will be around > 150 seconds. Ooops, intention was to run for 300 ms. Anyway, the test will pass in this case, since it checks that 0 < 150s <= 300s, which is true. I think that's robust, but it won't catch bugs which make the cpu time measurement a much too small. (And the test process clearly mustn't run any other threads in parallel)
On 2017/02/15 15:54:51, nisse-webrtc wrote: > On 2017/02/15 15:43:24, ilnik wrote: > > On 2017/02/15 15:34:35, nisse-webrtc wrote: > > > I think you could do a meaningful test like this: > > > > > > const int64_t kTestTimeNs = 300 * rtc::kNumMillisecondsPerSecond; > > > > > > int64_t wall_clock_start = rtc::TimeNanos(); > > > int64_t cpu_clock_start = rtc::GetProcessCpuTime(); > > > > > > while (rtc::TimeNanos() < wall_clock_start + kTestTimeNs) > > > ; > > > > > > int64_t cpu_clock_duration = rtc::GetPRocesscpuTime() - cpu_clock_start; > > > int64_t wall_clock_duration = rtc::TimeNanos() - wall_clock_start; > > > > > > EXPECT_GT(cpu_clock_duration, 0); > > > EXPECT_LE(cpu_clock_duration, wall_clock_duration); > > > > > > > No, the same issue here. For 300 seconds the test is running, the process may > be > > using only 50% of CPU due to high load. And cpu_clock_duration will be around > > 150 seconds. > > Ooops, intention was to run for 300 ms. Anyway, the test will pass in this case, > since > it checks that 0 < 150s <= 300s, which is true. I think that's robust, but it > won't catch bugs which make the cpu time measurement a much too small. (And the > test process clearly mustn't run any other threads in parallel) Yes, but if somehow, on some platform, instead of Cpu Time we will measure Wall Clock Time, that test will not catch it. Also, current test is just like that you proposed, but is running for 1 second and checks are a bit different.
I don't know enough about how hard this is to implement properly but it seems like a useful metric to have for perf, I agree to that. Another option for us could be to add a Python wrapper script instead for our perf tests (they're already wrapped in Chromium's https://cs.chromium.org/chromium/build/scripts/slave/runtest.py). Then we could implement tracking of the process' CPU time using Python instead. It seems fairly simple to do without third party libraries (like psutil): http://stackoverflow.com/questions/26475636/measure-elapsed-time-amount-of-me...
On 2017/02/16 07:57:14, kjellander_webrtc wrote: > I don't know enough about how hard this is to implement properly but it seems > like a useful metric to have for perf, I agree to that. > > Another option for us could be to add a Python wrapper script instead for our > perf tests (they're already wrapped in Chromium's > https://cs.chromium.org/chromium/build/scripts/slave/runtest.py). Then we could > implement tracking of the process' CPU time using Python instead. It seems > fairly simple to do without third party libraries (like psutil): > http://stackoverflow.com/questions/26475636/measure-elapsed-time-amount-of-me... Worth pointing out is that the Chrome infra team want to get rid of that old runtest.py, so they'd be happy if we moved off it. We haven't spent time on doing that yet since it contains the perf log scraping code which is needed for uploading the perf numbers to the perf dashboard (something we'd like to avoid having to maintain ourselves).
On 2017/02/16 07:57:14, kjellander_webrtc wrote: > I don't know enough about how hard this is to implement properly but it seems > like a useful metric to have for perf, I agree to that. > > Another option for us could be to add a Python wrapper script instead for our > perf tests (they're already wrapped in Chromium's > https://cs.chromium.org/chromium/build/scripts/slave/runtest.py). Then we could > implement tracking of the process' CPU time using Python instead. It seems > fairly simple to do without third party libraries (like psutil): > http://stackoverflow.com/questions/26475636/measure-elapsed-time-amount-of-me... If the objective is to measure the total cpu time used over a process' lifetime, I also think it makes more sense to use a separate wrapper program to do that. More or less like the unix time tool, which is a command-line frontend to rusage (but if there's some system-independent way to do the same thing in python, that sounds nice). clock_gettime is more useful for micro benchmarks, e.g., if you want to time 10000 iterations of some performance critical function.
On 2017/02/16 08:14:44, nisse-webrtc wrote: > On 2017/02/16 07:57:14, kjellander_webrtc wrote: > > I don't know enough about how hard this is to implement properly but it seems > > like a useful metric to have for perf, I agree to that. > > > > Another option for us could be to add a Python wrapper script instead for our > > perf tests (they're already wrapped in Chromium's > > https://cs.chromium.org/chromium/build/scripts/slave/runtest.py). Then we > could > > implement tracking of the process' CPU time using Python instead. It seems > > fairly simple to do without third party libraries (like psutil): > > > http://stackoverflow.com/questions/26475636/measure-elapsed-time-amount-of-me... > > If the objective is to measure the total cpu time used over a process' lifetime, > I also think it makes more sense to use a separate wrapper program to do that. > More or less like the unix time tool, which is a command-line frontend to rusage > (but if there's some system-independent way to do the same thing in python, that > sounds nice). > > clock_gettime is more useful for micro benchmarks, e.g., if you want to time > 10000 iterations of some performance critical function. That way we can measure each testcase independently. Measuring whole process lifetime is useless if we know that some test now takes 20% more compute time, but have no idea, which one is it. Also, we can potentially exclude heavy test-related computations. I am thinking of adding a thread-only CPU time also. Then I could, for example, subtract time consumed by PSNR and SSIM computations.
On 2017/02/16 08:43:10, ilnik wrote: > That way we can measure each testcase independently. Measuring whole process > lifetime is useless if we know that some test now takes 20% more compute time, > but have no idea, which one is it. I agree clock_gettime is the right tool for this use case. It's always a bit difficult to review when not knowing the broader context of the changes.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by ilnik@webrtc.org
On 2017/02/16 08:46:39, nisse-webrtc wrote: > On 2017/02/16 08:43:10, ilnik wrote: > > That way we can measure each testcase independently. Measuring whole process > > lifetime is useless if we know that some test now takes 20% more compute time, > > but have no idea, which one is it. > > I agree clock_gettime is the right tool for this use case. It's always a bit > difficult to review when not knowing the broader context of the changes. I've added GetThreadCpuTime. Still testing it. There were some problems with MAC, hope the method I found will work. The broad context is like that: we need a precise method to measure our CPU load in perf tests. GetProcessCpuTime() and GetThreadCpuTime() are the instruments for that.
On 2017/02/16 08:46:39, nisse-webrtc wrote: > On 2017/02/16 08:43:10, ilnik wrote: > > That way we can measure each testcase independently. Measuring whole process > > lifetime is useless if we know that some test now takes 20% more compute time, > > but have no idea, which one is it. > > I agree clock_gettime is the right tool for this use case. It's always a bit > difficult to review when not knowing the broader context of the changes. I also agree, those are all good points. Measuring with finer granularity will certainly allow us to detect more regressions. However, the fact that I'm unable to find anything like this in the Chromium codebase makes me think that it's possible to accomplish the same results with other techniques. Especially as Chrome is very speed focused and have a whole team working on only measuring performance. I suggest you read up on tracing [1] and/or reach out to the speed team [2] to hear their view on implementing something like this. They have a ton of experience in this field. We have a lot of trace calls in the code already but AFAIK nobody has looked at how to use that information from a purely native perspective (it's only used when WebRTC is a part of Chrome). You could probably check with somebody like tommi@ or magjed@ in our team if they know more about this. [1]: http://www.chromium.org/developers/how-tos/trace-event-profiling-tool [2]: https://www.chromium.org/developers/speed-infra
On 2017/02/16 09:31:14, kjellander_webrtc wrote: > On 2017/02/16 08:46:39, nisse-webrtc wrote: > > On 2017/02/16 08:43:10, ilnik wrote: > > > That way we can measure each testcase independently. Measuring whole process > > > lifetime is useless if we know that some test now takes 20% more compute > time, > > > but have no idea, which one is it. > > > > I agree clock_gettime is the right tool for this use case. It's always a bit > > difficult to review when not knowing the broader context of the changes. > > I also agree, those are all good points. Measuring with finer granularity will > certainly allow us to detect more regressions. > However, the fact that I'm unable to find anything like this in the Chromium > codebase makes me think that it's possible to accomplish the same results with > other techniques. Especially as Chrome is very speed focused and have a whole > team working on only measuring performance. > I suggest you read up on tracing [1] and/or reach out to the speed team [2] to > hear their view on implementing something like this. They have a ton of > experience in this field. We have a lot of trace calls in the code already but > AFAIK nobody has looked at how to use that information from a purely native > perspective (it's only used when WebRTC is a part of Chrome). > You could probably check with somebody like tommi@ or magjed@ in our team if > they know more about this. > > [1]: http://www.chromium.org/developers/how-tos/trace-event-profiling-tool > [2]: https://www.chromium.org/developers/speed-infra Tracing will give us time information. But what I am trying to do is to get CPU utilization metric. I was specifically asked to do that. This is a different story, although they are correlated. Also, there are similar things in chrome, see: https://cs.chromium.org/chromium/src/v8/src/base/platform/time.cc?q=CLOCK_THR... But you are right, looks like it's not used anywhere except in some tests.
https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:23: #endif At this point this file isn't overly large, but I still think it could be wort splitting this up into platform dependent .cc files and have gn pick the correct one to build. The fewer #ifdef's the better, imo. wdyt? https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:44: LOG_ERR(LS_ERROR) << "clock_gettime() failed."; I've not seen the LOG_ERR macro before. Is it to specifically log to stderr? Why not plain LOG(LS_ERROR) here and below? https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:68: LOG(LS_ERROR) << "No function to get CPU time"; Is there any non-supported platform? Maybe we should just do an RTC_NOT_REACHED(); instead? https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:20: // Consumes approximately kProcessingTimeMillisecs of CPU time. nit: formatting https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:22: int64_t& counter = *reinterpret_cast<int64_t*>(counter_pointer); Avoid non-const refs, just keep it after casting to int64_t* https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:37: int64_t* counter = new int64_t; No need to heap-allocate this. https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:46: kProcessingTimeMillisecs + kAllowedErrorMillisecs); EXPECT_NEAR ?
On 2017/02/16 11:39:12, ilnik wrote: > On 2017/02/16 09:31:14, kjellander_webrtc wrote: > > On 2017/02/16 08:46:39, nisse-webrtc wrote: > > > On 2017/02/16 08:43:10, ilnik wrote: > > > > That way we can measure each testcase independently. Measuring whole > process > > > > lifetime is useless if we know that some test now takes 20% more compute > > time, > > > > but have no idea, which one is it. > > > > > > I agree clock_gettime is the right tool for this use case. It's always a bit > > > difficult to review when not knowing the broader context of the changes. > > > > I also agree, those are all good points. Measuring with finer granularity will > > certainly allow us to detect more regressions. > > However, the fact that I'm unable to find anything like this in the Chromium > > codebase makes me think that it's possible to accomplish the same results with > > other techniques. Especially as Chrome is very speed focused and have a whole > > team working on only measuring performance. > > I suggest you read up on tracing [1] and/or reach out to the speed team [2] to > > hear their view on implementing something like this. They have a ton of > > experience in this field. We have a lot of trace calls in the code already but > > AFAIK nobody has looked at how to use that information from a purely native > > perspective (it's only used when WebRTC is a part of Chrome). > > You could probably check with somebody like tommi@ or magjed@ in our team if > > they know more about this. > > > > [1]: http://www.chromium.org/developers/how-tos/trace-event-profiling-tool > > [2]: https://www.chromium.org/developers/speed-infra > > Tracing will give us time information. But what I am trying to do is to get CPU > utilization metric. I was specifically asked to do that. This is a different > story, although they are correlated. > > Also, there are similar things in chrome, see: > https://cs.chromium.org/chromium/src/v8/src/base/platform/time.cc?q=CLOCK_THR... > But you are right, looks like it's not used anywhere except in some tests. I found the similar functionality in the base/process/process_metrics.h. It's not so fine grained and doesn't have any way to exclude threads. Is it preferable to use it instead? Should I just delete this CL?
https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:44: LOG_ERR(LS_ERROR) << "clock_gettime() failed."; On 2017/02/16 12:20:18, språng wrote: > I've not seen the LOG_ERR macro before. Is it to specifically log to stderr? Why > not plain LOG(LS_ERROR) here and below? I suggested using LOG_ERR. The difference to plain LOG is that it displays the errno value and/or the corresponding string. (At least according to the docs, I haven't really used it myself).
On 2017/02/16 12:21:23, ilnik wrote: > On 2017/02/16 11:39:12, ilnik wrote: > > On 2017/02/16 09:31:14, kjellander_webrtc wrote: > > > On 2017/02/16 08:46:39, nisse-webrtc wrote: > > > > On 2017/02/16 08:43:10, ilnik wrote: > > > > > That way we can measure each testcase independently. Measuring whole > > process > > > > > lifetime is useless if we know that some test now takes 20% more compute > > > time, > > > > > but have no idea, which one is it. > > > > > > > > I agree clock_gettime is the right tool for this use case. It's always a > bit > > > > difficult to review when not knowing the broader context of the changes. > > > > > > I also agree, those are all good points. Measuring with finer granularity > will > > > certainly allow us to detect more regressions. > > > However, the fact that I'm unable to find anything like this in the Chromium > > > codebase makes me think that it's possible to accomplish the same results > with > > > other techniques. Especially as Chrome is very speed focused and have a > whole > > > team working on only measuring performance. > > > I suggest you read up on tracing [1] and/or reach out to the speed team [2] > to > > > hear their view on implementing something like this. They have a ton of > > > experience in this field. We have a lot of trace calls in the code already > but > > > AFAIK nobody has looked at how to use that information from a purely native > > > perspective (it's only used when WebRTC is a part of Chrome). > > > You could probably check with somebody like tommi@ or magjed@ in our team if > > > they know more about this. > > > > > > [1]: http://www.chromium.org/developers/how-tos/trace-event-profiling-tool > > > [2]: https://www.chromium.org/developers/speed-infra > > > > Tracing will give us time information. But what I am trying to do is to get > CPU > > utilization metric. I was specifically asked to do that. This is a different > > story, although they are correlated. > > > > Also, there are similar things in chrome, see: > > > https://cs.chromium.org/chromium/src/v8/src/base/platform/time.cc?q=CLOCK_THR... > > But you are right, looks like it's not used anywhere except in some tests. > > I found the similar functionality in the base/process/process_metrics.h. It's > not so fine grained and doesn't have any way to exclude threads. > Is it preferable to use it instead? Should I just delete this CL? They're in chromium's base, not webrtc/base? If so we can't use them directly. I haven't looked in detail at what they've done, but we don't want to clone it if it's overly complex. I'm sure you can use it as inspiration though.
https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:23: #endif On 2017/02/16 12:20:18, språng wrote: > At this point this file isn't overly large, but I still think it could be wort > splitting this up into platform dependent .cc files and have gn pick the correct > one to build. The fewer #ifdef's the better, imo. > > wdyt? I think file is not big enough to do it. https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:68: LOG(LS_ERROR) << "No function to get CPU time"; On 2017/02/16 12:20:18, språng wrote: > Is there any non-supported platform? Maybe we should just do an > RTC_NOT_REACHED(); instead? Done. https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:22: int64_t& counter = *reinterpret_cast<int64_t*>(counter_pointer); On 2017/02/16 12:20:18, språng wrote: > Avoid non-const refs, just keep it after casting to int64_t* Done. https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:37: int64_t* counter = new int64_t; On 2017/02/16 12:20:18, språng wrote: > No need to heap-allocate this. _asan tests were mistaking this access for stack buffer overflows. To pass tests I had to use heap allocated counters. https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:46: kProcessingTimeMillisecs + kAllowedErrorMillisecs); On 2017/02/16 12:20:19, språng wrote: > EXPECT_NEAR ? Did not know about that. Should use it.
https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:37: int64_t* counter = new int64_t; On 2017/02/16 12:38:55, ilnik wrote: > On 2017/02/16 12:20:18, språng wrote: > > No need to heap-allocate this. > > _asan tests were mistaking this access for stack buffer overflows. To pass tests > I had to use heap allocated counters. Odd. Passing pointers to stack allocated objects is a very common thing to do. You have to wait for the other thread to finish before exiting the scope where the variable lives, but that's equally tru with manual new and delete. If you really need it on the heap, consider using std::unique_ptr, so you don't need any explicit delete.
https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:37: int64_t* counter = new int64_t; On 2017/02/16 12:49:46, nisse-webrtc wrote: > On 2017/02/16 12:38:55, ilnik wrote: > > On 2017/02/16 12:20:18, språng wrote: > > > No need to heap-allocate this. > > > > _asan tests were mistaking this access for stack buffer overflows. To pass > tests > > I had to use heap allocated counters. > > Odd. Passing pointers to stack allocated objects is a very common thing to do. > You have to wait for the other thread to finish before exiting the scope where > the variable lives, but that's equally tru with manual new and delete. If you > really need it on the heap, consider using std::unique_ptr, so you don't need > any explicit delete. Maybe ASAN were confused because there was another thread using pointer to stack-allocated variables in another thread. I will use unique_ptr now.
The CQ bit was checked by ilnik@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
On 2017/02/16 12:55:42, ilnik wrote: > https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... > File webrtc/base/cpu_time_unittest.cc (right): > > https://codereview.webrtc.org/2695743003/diff/360001/webrtc/base/cpu_time_uni... > webrtc/base/cpu_time_unittest.cc:37: int64_t* counter = new int64_t; > On 2017/02/16 12:49:46, nisse-webrtc wrote: > > On 2017/02/16 12:38:55, ilnik wrote: > > > On 2017/02/16 12:20:18, språng wrote: > > > > No need to heap-allocate this. > > > > > > _asan tests were mistaking this access for stack buffer overflows. To pass > > tests > > > I had to use heap allocated counters. > > > > Odd. Passing pointers to stack allocated objects is a very common thing to do. > > You have to wait for the other thread to finish before exiting the scope where > > the variable lives, but that's equally tru with manual new and delete. If you > > really need it on the heap, consider using std::unique_ptr, so you don't need > > any explicit delete. > > Maybe ASAN were confused because there was another thread using pointer to > stack-allocated variables in another thread. I will use unique_ptr now. Actually, ASAN was failing because another mistake fixed long time ago. Similar functionality is in base/ (not webrtc/base/) directory. From my understanding, I can't use it. Both Nisse's and Sprang's comments are implemented. Now, I need lgtm from the owner of the base/ directory. mflodman@, what do you think? Could you give it, or who do I ask?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Mainly nits https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:11: #if defined(WEBRTC_LINUX) Should these be "WEBRTC_POSIX" instead? https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:25: #include "webrtc/base/cpu_time.h" nit: Usually we put the include for the matching header file at the top of the cc file. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:68: RTC_NOT_REACHED(); This will only trigger an error at run time. Should it be a compile time error? https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h File webrtc/base/cpu_time.h (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:19: // Use to calculate difference as some offset may be presented. I don't quite understand the "use to calculate difference" comment. Is this saying that there are no guarantees about the initial time offset, and so this time should only be used for calculating differences? If so, it may be useful to make the return value a class that only *allows* taking differences. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:20: int64_t GetProcessCpuTime(); Can we call this GetProcessCpuTimeNanos, so the units are clear from the call site? We use this kind of naming for "SystemTimeNanos" and "TimeNanos". https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:22: // Returns total CPU time of a current thread in nanoseconds. nit: extra spaces in comments. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:39: std::unique_ptr<int64_t> counter(new int64_t); Is this because the int64_t needs to be on the heap? https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:44: EXPECT_NEAR(static_cast<double>(duration_nanos) / kNumNanosecsPerMillisec, nit: I'd prefer no cast to double https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:65: 2 * kProcessingTimeMillisecs, 2 * kAllowedErrorMillisecs); This test will fail if the system doesn't have multiple cores to run threads on. May want to make a note of that, or call CpuInfo::DetectNumberOfCores at the top of the test.
https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:11: #if defined(WEBRTC_LINUX) On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > Should these be "WEBRTC_POSIX" instead? No. I think POSIX is also defined for MAC, but it doesn't support CLOCK_THREAD_CPUTIME_ID, afaik. So the order of ifdefs will matter if POSIX is used, as alternatives will not be exclusive. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:25: #include "webrtc/base/cpu_time.h" On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > nit: Usually we put the include for the matching header file at the top of the > cc file. Done. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:68: RTC_NOT_REACHED(); On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > This will only trigger an error at run time. Should it be a compile time error? Done. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h File webrtc/base/cpu_time.h (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:19: // Use to calculate difference as some offset may be presented. On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > I don't quite understand the "use to calculate difference" comment. Is this > saying that there are no guarantees about the initial time offset, and so this > time should only be used for calculating differences? If so, it may be useful to > make the return value a class that only *allows* taking differences. I guess, just mentioning that may not start from 0 will be enough here. Just like with most of the other clock functions, there is some offset. For CPU time consumed by process it may be logical to start from 0 at the start of the process, but it's not like that for all the platforms. I think making special return type will be overkill here if that functions will be used in the same way other time functions are used. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:20: int64_t GetProcessCpuTime(); On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > Can we call this GetProcessCpuTimeNanos, so the units are clear from the call > site? We use this kind of naming for "SystemTimeNanos" and "TimeNanos". Done. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:22: // Returns total CPU time of a current thread in nanoseconds. On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > nit: extra spaces in comments. Done. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:39: std::unique_ptr<int64_t> counter(new int64_t); On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > Is this because the int64_t needs to be on the heap? I was getting ASAN errors, but it wasn't because counter was on stack, yet I thought it was. Fixed this now. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:44: EXPECT_NEAR(static_cast<double>(duration_nanos) / kNumNanosecsPerMillisec, On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > nit: I'd prefer no cast to double Done. https://codereview.webrtc.org/2695743003/diff/380001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:65: 2 * kProcessingTimeMillisecs, 2 * kAllowedErrorMillisecs); On 2017/02/16 18:49:19, Taylor Brandstetter wrote: > This test will fail if the system doesn't have multiple cores to run threads on. > May want to make a note of that, or call CpuInfo::DetectNumberOfCores at the top > of the test. Done.
lgtm https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:103: LOG_ERR(LS_ERROR) << "GetProcessTimes() failed."; Just noticed, this should say GetThreadTimes() https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:19: nit: extra newlines
https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:103: LOG_ERR(LS_ERROR) << "GetProcessTimes() failed."; On 2017/02/17 09:45:13, Taylor Brandstetter wrote: > Just noticed, this should say GetThreadTimes() Done. https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time_uni... File webrtc/base/cpu_time_unittest.cc (right): https://codereview.webrtc.org/2695743003/diff/400001/webrtc/base/cpu_time_uni... webrtc/base/cpu_time_unittest.cc:19: On 2017/02/17 09:45:14, Taylor Brandstetter wrote: > nit: extra newlines Done.
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2695743003/#ps420001 (title: "deadbeef@ comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
lgtm https://codereview.webrtc.org/2695743003/diff/420001/webrtc/base/cpu_time.cc File webrtc/base/cpu_time.cc (right): https://codereview.webrtc.org/2695743003/diff/420001/webrtc/base/cpu_time.cc#... webrtc/base/cpu_time.cc:69: static_assert(false); nit: Maybe even static_assert(false, "Platform support not yet implemented."); ? https://codereview.webrtc.org/2695743003/diff/420001/webrtc/base/cpu_time.h File webrtc/base/cpu_time.h (right): https://codereview.webrtc.org/2695743003/diff/420001/webrtc/base/cpu_time.h#n... webrtc/base/cpu_time.h:19: // May start not from 0. nit: "May not start at 0." Or just comment that time base is unknown and this can only be used for deltas.
The CQ bit was unchecked by ilnik@webrtc.org
The CQ bit was checked by ilnik@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from sprang@webrtc.org, deadbeef@webrtc.org Link to the patchset: https://codereview.webrtc.org/2695743003/#ps440001 (title: "implemented Sprang@ comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
CQ is committing da patch.
Bot data: {"patchset_id": 440001, "attempt_start_ts": 1487327644987290,
"parent_rev": "c7bf32a110af7f427e14c0dc32e3f3789e51c096", "commit_rev":
"3ff474b72ba1b34027e8391b1014c2838af445d4"}
Message was sent while issue was closed.
Description was changed from ========== Added GetCpuTime to base/ to get total CPU time consumed by process for perf tests. BUG=webrtc:7095 ========== to ========== Added GetCpuTime to base/ to get total CPU time consumed by process for perf tests. BUG=webrtc:7095 Review-Url: https://codereview.webrtc.org/2695743003 Cr-Commit-Position: refs/heads/master@{#16665} Committed: https://chromium.googlesource.com/external/webrtc/+/3ff474b72ba1b34027e8391b1... ==========
Message was sent while issue was closed.
Committed patchset #24 (id:440001) as https://chromium.googlesource.com/external/webrtc/+/3ff474b72ba1b34027e8391b1...
Message was sent while issue was closed.
A revert of this CL (patchset #24 id:440001) has been created in https://codereview.webrtc.org/2698333004/ by tommi@webrtc.org. The reason for reverting is: The GetThreadCpuTimeTest.SingleThread and .TwoThreads tests are unfortunately flaky on Mac (maybe other platforms). See for example: https://build.chromium.org/p/client.webrtc/builders/Mac%20Asan/builds/11271/s... https://build.chromium.org/p/client.webrtc/builders/Mac64%20Debug/builds/1039... https://build.chromium.org/p/client.webrtc/builders/Mac%20Asan/builds/11271/s... Since it's late, I'll have to revert the CL to get the tree and trybots green (instead of only disabling the failing tests)..
Message was sent while issue was closed.
https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn File webrtc/base/BUILD.gn (right): https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", Sorry I didn't notice earlier, but this should go in rtc_base_nonparallel_tests. These are tests that aren't run in parallel, so any tests based on real system time or OS-level resources like sockets or files should use it. That hopefully should fix the builtbot failures.
Message was sent while issue was closed.
On 2017/02/17 23:35:47, Taylor Brandstetter wrote: > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn > File webrtc/base/BUILD.gn (right): > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... > webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", > Sorry I didn't notice earlier, but this should go in rtc_base_nonparallel_tests. > These are tests that aren't run in parallel, so any tests based on real system > time or OS-level resources like sockets or files should use it. That hopefully > should fix the builtbot failures. Taylor, while I am fixing Build.gn, could I also move cpu_time.cc and cpu_time.h to rtc_base or rtc_base_approved? It looks like I can't depend on rtc_base_tests_utils from outside of this file.
Message was sent while issue was closed.
On 2017/02/20 08:42:47, ilnik wrote: > On 2017/02/17 23:35:47, Taylor Brandstetter wrote: > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn > > File webrtc/base/BUILD.gn (right): > > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... > > webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", > > Sorry I didn't notice earlier, but this should go in > rtc_base_nonparallel_tests. > > These are tests that aren't run in parallel, so any tests based on real system > > time or OS-level resources like sockets or files should use it. That hopefully > > should fix the builtbot failures. > > Taylor, while I am fixing Build.gn, could I also move cpu_time.cc and cpu_time.h > to rtc_base or rtc_base_approved? > It looks like I can't depend on rtc_base_tests_utils from outside of this file. If this is only intended to be used for tests, it seems like rtc_base_test_utils is the right place. Can you explain more about the issue you're seeing?
Message was sent while issue was closed.
On 2017/02/20 10:53:45, Taylor Brandstetter wrote: > On 2017/02/20 08:42:47, ilnik wrote: > > On 2017/02/17 23:35:47, Taylor Brandstetter wrote: > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn > > > File webrtc/base/BUILD.gn (right): > > > > > > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... > > > webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", > > > Sorry I didn't notice earlier, but this should go in > > rtc_base_nonparallel_tests. > > > These are tests that aren't run in parallel, so any tests based on real > system > > > time or OS-level resources like sockets or files should use it. That > hopefully > > > should fix the builtbot failures. > > > > Taylor, while I am fixing Build.gn, could I also move cpu_time.cc and > cpu_time.h > > to rtc_base or rtc_base_approved? > > It looks like I can't depend on rtc_base_tests_utils from outside of this > file. > > If this is only intended to be used for tests, it seems like rtc_base_test_utils > is the right place. Can you explain more about the issue you're seeing? I am trying to use functions from webrtc/base/cpu_time.cc in webrtc/video/video_quality_tests. So I need to somehow reference cpu_time.cc from webrtc/video/BUILD.gn. I don't fully understand these BUILD.gn files, but it looks like I can't add anything except from public_deps in other BUILD.gn files. rtc_base_tests_utils is not even defined as rtc_static_library, it's rtc_source_set. Do I add dependency on directly cpu_time.cc and cpu_time.h in video/BUILD.gn then?
Message was sent while issue was closed.
On 2017/02/20 11:44:37, ilnik wrote: > On 2017/02/20 10:53:45, Taylor Brandstetter wrote: > > On 2017/02/20 08:42:47, ilnik wrote: > > > On 2017/02/17 23:35:47, Taylor Brandstetter wrote: > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn > > > > File webrtc/base/BUILD.gn (right): > > > > > > > > > > > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... > > > > webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", > > > > Sorry I didn't notice earlier, but this should go in > > > rtc_base_nonparallel_tests. > > > > These are tests that aren't run in parallel, so any tests based on real > > system > > > > time or OS-level resources like sockets or files should use it. That > > hopefully > > > > should fix the builtbot failures. > > > > > > Taylor, while I am fixing Build.gn, could I also move cpu_time.cc and > > cpu_time.h > > > to rtc_base or rtc_base_approved? > > > It looks like I can't depend on rtc_base_tests_utils from outside of this > > file. > > > > If this is only intended to be used for tests, it seems like > rtc_base_test_utils > > is the right place. Can you explain more about the issue you're seeing? > > I am trying to use functions from webrtc/base/cpu_time.cc in > webrtc/video/video_quality_tests. So I need to somehow reference cpu_time.cc > from webrtc/video/BUILD.gn. > I don't fully understand these BUILD.gn files, but it looks like I can't add > anything except from public_deps in other BUILD.gn files. > rtc_base_tests_utils is not even defined as rtc_static_library, it's > rtc_source_set. > > Do I add dependency on directly cpu_time.cc and cpu_time.h in video/BUILD.gn > then? I think the right way is to add rtc_base_tests_utils as a dependency for video_quality_test.
Message was sent while issue was closed.
On 2017/02/20 12:21:29, nisse-webrtc wrote: > On 2017/02/20 11:44:37, ilnik wrote: > > On 2017/02/20 10:53:45, Taylor Brandstetter wrote: > > > On 2017/02/20 08:42:47, ilnik wrote: > > > > On 2017/02/17 23:35:47, Taylor Brandstetter wrote: > > > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn > > > > > File webrtc/base/BUILD.gn (right): > > > > > > > > > > > > > > > > > > > > https://codereview.webrtc.org/2695743003/diff/440001/webrtc/base/BUILD.gn#new... > > > > > webrtc/base/BUILD.gn:770: "cpu_time_unittest.cc", > > > > > Sorry I didn't notice earlier, but this should go in > > > > rtc_base_nonparallel_tests. > > > > > These are tests that aren't run in parallel, so any tests based on real > > > system > > > > > time or OS-level resources like sockets or files should use it. That > > > hopefully > > > > > should fix the builtbot failures. > > > > > > > > Taylor, while I am fixing Build.gn, could I also move cpu_time.cc and > > > cpu_time.h > > > > to rtc_base or rtc_base_approved? > > > > It looks like I can't depend on rtc_base_tests_utils from outside of this > > > file. > > > > > > If this is only intended to be used for tests, it seems like > > rtc_base_test_utils > > > is the right place. Can you explain more about the issue you're seeing? > > > > I am trying to use functions from webrtc/base/cpu_time.cc in > > webrtc/video/video_quality_tests. So I need to somehow reference cpu_time.cc > > from webrtc/video/BUILD.gn. > > I don't fully understand these BUILD.gn files, but it looks like I can't add > > anything except from public_deps in other BUILD.gn files. > > rtc_base_tests_utils is not even defined as rtc_static_library, it's > > rtc_source_set. > > > > Do I add dependency on directly cpu_time.cc and cpu_time.h in video/BUILD.gn > > then? > > I think the right way is to add rtc_base_tests_utils as a dependency > for video_quality_test. Thank, you. It works. Last time I tried to do this, I did something wrong, and got ninja errors. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
