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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

<groupId>org.springframework.data</groupId>
<artifactId>spring-data-redis</artifactId>
<version>3.2.0-SNAPSHOT</version>
<version>3.2.0-GH-2751-SNAPSHOT</version>

<name>Spring Data Redis</name>
<description>Spring Data module for Redis</description>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public class ClusterRedirectException extends DataRetrievalFailureException {
*/
public ClusterRedirectException(int slot, String targetHost, int targetPort, Throwable e) {

super(String.format("Redirect: slot %s to %s:%s.", slot, targetHost, targetPort), e);
super("Redirect: slot %s to %s:%s.".formatted(slot, targetHost, targetPort), e);

this.slot = slot;
this.host = targetHost;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,9 +382,8 @@ private void checkAndPotentiallyWaitUntilUnlocked(String name, RedisConnection c
// Re-interrupt current Thread to allow other participants to react.
Thread.currentThread().interrupt();

String message = String.format("Interrupted while waiting to unlock cache %s", name);

throw new PessimisticLockingFailureException(message, ex);
throw new PessimisticLockingFailureException("Interrupted while waiting to unlock cache %s"
.formatted(name), ex);
} finally {
this.statistics.incLockTime(name, System.nanoTime() - lockWaitTimeNs);
}
Expand Down
17 changes: 6 additions & 11 deletions src/main/java/org/springframework/data/redis/cache/RedisCache.java
Original file line number Diff line number Diff line change
Expand Up @@ -324,12 +324,9 @@ private Object processAndCheckValue(@Nullable Object value) {
Object cacheValue = preProcessCacheValue(value);

if (nullCacheValueIsNotAllowed(cacheValue)) {

String message = String.format("Cache '%s' does not allow 'null' values; Avoid storing null"
throw new IllegalArgumentException(("Cache '%s' does not allow 'null' values; Avoid storing null"
+ " via '@Cacheable(unless=\"#result == null\")' or configure RedisCache to allow 'null'"
+ " via RedisCacheConfiguration", getName());

throw new IllegalArgumentException(message);
+ " via RedisCacheConfiguration").formatted(getName()));
}

return cacheValue;
Expand Down Expand Up @@ -440,11 +437,9 @@ protected String convertKey(Object key) {
return key.toString();
}

String message = String.format("Cannot convert cache key %s to String; Please register a suitable Converter"
+ " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'",
source, key.getClass().getName());

throw new IllegalStateException(message);
throw new IllegalStateException(("Cannot convert cache key %s to String; Please register a suitable Converter"
+ " via 'RedisCacheConfiguration.configureKeyConverters(...)' or override '%s.toString()'")
.formatted(source, key.getClass().getName()));
}

private CompletableFuture<byte[]> retrieveValue(Object key) {
Expand Down Expand Up @@ -502,7 +497,7 @@ private String convertCollectionLikeOrMapKey(Object key, TypeDescriptor source)
return "[" + stringJoiner + "]";
}

throw new IllegalArgumentException(String.format("Cannot convert cache key [%s] to String", key));
throw new IllegalArgumentException("Cannot convert cache key [%s] to String".formatted(key));
}

private byte[] createAndConvertCacheKey(Object key) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -424,11 +424,9 @@ public void addCacheKeyConverter(Converter<?, String> cacheKeyConverter) {
public void configureKeyConverters(Consumer<ConverterRegistry> registryConsumer) {

if (!(getConversionService() instanceof ConverterRegistry)) {

String message = "'%s' returned by getConversionService() does not allow Converter registration;"
+ " Please make sure to provide a ConversionService that implements ConverterRegistry";

throw new IllegalStateException(String.format(message, getConversionService().getClass().getName()));
throw new IllegalStateException(("'%s' returned by getConversionService() does not allow Converter registration;"
+ " Please make sure to provide a ConversionService that implements ConverterRegistry")
.formatted(getConversionService().getClass().getName()));
}

registryConsumer.accept((ConverterRegistry) getConversionService());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -130,12 +130,9 @@ private <S, T> NodeResult<T> executeCommandOnSingleNode(ClusterCommandCallback<S
Assert.notNull(node, "RedisClusterNode must not be null");

if (redirectCount > this.maxRedirects) {

String message = String.format("Cannot follow Cluster Redirects over more than %s legs; "
+ "Consider increasing the number of redirects to follow; Current value is: %s.",
redirectCount, this.maxRedirects);

throw new TooManyClusterRedirectionsException(message);
throw new TooManyClusterRedirectionsException(("Cannot follow Cluster Redirects over more than %s legs;"
+ " Consider increasing the number of redirects to follow; Current value is: %s")
.formatted(redirectCount, this.maxRedirects));
}

RedisClusterNode nodeToUse = lookupNode(node);
Expand Down Expand Up @@ -178,7 +175,7 @@ private RedisClusterNode lookupNode(RedisClusterNode node) {
try {
return topologyProvider.getTopology().lookup(node);
} catch (ClusterStateFailureException ex) {
throw new IllegalArgumentException(String.format("Node %s is unknown to cluster", node), ex);
throw new IllegalArgumentException("Node %s is unknown to cluster".formatted(node), ex);
}
}

Expand Down Expand Up @@ -215,7 +212,7 @@ public <S, T> MultiNodeResult<T> executeCommandAsyncOnNodes(ClusterCommandCallba
try {
resolvedRedisClusterNodes.add(topology.lookup(node));
} catch (ClusterStateFailureException ex) {
throw new IllegalArgumentException(String.format("Node %s is unknown to cluster", node), ex);
throw new IllegalArgumentException("Node %s is unknown to cluster".formatted(node), ex);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,8 +140,8 @@ public RedisClusterNode getKeyServingMasterNode(byte[] key) {
}
}

throw new ClusterStateFailureException(
String.format("Could not find master node serving slot %s for key '%s',", slot, Arrays.toString(key)));
throw new ClusterStateFailureException("Could not find master node serving slot %s for key '%s',"
.formatted(slot, Arrays.toString(key)));
}

/**
Expand All @@ -160,8 +160,8 @@ public RedisClusterNode lookup(String host, int port) {
}
}

throw new ClusterStateFailureException(
String.format("Could not find node at %s:%s; Is your cluster info up to date", host, port));
throw new ClusterStateFailureException("Could not find node at %s:%d; Is your cluster info up to date"
.formatted(host, port));
}

/**
Expand All @@ -181,8 +181,8 @@ public RedisClusterNode lookup(String nodeId) {
}
}

throw new ClusterStateFailureException(
String.format("Could not find node at %s; Is your cluster info up to date", nodeId));
throw new ClusterStateFailureException("Could not find node at %s; Is your cluster info up to date"
.formatted(nodeId));
}

/**
Expand All @@ -209,8 +209,8 @@ public RedisClusterNode lookup(RedisClusterNode node) {
return lookup(node.getId());
}

throw new ClusterStateFailureException(
String.format("Could not find node at %s; Have you provided either host and port or the nodeId", node));
throw new ClusterStateFailureException(("Could not find node at %s;"
+ " Have you provided either host and port or the nodeId").formatted(node));
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,11 +102,11 @@ public static RedisNode fromString(String hostPortString) {
try {
port = Integer.parseInt(portString);
} catch (RuntimeException ignore) {
throw new IllegalArgumentException(String.format("Unparseable port number: %s", hostPortString));
throw new IllegalArgumentException("Unparseable port number: %s".formatted(hostPortString));
}

if (!isValidPort(port)) {
throw new IllegalArgumentException(String.format("Port number out of range: %s", hostPortString));
throw new IllegalArgumentException("Port number out of range: %s".formatted(hostPortString));
}

return new RedisNode(host, port);
Expand All @@ -122,28 +122,28 @@ public static RedisNode fromString(String hostPortString) {
private static String[] getHostAndPortFromBracketedHost(String hostPortString) {

if (hostPortString.charAt(0) != '[') {
throw new IllegalArgumentException(
String.format("Bracketed host-port string must start with a bracket: %s", hostPortString));
throw new IllegalArgumentException("Bracketed host-port string must start with a bracket: %s"
.formatted(hostPortString));
}

int colonIndex = hostPortString.indexOf(':');
int closeBracketIndex = hostPortString.lastIndexOf(']');

if (!(colonIndex > -1 && closeBracketIndex > colonIndex)) {
throw new IllegalArgumentException(String.format("Invalid bracketed host/port: %s", hostPortString));
throw new IllegalArgumentException("Invalid bracketed host/port: %s".formatted(hostPortString));
}

String host = hostPortString.substring(1, closeBracketIndex);
if (closeBracketIndex + 1 == hostPortString.length()) {
return new String[] { host, "" };
} else {
if (!(hostPortString.charAt(closeBracketIndex + 1) == ':')) {
throw new IllegalArgumentException(
String.format("Only a colon may follow a close bracket: %s", hostPortString));
throw new IllegalArgumentException("Only a colon may follow a close bracket: %s"
.formatted(hostPortString));
}
for (int i = closeBracketIndex + 2; i < hostPortString.length(); ++i) {
if (!Character.isDigit(hostPortString.charAt(i))) {
throw new IllegalArgumentException(String.format("Port must be numeric: %s", hostPortString));
throw new IllegalArgumentException("Port must be numeric: %s".formatted(hostPortString));
}
}
return new String[] { host, hostPortString.substring(closeBracketIndex + 2) };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ public Optional<char[]> toOptional() {

@Override
public String toString() {
return String.format("%s[%s]", getClass().getSimpleName(), isPresent() ? "*****" : "<none>");
return "%s[%s]".formatted(getClass().getSimpleName(), isPresent() ? "*****" : "<none>");
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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));

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.

Copy link
Contributor Author

@jxblum jxblum Nov 21, 2023

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.

Copy link
Contributor Author

@jxblum jxblum Nov 21, 2023

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.

Copy link
Contributor Author

@jxblum jxblum Nov 21, 2023

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.

Copy link
Contributor Author

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.

Assert.isTrue(index >= 0, "Invalid DB index '%d'; non-negative index required".formatted(index));

this.database = index;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public int getDatabase() {
@Override
public void setDatabase(int index) {

Assert.isTrue(index >= 0, () -> String.format("Invalid DB index '%s' (a positive index required)", index));
Assert.isTrue(index >= 0, () -> "Invalid DB index '%s'; non-negative index required".formatted(index));

this.database = index;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,8 @@ public RedisStandaloneConfiguration(String hostName) {
public RedisStandaloneConfiguration(String hostName, int port) {

Assert.hasText(hostName, "Host name must not be null or empty");
Assert.isTrue(port >= 1 && port <= 65535,
() -> String.format("Port %d must be a valid TCP port in the range between 1-65535", port));
Assert.isTrue(port >= 1 && port <= 65535, "Port %d must be a valid TCP port in the range between 1-65535"
.formatted(port));

this.hostName = hostName;
this.port = port;
Expand Down Expand Up @@ -103,7 +103,7 @@ public int getDatabase() {
@Override
public void setDatabase(int index) {

Assert.isTrue(index >= 0, () -> String.format("Invalid DB index '%s' (a positive index required)", index));
Assert.isTrue(index >= 0, "Invalid DB index '%d'; non-negative index required".formatted(index));

this.database = index;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public int getDatabase() {
@Override
public void setDatabase(int index) {

Assert.isTrue(index >= 0, () -> String.format("Invalid DB index '%s' (a positive index required)", index));
Assert.isTrue(index >= 0, "Invalid DB index '%d'; non-negative index required".formatted(index));

this.database = index;
this.nodes.forEach(it -> it.setDatabase(database));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ public static Object parse(Object source, String sourcePath, Map<String, Class<?
}

if (LOGGER.isDebugEnabled()) {
LOGGER.debug(String.format("parsing %s (%s) as %s", sourcePath, path, targetType));
LOGGER.debug("parsing %s (%s) as %s".formatted(sourcePath, path, targetType));
}

if (targetType == Object.class) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -761,7 +761,7 @@ public Jedis getResourceForSpecificNode(RedisClusterNode node) {
return new Jedis(connection);
}

throw new DataAccessResourceFailureException(String.format("Node %s is unknown to cluster", node));
throw new DataAccessResourceFailureException("Node %s is unknown to cluster".formatted(node));
}

private ConnectionPool getResourcePoolForSpecificNode(RedisClusterNode node) {
Expand All @@ -779,8 +779,8 @@ private Connection getConnectionForSpecificNode(RedisClusterNode node) {
RedisClusterNode member = topologyProvider.getTopology().lookup(node);

if (!member.hasValidHost()) {
throw new DataAccessResourceFailureException(String
.format("Cannot obtain connection to node %ss as it is not associated with a hostname", node.getId()));
throw new DataAccessResourceFailureException("Cannot obtain connection to node %ss; "
+ "it is not associated with a hostname".formatted(node.getId()));
}

if (member != null && connectionHandler != null) {
Expand Down Expand Up @@ -872,7 +872,7 @@ public ClusterTopology getTopology() {
StringBuilder stringBuilder = new StringBuilder();

for (Entry<String, Exception> entry : errors.entrySet()) {
stringBuilder.append(String.format("\r\n\t- %s failed: %s", entry.getKey(), entry.getValue().getMessage()));
stringBuilder.append("\r\n\t- %s failed: %s".formatted(entry.getKey(), entry.getValue().getMessage()));
}

throw new ClusterStateFailureException(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -347,10 +347,11 @@ public Long time(RedisClusterNode node, TimeUnit timeUnit) {
public void killClient(String host, int port) {

Assert.hasText(host, "Host for 'CLIENT KILL' must not be 'null' or 'empty'");
String hostAndPort = String.format("%s:%s", host, port);
String hostAndPort = "%s:%d".formatted(host, port);

connection.getClusterCommandExecutor()
.executeCommandOnAllNodes((JedisClusterCommandCallback<String>) client -> client.clientKill(hostAndPort));
JedisClusterCommandCallback<String> command = client -> client.clientKill(hostAndPort);

connection.getClusterCommandExecutor().executeCommandOnAllNodes(command);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -908,8 +908,8 @@ public Set<Tuple> zInterWithScores(Aggregate aggregate, Weights weights, byte[].

Assert.notNull(sets, "Sets must not be null");
Assert.noNullElements(sets, "Source sets must not contain null elements");
Assert.isTrue(weights.size() == sets.length, () -> String
.format("The number of weights (%d) must match the number of source sets (%d)", weights.size(), sets.length));
Assert.isTrue(weights.size() == sets.length, () ->
"The number of weights %d must match the number of source sets %d".formatted(weights.size(), sets.length));

if (ClusterSlotHashUtil.isSameSlotForAllKeys(sets)) {

Expand Down Expand Up @@ -951,8 +951,8 @@ public Long zInterStore(byte[] destKey, Aggregate aggregate, Weights weights, by
Assert.notNull(destKey, "Destination key must not be null");
Assert.notNull(sets, "Source sets must not be null");
Assert.noNullElements(sets, "Source sets must not contain null elements");
Assert.isTrue(weights.size() == sets.length, () -> String
.format("The number of weights (%d) must match the number of source sets (%d)", weights.size(), sets.length));
Assert.isTrue(weights.size() == sets.length,
"The number of weights %d must match the number of source sets %d".formatted(weights.size(), sets.length));

byte[][] allKeys = ByteUtils.mergeArrays(destKey, sets);

Expand Down Expand Up @@ -1008,8 +1008,8 @@ public Set<Tuple> zUnionWithScores(Aggregate aggregate, Weights weights, byte[].

Assert.notNull(sets, "Sets must not be null");
Assert.noNullElements(sets, "Source sets must not contain null elements");
Assert.isTrue(weights.size() == sets.length, () -> String
.format("The number of weights (%d) must match the number of source sets (%d)", weights.size(), sets.length));
Assert.isTrue(weights.size() == sets.length, () ->
"The number of weights %d must match the number of source sets %d".formatted(weights.size(), sets.length));

if (ClusterSlotHashUtil.isSameSlotForAllKeys(sets)) {

Expand Down Expand Up @@ -1052,8 +1052,8 @@ public Long zUnionStore(byte[] destKey, Aggregate aggregate, Weights weights, by
Assert.notNull(destKey, "Destination key must not be null");
Assert.notNull(sets, "Source sets must not be null");
Assert.noNullElements(sets, "Source sets must not contain null elements");
Assert.isTrue(weights.size() == sets.length, () -> String
.format("The number of weights (%d) must match the number of source sets (%d)", weights.size(), sets.length));
Assert.isTrue(weights.size() == sets.length,
"The number of weights %d must match the number of source sets %d".formatted(weights.size(), sets.length));

byte[][] allKeys = ByteUtils.mergeArrays(destKey, sets);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -952,7 +952,7 @@ private Jedis getActiveSentinel() {
return jedis;
}
} catch (Exception ex) {
log.warn(String.format("Ping failed for sentinel host: %s", node.getHost()), ex);
log.warn("Ping failed for sentinel host: %s".formatted(node.getHost()), ex);
} finally {
if (!success && jedis != null) {
jedis.close();
Expand Down Expand Up @@ -989,8 +989,8 @@ private int getConnectTimeout() {
private MutableJedisClientConfiguration getMutableConfiguration() {

Assert.state(clientConfiguration instanceof MutableJedisClientConfiguration,
() -> String.format("Client configuration must be instance of MutableJedisClientConfiguration but is %s",
ClassUtils.getShortName(clientConfiguration.getClass())));
() -> "Client configuration must be instance of MutableJedisClientConfiguration but is %s"
.formatted(ClassUtils.getShortName(clientConfiguration.getClass())));

return (MutableJedisClientConfiguration) clientConfiguration;
}
Expand All @@ -1005,10 +1005,10 @@ private void assertInitialized() {

switch (current) {
case CREATED, STOPPED -> throw new IllegalStateException(
String.format("JedisConnectionFactory has been %s. Use start() to initialize it", current));
"JedisConnectionFactory has been %s. Use start() to initialize it".formatted(current));
case DESTROYED -> throw new IllegalStateException(
"JedisConnectionFactory was destroyed and cannot be used anymore");
default -> throw new IllegalStateException(String.format("JedisConnectionFactory is %s", current));
default -> throw new IllegalStateException("JedisConnectionFactory is %s".formatted(current));
}
}

Expand Down