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

Encapsulate code converting std::chrono::duration to timeval #1915

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+20 −10
Open

Encapsulate code converting std::chrono::duration to timeval #1915

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
df12c15
Encapsulate code converting std::chrono::duration to timeval
rousskov Oct 11, 2024
4e7a5ab
Found one more place that converts chrono::duration to timeval
rousskov Oct 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions src/format/Format.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,11 +640,7 @@ Format::Format::assemble(MemBuf &mb, const AccessLogEntry::Pointer &al, int logS
const auto &timer = (!al->hier.totalPeeringTime.ran() && al->request) ?
al->request->hier.totalPeeringTime : al->hier.totalPeeringTime;
if (timer.ran()) {
using namespace std::chrono_literals;
const auto duration = timer.total();
outtv.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(duration).count();
const auto totalUsec = std::chrono::duration_cast<std::chrono::microseconds>(duration);
outtv.tv_usec = (totalUsec % std::chrono::microseconds(1s)).count();
outtv = ToTimeval(timer.total());
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR; I do not believe this one is a true need. The doMsec cases should all be replaced by an output printing chrono::duration instead.

Better to add that new display logic and make this %code the first usage of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The doMsec cases should all be replaced by an output printing chrono::duration instead.

The changes in this PR do not preclude those future assembly() improvements. In fact, they can be viewed as making those future improvements slightly easier!

Better to add that new display logic and make this %code the first usage of it.

Correctly improving assemble() to get rid of doMsec and similar code would be good, but it is a complex, large task that is orthogonal to the small improvement offered by this PR. We should not view this as "either or" or "better X than Y" situation. Both "X" and "Y" are moving Squid code forward. "X" got here first; let's merge it.

doMsec = 1;
}
}
Expand Down
4 changes: 1 addition & 3 deletions src/icmp/pinger.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,7 @@ main(int, char **)
#endif

for (;;) {
struct timeval tv;
tv.tv_sec = std::chrono::seconds(PingerTimeout).count();
tv.tv_usec = 0;
auto tv = ToTimeval(PingerTimeout);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This tv cannot be constant because it is fed to select(2) that requires a non-constant timeval timeout parameter.

Copy link
Contributor

Choose a reason for hiding this comment

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

The new converter handles tv_usec which adds a performance regression calculating it to be 0 on every loop iteration. That being the only real "gain" pushes this into the "not valueable yet" type of changes for me.

Copy link
Contributor Author

@rousskov rousskov Oct 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The new converter handles tv_usec which adds a performance regression calculating it to be 0 on every loop iteration.

AFAICT, the above statement is based on two false assumptions:

False assumption A: Proposed compiled code will be different from official compiled code. In reality, modern optimizing compilers see enough in this context to generate identical assembly in both cases (even at -O1 optimization level)!

False assumption B: This loop is performance sensitive (at the precision level of a few integer operations added or removed). In reality, this is an I/O-waiting loop in a pinger helper. Adding a few quick ops to that loop would not change pinger performance, even under heavy load.

In summary, this PR does not "add a performance regression".

N.B. Proposed loop code is also clearly better -- it is a bit easier to read, and it supports reasonable PingerTimeout values that the official code silently mishandles. We may never need to support fractional seconds in this context, but better code is still an improvement.

That being the only real "gain" pushes this into the "not valueable yet" type of changes for me.

Please do not block PRs just because you do not see value in them (when other core developers do see value).

FD_ZERO(&R);
if (icmp4_worker >= 0) {
FD_SET(icmp4_worker, &R);
Expand Down
17 changes: 17 additions & 0 deletions src/time/gadgets.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#ifndef SQUID_SRC_TIME_GADGETS_H
#define SQUID_SRC_TIME_GADGETS_H

#include <chrono>
#include <ctime>
#include <iosfwd>

Expand Down Expand Up @@ -95,6 +96,22 @@ inline long int tvToMsec(struct timeval &t)
return t.tv_sec * 1000 + t.tv_usec / 1000;
}

/// Converts std::chrono::duration to timeval. Input values up to ~68 years do
/// not overflow timeval even if timeval::tv_sec (i.e. time_t) has only 32 bits.
/// Input values with precision higher than microseconds decrease precision
/// because timeval::tv_usec stores microseconds.
template <typename Duration>
timeval
ToTimeval(const Duration duration)
{
using namespace std::chrono_literals;
timeval out;
out.tv_sec = std::chrono::duration_cast<std::chrono::seconds>(duration).count();
const auto totalUsec = std::chrono::duration_cast<std::chrono::microseconds>(duration);
out.tv_usec = (totalUsec % std::chrono::microseconds(1s)).count();
return out;
}

/// prints <seconds>.<microseconds>
std::ostream &operator <<(std::ostream &, const timeval &);

Expand Down