Switch Mem::Meter to use std::chrono API#2392
Switch Mem::Meter to use std::chrono API#2392yadij wants to merge 1 commit intosquid-cache:masterfrom
Conversation
.. instead of our old time API.
yadij
added
the
S-could-use-an-approval
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
Please stop posting new pull requests until the backlog is dealt with. I plan to come back to this PR after that happens.
| ssize_t currentLevel() const {return level;} | ||
| ssize_t peak() const {return hwater_level;} | ||
| time_t peakTime() const {return hwater_stamp;} | ||
| const Timestamp &peakTime() const {return hwater_stamp;} |
There was a problem hiding this comment.
Please do not introduce unnecessary references. The size of this data member is usually 8 bytes, same as a reference size, but without the reference complications.
| const Timestamp &peakTime() const {return hwater_stamp;} | |
| auto peakTime() const { return hwater_stamp; } |
| if (hwater_level < level) { | ||
| hwater_level = level; | ||
| hwater_stamp = squid_curtime ? squid_curtime : time(nullptr); | ||
| hwater_stamp = std::chrono::system_clock::now(); |
There was a problem hiding this comment.
IIRC, we should continue using Squid current time "cache" in code like this. getCurrentTime() is already using std::chrono::system_clock::now() internally, so we need to remember/expose that value via a function. I did not check whether there are open PRs doing that already.
|
|
||
| auto AsHours = [](const auto &base) -> double { | ||
| const auto span = std::chrono::duration_cast<std::chrono::seconds>(std::chrono::system_clock::now() - base); | ||
| return span.count() / 3600.0; |
There was a problem hiding this comment.
This kind of manual math is essentially a reinterpret_cast<> equivalent. It is error-prone. I hope it is possible to avoid this unchecked (by compiler) time "casting" when using std::chrono in new code.
| stream << delim; | ||
| } | ||
|
|
||
| auto AsHours = [](const auto &base) -> double { |
There was a problem hiding this comment.
Please use lowercase for local variables, avoid unwanted references (see another change request), and use const/AAA as much as possible.
And since this is not an I/O manipulator but a casting function, let's call it "[convert] toX()" rather than "[print] asX()".
Here is a sketch:
| auto AsHours = [](const auto &base) -> double { | |
| const auto toHours = [](const auto base) { |
| } | ||
|
|
||
| auto AsHours = [](const auto &base) -> double { | ||
| const auto span = std::chrono::duration_cast<std::chrono::seconds>(std::chrono::system_clock::now() - base); |
There was a problem hiding this comment.
I hope it is possible to avoid duration_cast in this math. If it is possible, we should avoid it. I do not have a ready-to-use recipe at this time.
There was a problem hiding this comment.
The system_clock operates in nanosecond ticks and our math is calculating in seconds. AFAIK, the duration cast is the "proper" (ie most efficient) way to change time scales.
rousskov
removed
the
S-could-use-an-approval
2 participants
.. instead of our old time API.