Encapsulate code converting std::chrono::duration to timeval #1915
Conversation
| struct timeval tv; | ||
| tv.tv_sec = std::chrono::seconds(PingerTimeout).count(); | ||
| tv.tv_usec = 0; | ||
| auto tv = ToTimeval(PingerTimeout); |
There was a problem hiding this comment.
This tv cannot be constant because it is fed to select(2) that requires a non-constant timeval timeout parameter.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The new converter handles
tv_usecwhich adds a performance regression calculating it to be0on 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).
| 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()); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
The
doMseccases should all be replaced by an output printingchrono::durationinstead.
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.
| struct timeval tv; | ||
| tv.tv_sec = std::chrono::seconds(PingerTimeout).count(); | ||
| tv.tv_usec = 0; | ||
| auto tv = ToTimeval(PingerTimeout); |
There was a problem hiding this comment.
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.
| 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()); |
There was a problem hiding this comment.
The
doMseccases should all be replaced by an output printingchrono::durationinstead.
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.
| struct timeval tv; | ||
| tv.tv_sec = std::chrono::seconds(PingerTimeout).count(); | ||
| tv.tv_usec = 0; | ||
| auto tv = ToTimeval(PingerTimeout); |
There was a problem hiding this comment.
The new converter handles
tv_usecwhich adds a performance regression calculating it to be0on 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).
rousskov
added
the
S-waiting-for-reviewer
Old and new code snippets yield the same optimized assembly: https://godbolt.org/z/4rr3WPc9e Old and new getCurrentTime() have nearly identical optimized assembly (with minor differences due to current_time being a global that new getCurrentTime() is still using to compute the other two globals). Both assembly version have eight memory accesses: https://godbolt.org/z/E9hc48G1K The number of assembly operations can be reduced further by not using current_time global to compute current_dtime and squid_curtime: https://godbolt.org/z/4rr3WPc9e
While working on something else, I accidentally found one more place that converts std::chrono::duration to timeval by hand and updated it in PR branch commit 4e7a5ab. |
No description provided.