Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2175713002:
Log current time if we stop logging before stop_time_. (Closed)
|
Created:
4 years, 5 months ago by terelius Modified:
4 years, 4 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, the sun, pbos-webrtc, stefan-webrtc, mflodman Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionLog current time if we stop logging before stop_time_.
This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool.
BUG=webrtc:6138
Committed: https://crrev.com/76a44d5dafa4114c52c55d5dd84de3f4f02dc95b
Cr-Commit-Position: refs/heads/master@{#13581}
Patch Set 1 #Patch Set 2 : Format #
Total comments: 3
Messages
Total messages: 20 (8 generated)
terelius@webrtc.org changed reviewers: + ivoc@webrtc.org, stefan@webrtc.org
Please take a look
Description was changed from ========== Log current time we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 ========== to ========== Log current time if we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 ==========
https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.cc (right): https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:231: std::min(stop_time_, clock_->TimeInMicroseconds())); Do you know if the value from TimeInMicroseconds() can wrap?
pbos@webrtc.org changed reviewers: + pbos@webrtc.org
https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.cc (right): https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:231: std::min(stop_time_, clock_->TimeInMicroseconds())); On 2016/07/27 09:18:00, ivoc wrote: > Do you know if the value from TimeInMicroseconds() can wrap? This should never wrap. (It calls rtc::TimeMicros() which is ~64-bit sized, all of our code relies on this not wrapping).
https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... File webrtc/call/rtc_event_log_helper_thread.cc (right): https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... webrtc/call/rtc_event_log_helper_thread.cc:231: std::min(stop_time_, clock_->TimeInMicroseconds())); On 2016/07/27 16:44:20, pbos-webrtc wrote: > On 2016/07/27 09:18:00, ivoc wrote: > > Do you know if the value from TimeInMicroseconds() can wrap? > > This should never wrap. (It calls rtc::TimeMicros() which is ~64-bit sized, all > of our code relies on this not wrapping). When I wrote the code, I assumed that the 64-bit time couldn't wrap. However, after looking at it earlier today, I am not so sure. On mac, rtc::TimeMicros() does ticks = mach_absolute_time() * timebase.numer / timebase.denom; where mach_absolute_time() measures (roughly) CPU cycles and timebase is a fraction that scales the CPU cycles to nanoseconds. If the timebase is 1000000000/33333335 then the computation will wrap after about 10 minutes. https://developer.apple.com/library/mac/qa/qa1398/_index.html also mentions that there is a danger of overflowing. On the other hand, I've also read that the timebase is 1/1 on Intel CPU so this would only be an issue on PowerPC. If it does wrap on some platform, we're going to have bigger problems than just an incorrect timestamp in a log, though.
On 2016/07/27 20:34:40, terelius wrote: > https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... > File webrtc/call/rtc_event_log_helper_thread.cc (right): > > https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... > webrtc/call/rtc_event_log_helper_thread.cc:231: std::min(stop_time_, > clock_->TimeInMicroseconds())); > On 2016/07/27 16:44:20, pbos-webrtc wrote: > > On 2016/07/27 09:18:00, ivoc wrote: > > > Do you know if the value from TimeInMicroseconds() can wrap? > > > > This should never wrap. (It calls rtc::TimeMicros() which is ~64-bit sized, > all > > of our code relies on this not wrapping). > > When I wrote the code, I assumed that the 64-bit time couldn't wrap. However, > after looking at it earlier today, I am not so sure. > > On mac, rtc::TimeMicros() does > ticks = mach_absolute_time() * timebase.numer / timebase.denom; > where mach_absolute_time() measures (roughly) CPU cycles and timebase is a > fraction that scales the CPU cycles to nanoseconds. If the timebase is > 1000000000/33333335 then the computation will wrap after about 10 minutes. > > https://developer.apple.com/library/mac/qa/qa1398/_index.html also mentions that > there is a danger of overflowing. On the other hand, I've also read that the > timebase is 1/1 on Intel CPU so this would only be an issue on PowerPC. > > If it does wrap on some platform, we're going to have bigger problems than just > an incorrect timestamp in a log, though. If that's the case then that's a bug on rtc::TimeMicros that needs to be fixed there (and not guarded-for here). Feel free to create a bug for it if the code looks dubious. :)
lgtm
On 2016/07/27 20:36:42, pbos wrote: > On 2016/07/27 20:34:40, terelius wrote: > > > https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... > > File webrtc/call/rtc_event_log_helper_thread.cc (right): > > > > > https://codereview.webrtc.org/2175713002/diff/20001/webrtc/call/rtc_event_log... > > webrtc/call/rtc_event_log_helper_thread.cc:231: std::min(stop_time_, > > clock_->TimeInMicroseconds())); > > On 2016/07/27 16:44:20, pbos-webrtc wrote: > > > On 2016/07/27 09:18:00, ivoc wrote: > > > > Do you know if the value from TimeInMicroseconds() can wrap? > > > > > > This should never wrap. (It calls rtc::TimeMicros() which is ~64-bit sized, > > all > > > of our code relies on this not wrapping). > > > > When I wrote the code, I assumed that the 64-bit time couldn't wrap. However, > > after looking at it earlier today, I am not so sure. > > > > On mac, rtc::TimeMicros() does > > ticks = mach_absolute_time() * timebase.numer / timebase.denom; > > where mach_absolute_time() measures (roughly) CPU cycles and timebase is a > > fraction that scales the CPU cycles to nanoseconds. If the timebase is > > 1000000000/33333335 then the computation will wrap after about 10 minutes. > > > > https://developer.apple.com/library/mac/qa/qa1398/_index.html also mentions > that > > there is a danger of overflowing. On the other hand, I've also read that the > > timebase is 1/1 on Intel CPU so this would only be an issue on PowerPC. > > > > If it does wrap on some platform, we're going to have bigger problems than > just > > an incorrect timestamp in a log, though. > > If that's the case then that's a bug on rtc::TimeMicros that needs to be fixed > there (and not guarded-for here). Feel free to create a bug for it if the code > looks dubious. :) Good points, LGTM.
The CQ bit was checked by terelius@webrtc.org
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
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...)
The CQ bit was checked by terelius@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Log current time if we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 ========== to ========== Log current time if we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Log current time if we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 ========== to ========== Log current time if we stop logging before stop_time_. This happens if we stop logging because we have reached the file size limit. The large timestamp causes problems in the analysis tool. BUG=webrtc:6138 Committed: https://crrev.com/76a44d5dafa4114c52c55d5dd84de3f4f02dc95b Cr-Commit-Position: refs/heads/master@{#13581} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/76a44d5dafa4114c52c55d5dd84de3f4f02dc95b Cr-Commit-Position: refs/heads/master@{#13581} |
