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

Maintenance: improved stat5minClientRequests() naming #1951

Closed
wants to merge 5 commits into from
Closed

Maintenance: improved stat5minClientRequests() naming #1951

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
9145a8f
Renamed stat5minClientRequests() to reflect its meaning
eduard-bagdasaryan Nov 19, 2024
90aa746
Polished and added a comment
eduard-bagdasaryan Nov 20, 2024
b50d546
Let statSawRecentRequests() give requests number on startup
eduard-bagdasaryan Nov 21, 2024
f936f71
Merged from master
eduard-bagdasaryan Nov 21, 2024
e2478f7
No 'static' for an integer constant
eduard-bagdasaryan Nov 21, 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
2 changes: 1 addition & 1 deletion src/neighbors.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1159,7 +1159,7 @@ peerScheduleDnsRefreshCheck(const double delayInSeconds)
static void
peerDnsRefreshCheck(void *)
{
if (!stat5minClientRequests()) {
if (!statSawRecentRequests()) {
/* no recent client traffic, wait a bit */
peerScheduleDnsRefreshCheck(180.0);
return;
Expand Down
14 changes: 10 additions & 4 deletions src/stat.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1684,11 +1684,17 @@ snmpStatGet(int minutes)
return &CountHist[minutes];
}

int
stat5minClientRequests(void)
bool
statSawRecentRequests()
{
assert(N_COUNT_HIST > 5);
return statCounter.client_http.requests - CountHist[5].client_http.requests;
const auto recentMinutes = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

This use of auto only works because the default compilers assume for C++99 integer literals matches the current type of NCountHist. When the type of that counter is altered to be a correct counting type (ie size_t) this will break.

For now you may as well use:

Suggested change
const auto recentMinutes = 5;
const int recentMinutes = 5;

Or, if you want to be pedantic about future-safe code:

Suggested change
const auto recentMinutes = 5;
const decltype(NCountHist) recentMinutes = 5;

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 use of auto only works because

I performed a simple experiment, varying types of the second variable (NCountHist) from signed to unsigned and it compiled OK.

Copy link
Contributor

Choose a reason for hiding this comment

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

Amos: This use of auto only works because the default compilers assume for C++99 integer literals matches the current type of NCountHist.

Compilers do not assume that in this case: They know that both types are signed int. And this use of auto should work after NCountHist becomes unsigned because compilers know that the corresponding/known small positive constant is not going to cause signed-vs-unsigned problems. I am guessing these constant-related checks were added to compilers to prevent a flood of false warnings because many use cases use a combination of size_t and a small signed constant.

Amos: When the type of that counter is altered to be a correct counting type (ie size_t) this will break.

Eduard: I performed a simple experiment, varying types of the second variable (NCountHist) from signed to unsigned and it compiled OK.

It sounds like the assertion behind this change request has been proven false. That outcome matches my expectations and test cases as well. After all, if comparison with signed integer literals and equivalent named constants would generate warnings, then even basic expressions like size > 0 would generate warnings!

@yadij, if you believe that this auto will cause problems that were not exposed by changing NCountHist to an unsigned integer type, please detail your concerns.

Amos: For now you may as well use:

Suggested change
const auto recentMinutes = 5;
const int recentMinutes = 5;

I do not see enough reasons to deviate from AAA style here.

Or, if you want to be pedantic about future-safe code:

Suggested change
const auto recentMinutes = 5;
const decltype(NCountHist) recentMinutes = 5;

To avoid such complications, we routinely assume size_t is the right type for counting things in containers. I think our current approach is the best approach for cases where explicit type is needed (e.g., index variables in classic for loops). In this particular case, an explicit type is not needed, and I do not see enough reasons to complicate things by violating AAA.

BTW, should we need to force an unsigned type in a similar-but-different use case somewhere, we can (and, hence, should) still follow AAA style with:

Suggested change
const auto recentMinutes = 5;
const auto recentMinutes = 5U;

Again, this kind of unsigned constant value enforcement should only be done in rare cases where code would not compile without it -- we do not want to start sprinkling code with Us and such!

Copy link
Contributor

@yadij yadij Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Amos: This use of auto only works because the default compilers assume for C++99 integer literals matches the current type of NCountHist.

Compilers do not assume that in this case: They know that both types are signed int.

A quick test of your statements shows reality:

    size_t A = 0;
    const auto B = 5;
    const auto C = 5U;

    std::cout << "A : { size: " << sizeof(A) << ", type: " << typeid(A).name() << "}\n";
    std::cout << "B : { size: " << sizeof(B) << ", type: " << typeid(B).name() << "}\n";
    std::cout << "C : { size: " << sizeof(C) << ", type: " << typeid(C).name() << "}\n";

output:

A : { size: 8, type: m, value: 0}
B : { size: 4, type: i, value: 5}
C : { size: 4, type: j, value: 5}

And this use of auto should work after NCountHist becomes unsigned because compilers know that the corresponding/known small positive constant is not going to cause signed-vs-unsigned problems. I am guessing these constant-related checks were added to compilers to prevent a flood of false warnings because many use cases use a combination of size_t and a small signed constant.

So we are both "guessing" here (about why this PR code builds). What I do know is that conversion of other counters has recently produced compile errors about "signed vs unsigned" when using auto for constants that are compared to size_t in conditionals.

Copy link
Contributor

@rousskov rousskov Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Amos: This use of auto only works because the default compilers assume for C++99 integer literals matches the current type of NCountHist.

Alex: Compilers do not assume that in this case: They know that both types are signed int.

Amos: A quick test of your statements shows reality:

    size_t A = 0;
    const auto B = 5;
    const auto C = 5U;

    std::cout << "A : { size: " << sizeof(A) << ", type: " << typeid(A).name() << "}\n";
    std::cout << "B : { size: " << sizeof(B) << ", type: " << typeid(B).name() << "}\n";
    std::cout << "C : { size: " << sizeof(C) << ", type: " << typeid(C).name() << "}\n";

output:

A : { size: 8, type: m, value: 0}
B : { size: 4, type: i, value: 5}
C : { size: 4, type: j, value: 5}

AFAICT, the above test results do not contradict my quoted statement: They show that B type is signed int (the same type as NCountHist type).

Alex: And this use of auto should work after NCountHist becomes unsigned because compilers know that the corresponding/known small positive constant is not going to cause signed-vs-unsigned problems. I am guessing these constant-related checks were added to compilers to prevent a flood of false warnings because many use cases use a combination of size_t and a small signed constant.

So we are both "guessing" here (about why this PR code builds)

I am not guessing about why PR code builds (where both types are the same) and about what the outcome of changing NCountHist type to size_t is going to be (Eduard and I have both independently tested that or similar cases). My guess was only about why compilers have chosen to invest extra effort in treating known integer constants in such contexts that way. That special treatment existence is not a guess -- it is a proven fact.

What I do know is that conversion of other counters has recently produced compile errors about "signed vs unsigned" when using auto for constants that are compared to size_t in conditionals.

There are cases where compilers will produce warnings, but (evidently) this is not one of them. I see no evidence that the alleged future problem is real, and I do not want us to start adding U suffixes (and/or AAA exceptions) in similar cases until there is evidence that such unpleasant actions are warranted.

I am happy to discuss the implications of other cases where compilers produce warnings.

assert(N_COUNT_HIST > recentMinutes);

// Math below computes the number of requests during the last 0-6 minutes.
// CountHist is based on "minutes passed since Squid start" periods. It cannot
// deliver precise info for "last N minutes", but we do not need to be precise.
const auto oldRequests = (NCountHist > recentMinutes) ? CountHist[recentMinutes].client_http.requests : 0;
return statCounter.client_http.requests - oldRequests;
}

static double
Expand Down
5 changes: 3 additions & 2 deletions src/stat.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,9 @@
void statInit(void);
double median_svc_get(int, int);
void pconnHistCount(int, int);
int stat5minClientRequests(void);
double stat5minCPUUsage(void);
/// whether we processed any incoming requests in the last few minutes
/// \sa ClientHttpRequest::updateCounters()
bool statSawRecentRequests();
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 function tracks the number of already processed/logged requests, instead of the number of received/incoming client requests. The time gap between receiving a request and logging it may be significant, especially when bad/stale cache_peer addresses lead to various retries and timeouts. In such situations, incoming requests would benefit from fresh cache_peer addresses, so the function should return true sooner, based on incoming (rather than processed) request counters.

double statRequestHitRatio(int minutes);
double statRequestHitMemoryRatio(int minutes);
double statRequestHitDiskRatio(int minutes);
Expand Down