DATAREDIS-698 - First round of polishing.

Eagerly initialize known commands. Preallocate Jedis response builder. Replace list concatenation with array copy. Reject null arguments in execute() Visibility modifiers, Javadoc.

Author: mp911de

Diff for: 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, new byte[0][]);
 	}
 
 	/*

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

@@ -29,26 +29,33 @@
 
 import java.lang.reflect.Field;
 import java.lang.reflect.Method;
-import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
-import java.util.List;
+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 invoke arbitraty 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 +67,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,24 +82,64 @@ 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
+	/**
+	 * 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}.
+	 */
+	@Nullable // TODO: Change keys and args to byte[][]?
 	static <T> T execute(String command, Collection<byte[]> keys, Collection<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()][]));
+		sendCommand(client, command, commandArgs);
 
 		return (T) client.getOne();
 	}
 
-	static Client retrieveClient(Jedis jedis) {
-		return (Client) ReflectionUtils.getField(CLIENT_FIELD, jedis);
+	private static byte[][] getCommandArguments(Collection<byte[]> keys, Collection<byte[]> args) {
+
+		byte[][] commandArgs = new byte[keys.size() + args.size()][];
+
+		int index = 0;
+		for (byte[] key : keys) {
+			commandArgs[index++] = key;
+		}
+
+		for (byte[] arg : args) {
+			commandArgs[index++] = arg;
+		}
+
+		return commandArgs;
 	}
 
+	/**
+	 * Send a Redis command and retrieve the {@link Client} for response retrieval.
+	 * 
+	 * @param jedis
+	 * @param command
+	 * @param args
+	 * @return
+	 */
 	static Client sendCommand(Jedis jedis, String command, byte[][] args) {
 
 		Client client = retrieveClient(jedis);
@@ -106,7 +153,7 @@ static Client sendCommand(Jedis jedis, String command, byte[][] 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 +162,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 +173,27 @@ 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);
-	}
-
-	static boolean isKnownCommand(String command) {
-
-		try {
-			Command.valueOf(command);
-			return true;
-		} catch (IllegalArgumentException e) {
-			return false;
-		}
+		dfa.setPropertyValue("pipelinedCommands", pipelinedCommands + 1);
 	}
 
+	/**
+	 * @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;
-			}
+	private static boolean isKnownCommand(String command) {
+		return KNOWN_COMMANDS.contains(command);
+	}
 
-			public String toString() {
-				return "Object";
-			}
-		});
+	@SuppressWarnings({ "unchecked", "ConstantConditions" })
+	static Response<Object> getGetResponse(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);
+	}
 }

Diff for: src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java

@@ -42,7 +42,6 @@
 import org.springframework.lang.Nullable;
 import org.springframework.util.Assert;
 import org.springframework.util.CollectionUtils;
-import org.springframework.util.ObjectUtils;
 import org.springframework.util.StringUtils;
 
 /**
@@ -273,20 +272,18 @@ public RedisHyperLogLogCommands hyperLogLogCommands() {
 	 */
 	@Override
 	public Object execute(String command, byte[]... args) {
-		Assert.hasText(command, "a valid command needs to be specified");
+
+		Assert.hasText(command, "A valid command needs to be specified!");
+		Assert.notNull(args, "Arguments must not be null!");
+
 		try {
-			List<byte[]> mArgs = new ArrayList<>();
-			if (!ObjectUtils.isEmpty(args)) {
-				Collections.addAll(mArgs, args);
-			}
 
-			Client client = JedisClientUtils.sendCommand(this.jedis, command, mArgs.toArray(new byte[mArgs.size()][]));
+			Client client = JedisClientUtils.sendCommand(this.jedis, command, args);
 
 			if (isQueueing() || isPipelined()) {
-				Object target = (isPipelined() ? pipeline : transaction);
-				@SuppressWarnings("unchecked")
 
-				Response<Object> result = JedisClientUtils.getGetResponse(target);
+				Response<Object> result = JedisClientUtils
+						.getGetResponse(isPipelined() ? getRequiredPipeline() : getRequiredTransaction());
 				if (isPipelined()) {
 					pipeline(new JedisResult(result));
 				} else {