Maintenance: improved stat5minClientRequests() naming

[eduard-bagdasaryan]

stat5minClientRequests() was to meant to return the number of recent
client requests. However, the function did not provide implied 5 minute
precision. It returned, roughly speaking, the number of requests during
the last 0-6 minutes. The new, less strict function name and boolean
type avoid this false precision implication.

Also removed unused stat5minCPUUsage().

[eduard-bagdasaryan]

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.

[rousskov]

Thank you.

[yadij]

I do not insist on this change, but if not addressed here it will cause issues for somebody later.

[yadij]

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;

[eduard-bagdasaryan]

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.

[rousskov]

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!

[yadij]

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.

[rousskov]

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.