-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
Replace String.format(…, args)
with "…".formatted(args)
#2752
Conversation
@@ -223,7 +223,7 @@ public int getDatabase() { | |||
@Override | |||
public void setDatabase(int index) { | |||
|
|||
Assert.isTrue(index >= 0, () -> String.format("Invalid DB index '%d'; non-negative index required", index)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this is a good change.
Without a supplier, we are needlessly constructing a string.
This assertion is almost always true, so it should be optimised for a happy path.
The same applies to other similar changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the feedback. I agree.
I wrote a simple JMH Benchmark today comparing String.formatted(..)
with and without the use of a Supplier
to the assertion, such as isTrue(..)
above. It turns out that, without the Supplier
the invocation performance (throughput & average time) is much worse over a large number of iterations, even despite the added Object allocations for Suppliers
. I did not look at the Heap memory map, but I suspect it will create a bit more garbage than necessary, at least until JIT compiler optimizations kick in.
However, I will point out that, I was careful to only remove the Suppliers
in non-critical code paths, such as in this case... "Configuration".
Configuration is only really ever in invoked on Spring container startup, once, when the beans are constructed, configured and initialized.
In other cases, I believe I only removed the use of a Supplier
in tests.
I will re-review the changes to make sure and introduce any necessary Supplier
that may have been mistakenly removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget to mention, keep in mind that the Supplier
Lambda in this case is NOT a non-capturing Lambda since it uses arguments from the enclosing method. Therefore, it, too, results in Object construction every time the method is called (though RedisSentinelConfiguration.setDatabase(:int)
is only likely to be called once in this case).
Still, let's be clear here what we are talking about in terms of "optimizations".
You must always consider resource utilization when considering performance as often they are related. You must also measure. And finally, you must take into consideration the code path. Sacrificing readability, correctness or other factors for the sake of performance is usually not the best decision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In reviewing my changes for this PR, along with writing a JMH Benchmark, I definitely think the use of a Supplier
in assertions is necessary in SD Redis command classes, where the codepath will be hotter at runtime than it will be in configuration classes, for instance, such as here, as well as in similar places.
In other cases, such as the KeyspaceIdentifier and BinaryKeyspaceIdentifier, I introduced the use of a Supplier
inside the assertion where a Supplier
was not used before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have updated this PR to (re)introduce the use of the Supplier
inside assertions. Only very few were inside critical codepaths, like command classes (specifically Jedis and Lettuce ZSet commands). The rest were in configuration classes (non-critical codepaths).
It should be noted, that most of the String formatting occurs inside Exception construction (e.g. InvalidDataAccessApiUsageException
) and Logger statements. There is no option for using a Supplier
in most of these cases, not without extra guards; for example:
public void someMethod(Object... args) {
//...
logDebug(() ->
"Some formatted String with args [%s]".formatted(Arrays.toString(args));
}
private void logDebug(Supplier<String> logMessage) {
if (logger.isDebugEnabled()) {
logger.debug(logMessage.get());
}
}
This may make for some nice optimizations down the road (and in certain cases, I noticed I have introduced log helper methods, just like described above), but that is not a concern in this PR.
In conclusion, I still feel the appearance of using String.formatted(..)
is much nicer than Spring.format(..)
, as it calls attention to the Exception/Log/Whatever message immediately.
String.format("..", args)
with "..".formatted(args)
String.format(…, args)
with "…".formatted(args)
Thank you for your contribution. That's merged, polished, and backported now. |
See #2751.
Please read the description in #2751.
I also think formatted Strings work nicer if natural line breaks are preserved.
For example:
Rather than this non-sense:
That is orphans, or even partly broken argument lists.
Finally...
This is not strictly necessary. But, in my opinion, I think this is a nice improvement over our existing String formatting.
I also took the opportunity to clean up a (minimal) few compiler warnings while stumbling across message formatting.
Also, I changed a couple assertions that unnecessarily used a
Supplier
that did not need to and addedSuppliers
to a few assertions that should have had them.