DATAREDIS-698 - Polishing.

Eagerly initialize known commands. Preallocate Jedis response builder. Replace list concatenation with array copy. Reject null arguments in execute(). Remove inversion through collections with direct byte array creation. Reorder arguments in signatures, visibility modifiers, Javadoc.

Original pull request: #283.

Author: mp911de

src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java

@@ -3047,7 +3047,7 @@ public void openPipeline() {
 	 */
 	@Override
 	public Object execute(String command) {
-		return execute(command, (byte[][]) null);
+		return execute(command, EMPTY_2D_BYTE_ARRAY);
 	}
 
 	/*

src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java

@@ -15,7 +15,6 @@
  */
 package org.springframework.data.redis.connection;
 
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.List;
 import java.util.Properties;
@@ -142,16 +141,22 @@ default List<RedisClientInfo> getClientList(RedisClusterNode node) {
 	 */
 	@Nullable
 	@Override
+	@SuppressWarnings("unchecked")
 	default <T> T execute(String command, byte[] key, Collection<byte[]> args) {
 
 		Assert.notNull(command, "Command must not be null!");
 		Assert.notNull(key, "Key must not be null!");
 		Assert.notNull(args, "Args must not be null!");
 
-		ArrayList<byte[]> allArgs = new ArrayList();
-		allArgs.add(key);
-		allArgs.addAll(args);
+		byte[][] commandArgs = new byte[args.size() + 1][];
 
-		return (T) execute(command, allArgs.toArray(new byte[allArgs.size()][]));
+		commandArgs[0] = key;
+		int targetIndex = 1;
+
+		for (byte[] binaryArgument : args) {
+			commandArgs[targetIndex++] = binaryArgument;
+		}
+
+		return (T) execute(command, commandArgs);
 	}
 }

src/main/java/org/springframework/data/redis/connection/ReactiveHashCommands.java

@@ -623,17 +623,16 @@ public HStrLenCommand from(ByteBuffer key) {
 		}
 
 		/**
-		 * @return {@literal null} if not already set.
+		 * @return the field.
 		 */
-		@Nullable
 		public ByteBuffer getField() {
 			return field;
 		}
 	}
 
 	/**
-	 * Get the length of the value associated with {@code hashKey}. If either the {@code key} or the {@code hashKey} do
-	 * not exist, {@code 0} is emitted.
+	 * Get the length of the value associated with {@code field}. If either the {@code key} or the {@code field} do not
+	 * exist, {@code 0} is emitted.
 	 *
 	 * @param key must not be {@literal null}.
 	 * @param field must not be {@literal null}.
@@ -649,8 +648,8 @@ default Mono<Long> hStrLen(ByteBuffer key, ByteBuffer field) {
 	}
 
 	/**
-	 * Get the length of the value associated with {@code hashKey}. If either the {@code key} or the {@code hashKey} do
-	 * not exist, {@code 0} is emitted.
+	 * Get the length of the value associated with {@code field}. If either the {@code key} or the {@code field} do not
+	 * exist, {@code 0} is emitted.
 	 *
 	 * @param commands must not be {@literal null}.
 	 * @return never {@literal null}.

src/main/java/org/springframework/data/redis/connection/RedisClusterConnection.java

@@ -65,7 +65,7 @@ public interface RedisClusterConnection extends RedisConnection, RedisClusterCom
 	 * <pre>
 	 * <code>
 	 * // SET foo bar EX 10 NX
-	 * execute("SET", "foo".getBytes(), asBinaryList("bar", "EX", 10, "NX")
+	 * execute("SET", "foo".getBytes(), asBinaryList("bar", "EX", 10, "NX"))
 	 * </code>
 	 * </pre>
 	 *

src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java

@@ -29,26 +29,31 @@
 
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
-import java.util.Collection;
-import java.util.List;
+import java.util.Arrays;
+import java.util.Set;
 import java.util.function.Supplier;
+import java.util.stream.Collectors;
 
 import org.springframework.beans.DirectFieldAccessor;
-import org.springframework.lang.Nullable;
 import org.springframework.util.ClassUtils;
 import org.springframework.util.ReflectionUtils;
 
 /**
+ * Utility class to dispatch arbitrary Redis commands using Jedis commands.
+ *
  * @author Christoph Strobl
+ * @author Mark Paluch
  * @since 2.1
  */
+@SuppressWarnings({ "unchecked", "ConstantConditions" })
 class JedisClientUtils {
 
 	private static final Field CLIENT_FIELD;
 	private static final Method SEND_COMMAND;
 	private static final Method GET_RESPONSE;
 	private static final Method PROTOCOL_SEND_COMMAND;
+	private static final Set<String> KNOWN_COMMANDS;
+	private static final Builder<Object> OBJECT_BUILDER;
 
 	static {
 
@@ -60,12 +65,12 @@ class JedisClientUtils {
 		ReflectionUtils.makeAccessible(PROTOCOL_SEND_COMMAND);
 
 		try {
+
 			Class<?> commandType = ClassUtils.isPresent("redis.clients.jedis.ProtocolCommand", null)
 					? ClassUtils.forName("redis.clients.jedis.ProtocolCommand", null)
 					: ClassUtils.forName("redis.clients.jedis.Protocol$Command", null);
 
-			SEND_COMMAND = ReflectionUtils.findMethod(Connection.class, "sendCommand",
-					new Class[] { commandType, byte[][].class });
+			SEND_COMMAND = ReflectionUtils.findMethod(Connection.class, "sendCommand", commandType, byte[][].class);
 		} catch (Exception e) {
 			throw new NoClassDefFoundError(
 					"Could not find required flavor of command required by 'redis.clients.jedis.Connection#sendCommand'.");
@@ -75,38 +80,56 @@ class JedisClientUtils {
 
 		GET_RESPONSE = ReflectionUtils.findMethod(Queable.class, "getResponse", Builder.class);
 		ReflectionUtils.makeAccessible(GET_RESPONSE);
+
+		KNOWN_COMMANDS = Arrays.stream(Command.values()).map(Enum::name).collect(Collectors.toSet());
+
+		OBJECT_BUILDER = new Builder<Object>() {
+			public Object build(Object data) {
+				return data;
+			}
+
+			public String toString() {
+				return "Object";
+			}
+		};
 	}
 
-	@Nullable
-	static <T> T execute(String command, Collection<byte[]> keys, Collection<byte[]> args, Supplier<Jedis> jedis) {
+	/**
+	 * Execute an arbitrary on the supplied {@link Jedis} instance.
+	 *
+	 * @param command the command.
+	 * @param keys must not be {@literal null}, may be empty.
+	 * @param args must not be {@literal null}, may be empty.
+	 * @param jedis must not be {@literal null}.
+	 * @return the response, can be be {@literal null}.
+	 */
+	static <T> T execute(String command, byte[][] keys, byte[][] args, Supplier<Jedis> jedis) {
 
-		List<byte[]> mArgs = new ArrayList<>(keys);
-		mArgs.addAll(args);
+		byte[][] commandArgs = getCommandArguments(keys, args);
 
-		Client client = retrieveClient(jedis.get());
-		sendCommand(client, command, mArgs.toArray(new byte[mArgs.size()][]));
+		Client client = sendCommand(command, commandArgs, jedis.get());
 
 		return (T) client.getOne();
 	}
 
-	static Client retrieveClient(Jedis jedis) {
-		return (Client) ReflectionUtils.getField(CLIENT_FIELD, jedis);
-	}
-
-	static Client sendCommand(Jedis jedis, String command, byte[][] args) {
+	/**
+	 * Send a Redis command and retrieve the {@link Client} for response retrieval.
+	 *
+	 * @param command the command.
+	 * @param args must not be {@literal null}, may be empty.
+	 * @param jedis must not be {@literal null}.
+	 * @return the {@link Client} instance used to send the command.
+	 */
+	static Client sendCommand(String command, byte[][] args, Jedis jedis) {
 
 		Client client = retrieveClient(jedis);
 
-		if (isKnownCommand(command)) {
-			ReflectionUtils.invokeMethod(SEND_COMMAND, client, Command.valueOf(command.trim().toUpperCase()), args);
-		} else {
-			sendProtocolCommand(client, command, args);
-		}
+		sendCommand(client, command, args);
 
 		return client;
 	}
 
-	static void sendCommand(Client client, String command, byte[][] args) {
+	private static void sendCommand(Client client, String command, byte[][] args) {
 
 		if (isKnownCommand(command)) {
 			ReflectionUtils.invokeMethod(SEND_COMMAND, client, Command.valueOf(command.trim().toUpperCase()), args);
@@ -115,8 +138,9 @@ static void sendCommand(Client client, String command, byte[][] args) {
 		}
 	}
 
-	static void sendProtocolCommand(Client client, String command, byte[][] args) {
+	private static void sendProtocolCommand(Client client, String command, byte[][] args) {
 
+		// quite expensive to construct for each command invocation
 		DirectFieldAccessor dfa = new DirectFieldAccessor(client);
 
 		client.connect();
@@ -125,34 +149,52 @@ static void sendProtocolCommand(Client client, String command, byte[][] args) {
 		ReflectionUtils.invokeMethod(PROTOCOL_SEND_COMMAND, null, os, SafeEncoder.encode(command), args);
 
 		Integer pipelinedCommands = (Integer) dfa.getPropertyValue("pipelinedCommands");
-		dfa.setPropertyValue("pipelinedCommands", pipelinedCommands.intValue() + 1);
+		dfa.setPropertyValue("pipelinedCommands", pipelinedCommands + 1);
 	}
 
-	static boolean isKnownCommand(String command) {
+	private static boolean isKnownCommand(String command) {
+		return KNOWN_COMMANDS.contains(command);
+	}
 
-		try {
-			Command.valueOf(command);
-			return true;
-		} catch (IllegalArgumentException e) {
-			return false;
+	private static byte[][] getCommandArguments(byte[][] keys, byte[][] args) {
+
+		if (keys.length == 0) {
+			return args;
+		}
+
+		if (args.length == 0) {
+			return keys;
 		}
+
+		byte[][] commandArgs = new byte[keys.length + args.length][];
+
+		System.arraycopy(keys, 0, commandArgs, 0, keys.length);
+		System.arraycopy(args, 0, commandArgs, keys.length, args.length);
+
+		return commandArgs;
 	}
 
+	/**
+	 * @param jedis the client instance.
+	 * @return {@literal true} if the connection has entered {@literal MULTI} state.
+	 */
 	static boolean isInMulti(Jedis jedis) {
 		return retrieveClient(jedis).isInMulti();
 	}
 
-	static Response<Object> getGetResponse(Object target) {
-
-		return (Response<Object>) ReflectionUtils.invokeMethod(GET_RESPONSE, target, new Builder<Object>() {
-			public Object build(Object data) {
-				return data;
-			}
-
-			public String toString() {
-				return "Object";
-			}
-		});
+	/**
+	 * Retrieve the {@link Response} object from a {@link redis.clients.jedis.Transaction} or a
+	 * {@link redis.clients.jedis.Pipeline} for response synchronization.
+	 *
+	 * @param target a {@link redis.clients.jedis.Transaction} or {@link redis.clients.jedis.Pipeline}, must not be
+	 *          {@literal null}.
+	 * @return the {@link Response} wrapper object.
+	 */
+	static Response<Object> getResponse(Object target) {
+		return (Response<Object>) ReflectionUtils.invokeMethod(GET_RESPONSE, target, OBJECT_BUILDER);
 	}
 
+	private static Client retrieveClient(Jedis jedis) {
+		return (Client) ReflectionUtils.getField(CLIENT_FIELD, jedis);
+	}
 }

src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java

@@ -23,10 +23,7 @@
 import redis.clients.jedis.JedisClusterConnectionHandler;
 import redis.clients.jedis.JedisPool;
 
-import java.util.ArrayList;
-import java.util.Arrays;
 import java.util.Collection;
-import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.LinkedHashSet;
 import java.util.List;
@@ -70,6 +67,8 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection {
 	private static final ExceptionTranslationStrategy EXCEPTION_TRANSLATION = new FallbackExceptionTranslationStrategy(
 			JedisConverters.exceptionConverter());
 
+	private static final byte[][] EMPTY_2D_BYTE_ARRAY = new byte[0][];
+
 	private final Log log = LogFactory.getLog(getClass());
 
 	private final JedisCluster cluster;
@@ -130,14 +129,16 @@ public JedisClusterConnection(JedisCluster cluster, ClusterCommandExecutor execu
 	 * (non-Javadoc)
 	 * @see org.springframework.data.redis.connection.RedisCommands#execute(java.lang.String, byte[][])
 	 */
+	@Nullable
 	@Override
 	public Object execute(String command, byte[]... args) {
 
 		Assert.notNull(command, "Command must not be null!");
+		Assert.notNull(args, "Args must not be null!");
 
 		return clusterCommandExecutor
 				.executeCommandOnArbitraryNode((JedisClusterCommandCallback<Object>) client -> JedisClientUtils.execute(command,
-						Collections.emptyList(), Arrays.asList(args), () -> client))
+						EMPTY_2D_BYTE_ARRAY, args, () -> client))
 				.getValue();
 	}
 
@@ -153,14 +154,26 @@ public <T> T execute(String command, byte[] key, Collection<byte[]> args) {
 		Assert.notNull(key, "Key must not be null!");
 		Assert.notNull(args, "Args must not be null!");
 
-		Collection<byte[]> commandArgs = new ArrayList<>();
-		commandArgs.add(key);
-		commandArgs.addAll(args);
+		byte[][] commandArgs = getCommandArguments(key, args);
 
 		RedisClusterNode keyMaster = topologyProvider.getTopology().getKeyServingMasterNode(key);
 
 		return clusterCommandExecutor.executeCommandOnSingleNode((JedisClusterCommandCallback<T>) client -> JedisClientUtils
-				.execute(command, Collections.emptyList(), commandArgs, () -> client), keyMaster).getValue();
+				.execute(command, EMPTY_2D_BYTE_ARRAY, commandArgs, () -> client), keyMaster).getValue();
+	}
+
+	private static byte[][] getCommandArguments(byte[] key, Collection<byte[]> args) {
+
+		byte[][] commandArgs = new byte[args.size() + 1][];
+
+		commandArgs[0] = key;
+		int targetIndex = 1;
+
+		for (byte[] binaryArgument : args) {
+			commandArgs[targetIndex++] = binaryArgument;
+		}
+
+		return commandArgs;
 	}
 
 	/*
@@ -816,7 +829,8 @@ static class JedisClusterNodeResourceProvider implements ClusterNodeResourceProv
 
 				PropertyAccessor accessor = new DirectFieldAccessFallbackBeanWrapper(cluster);
 				this.connectionHandler = accessor.isReadableProperty("connectionHandler")
-						? (JedisClusterConnectionHandler) accessor.getPropertyValue("connectionHandler") : null;
+						? (JedisClusterConnectionHandler) accessor.getPropertyValue("connectionHandler")
+						: null;
 			} else {
 				this.connectionHandler = null;
 			}