Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

log messages periodically (time-based) #669

Merged
merged 1 commit into from
Jul 5, 2021
Merged

log messages periodically (time-based) #669

merged 1 commit into from
Jul 5, 2021

Conversation

darbitman
Copy link
Contributor

@darbitman darbitman commented Jun 13, 2021 •
edited
Loading

Closes #663

  • Use constexpr if c++11, otherwise const
  • If C++11 (or greater), provide an implementation to log every time period T using std::chrono and std::atomic
  • For pre-C++11, use struct timespec from <ctime> or <time.h> on Linux platforms. On Windows platforms, use QueryPerformanceCounter and QueryPerformanceFrequency

@google-cla google-cla bot added the cla: yes label Jun 13, 2021
@darbitman darbitman marked this pull request as ready for review June 16, 2021 02:16
sergiud
sergiud requested changes Jun 16, 2021
View reviewed changes
Copy link
Collaborator

@sergiud sergiud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. Could you please add tests?

src/glog/logging.h.in Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
@darbitman darbitman marked this pull request as draft June 16, 2021 13:10
@darbitman darbitman marked this pull request as ready for review June 26, 2021 17:33
@darbitman
Copy link
Contributor Author

Thanks for the PR. Could you please add tests?

Done. hopefully they're sufficient.

@sergiud
Copy link
Collaborator

sergiud commented Jun 26, 2021

There are some build failures.

@darbitman darbitman marked this pull request as draft June 29, 2021 03:36
@darbitman darbitman marked this pull request as ready for review July 2, 2021 01:42
sergiud
sergiud requested changes Jul 3, 2021
View reviewed changes
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/logging_unittest.cc Outdated Show resolved Hide resolved
@darbitman darbitman force-pushed the log-every-time-period branch 2 times, most recently from 497f1f4 to fd83d3c Compare July 3, 2021 19:31
sergiud
sergiud requested changes Jul 4, 2021
View reviewed changes
Copy link
Collaborator

@sergiud sergiud left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks mostly fine. I still have a few remarks. Could you please eventually squash the commits?

bazel/glog.bzl Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/glog/logging.h.in Outdated Show resolved Hide resolved
src/logging_unittest.cc Outdated
return stream;
}
// get elapsed time in microseconds
int64 elapsedTime_us(const timespec& begin, const timespec& end) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I previously overlooked. Wouldn't it make sense to always you the highest possible precision in ns?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for this particular test, because the error tolerance is 500us and any variation in the hundreds of nanoseconds is insignificant. That being said, I'll go back and change to nanoseconds to remove some of the math just to simply the functions.

@darbitman
Copy link
Contributor Author

Looks mostly fine. I still have a few remarks. Could you please eventually squash the commits?

Yeah I'll do that.

One last thing...
I have the HAVE_CXX11_CHRONO guard in logging_unittest.cc:1016, but I realized I removed that from the config.h.in. How do you think I should proceed since you mentioned that the config.h macros are really used internally?

@sergiud
Copy link
Collaborator

sergiud commented Jul 4, 2021

You can still define HAVE_CXX11_CHRONO in the config.h.cmake.in if you need it.

@darbitman
Copy link
Contributor Author

darbitman commented Jul 4, 2021 •
edited
Loading

That'll be plan B. Do you think it's fair to say for OS_LINUX (or as the #else clause to an #ifdef OS_WINDOWS) I can just use struct timespec even if <chrono> exists and keep the implementation for OS_WINDOWS as is?

@sergiud
Copy link
Collaborator

sergiud commented Jul 4, 2021

I would prefer <chrono> over platform specific functionality.

sergiud
sergiud requested changes Jul 5, 2021
View reviewed changes
src/glog/logging.h.in Outdated Show resolved Hide resolved
@darbitman
Implement LOG_EVERY_T.
a1374c4 
Use <chrono> and <atomic> for C++11 or greater.
For non-Windows pre-C++11 systems, use <time.h> and built-in atomic operations.
For Windows pre-C++11, use the Windows implementations for time and atomic operations.
@sergiud sergiud self-assigned this Jul 5, 2021
@sergiud sergiud added this to the 0.6 milestone Jul 5, 2021
@sergiud
Copy link
Collaborator

sergiud commented Jul 5, 2021

Thanks!

@sergiud sergiud merged commit 42ce901 into google:master Jul 5, 2021
@sergiud sergiud mentioned this pull request Jul 14, 2021
Closed
@sergiud sergiud mentioned this pull request Jan 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergiud sergiud sergiud approved these changes

Assignees

@sergiud sergiud

Labels
cla: yes
Projects
None yet
Milestone
0.6
Development

Successfully merging this pull request may close these issues.

Periodic log using time instead of count
2 participants
@darbitman @sergiud