From 8adea79e67b1ab55cfbeb12a8c215e8ed4bf21a2 Mon Sep 17 00:00:00 2001 From: Mark Paluch Date: Fri, 6 Oct 2017 13:52:45 +0200 Subject: [PATCH] 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. --- .../DefaultStringRedisConnection.java | 2 +- .../DefaultedRedisClusterConnection.java | 15 ++- .../connection/ReactiveHashCommands.java | 11 +- .../connection/RedisClusterConnection.java | 2 +- .../connection/jedis/JedisClientUtils.java | 126 ++++++++++++------ .../jedis/JedisClusterConnection.java | 32 +++-- .../connection/jedis/JedisConnection.java | 19 ++- .../lettuce/LettuceReactiveHashCommands.java | 4 +- .../DefaultStringRedisConnectionTests.java | 13 +- 9 files changed, 136 insertions(+), 88 deletions(-) diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java index f2724bc92..689ce22c8 100644 --- a/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/DefaultStringRedisConnection.java @@ -3047,7 +3047,7 @@ public class DefaultStringRedisConnection implements StringRedisConnection, Deco */ @Override public Object execute(String command) { - return execute(command, (byte[][]) null); + return execute(command, EMPTY_2D_BYTE_ARRAY); } /* diff --git a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java index fe210456e..7b06aebd2 100644 --- a/src/main/java/org/springframework/data/redis/connection/DefaultedRedisClusterConnection.java +++ b/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 @@ public interface DefaultedRedisClusterConnection extends RedisClusterConnection, */ @Nullable @Override + @SuppressWarnings("unchecked") default T execute(String command, byte[] key, Collection 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 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); } } diff --git a/src/main/java/org/springframework/data/redis/connection/ReactiveHashCommands.java b/src/main/java/org/springframework/data/redis/connection/ReactiveHashCommands.java index bf9a3ab61..a737afd0b 100644 --- a/src/main/java/org/springframework/data/redis/connection/ReactiveHashCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/ReactiveHashCommands.java @@ -623,17 +623,16 @@ public interface ReactiveHashCommands { } /** - * @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 @@ public interface ReactiveHashCommands { } /** - * 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}. diff --git a/src/main/java/org/springframework/data/redis/connection/RedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/RedisClusterConnection.java index 8bb79ef60..b690ac475 100644 --- a/src/main/java/org/springframework/data/redis/connection/RedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/RedisClusterConnection.java @@ -65,7 +65,7 @@ public interface RedisClusterConnection extends RedisConnection, RedisClusterCom * 
 	 * 
 	 * // SET foo bar EX 10 NX
-	 * execute("SET", "foo".getBytes(), asBinaryList("bar", "EX", 10, "NX")
+	 * execute("SET", "foo".getBytes(), asBinaryList("bar", "EX", 10, "NX"))
 	 * 
 	 *
* diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java index eb79a2607..3fa9537b9 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClientUtils.java @@ -29,26 +29,31 @@ import redis.clients.util.SafeEncoder; 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 KNOWN_COMMANDS; + private static final Builder 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() { + public Object build(Object data) { + return data; + } + + public String toString() { + return "Object"; + } + }; } - @Nullable - static T execute(String command, Collection keys, Collection args, Supplier 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 execute(String command, byte[][] keys, byte[][] args, Supplier jedis) { - List 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 @@ class JedisClientUtils { } } - 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 @@ class JedisClientUtils { 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 getGetResponse(Object target) { - - return (Response) ReflectionUtils.invokeMethod(GET_RESPONSE, target, new Builder() { - 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 getResponse(Object target) { + return (Response) ReflectionUtils.invokeMethod(GET_RESPONSE, target, OBJECT_BUILDER); } + private static Client retrieveClient(Jedis jedis) { + return (Client) ReflectionUtils.getField(CLIENT_FIELD, jedis); + } } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java index 3167a8ad9..40ac08398 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisClusterConnection.java @@ -23,10 +23,7 @@ import redis.clients.jedis.JedisCluster; 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 class JedisClusterConnection implements DefaultedRedisClusterConnection { * (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) client -> JedisClientUtils.execute(command, - Collections.emptyList(), Arrays.asList(args), () -> client)) + EMPTY_2D_BYTE_ARRAY, args, () -> client)) .getValue(); } @@ -153,14 +154,26 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection { Assert.notNull(key, "Key must not be null!"); Assert.notNull(args, "Args must not be null!"); - Collection commandArgs = new ArrayList<>(); - commandArgs.add(key); - commandArgs.addAll(args); + byte[][] commandArgs = getCommandArguments(key, args); RedisClusterNode keyMaster = topologyProvider.getTopology().getKeyServingMasterNode(key); return clusterCommandExecutor.executeCommandOnSingleNode((JedisClusterCommandCallback) 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 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 @@ public class JedisClusterConnection implements DefaultedRedisClusterConnection { PropertyAccessor accessor = new DirectFieldAccessFallbackBeanWrapper(cluster); this.connectionHandler = accessor.isReadableProperty("connectionHandler") - ? (JedisClusterConnectionHandler) accessor.getPropertyValue("connectionHandler") : null; + ? (JedisClusterConnectionHandler) accessor.getPropertyValue("connectionHandler") + : null; } else { this.connectionHandler = null; } diff --git a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java index 834f231ae..475d89af3 100644 --- a/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java +++ b/src/main/java/org/springframework/data/redis/connection/jedis/JedisConnection.java @@ -42,7 +42,6 @@ import org.springframework.data.redis.connection.convert.TransactionResultConver 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 class JedisConnection extends AbstractRedisConnection { */ @Override public Object execute(String command, byte[]... args) { - Assert.hasText(command, "a valid command needs to be specified"); - try { - List mArgs = new ArrayList<>(); - if (!ObjectUtils.isEmpty(args)) { - Collections.addAll(mArgs, args); - } - Client client = JedisClientUtils.sendCommand(this.jedis, command, mArgs.toArray(new byte[mArgs.size()][])); + Assert.hasText(command, "A valid command needs to be specified!"); + Assert.notNull(args, "Arguments must not be null!"); + + try { + + Client client = JedisClientUtils.sendCommand(command, args, this.jedis); if (isQueueing() || isPipelined()) { - Object target = (isPipelined() ? pipeline : transaction); - @SuppressWarnings("unchecked") - Response result = JedisClientUtils.getGetResponse(target); + Response result = JedisClientUtils + .getResponse(isPipelined() ? getRequiredPipeline() : getRequiredTransaction()); if (isPipelined()) { pipeline(new JedisResult(result)); } else { diff --git a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveHashCommands.java b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveHashCommands.java index 99827eafb..326b638a0 100644 --- a/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveHashCommands.java +++ b/src/main/java/org/springframework/data/redis/connection/lettuce/LettuceReactiveHashCommands.java @@ -224,8 +224,8 @@ class LettuceReactiveHashCommands implements ReactiveHashCommands { return connection.execute(cmd -> Flux.from(commands).flatMap(command -> { - Assert.notNull(command.getKey(), "Command.getKey() must not be null!"); - Assert.notNull(command.getField(), "Command.getField() must not be null!"); + Assert.notNull(command.getKey(), "Key must not be null!"); + Assert.notNull(command.getField(), "Field must not be null!"); return cmd.hstrlen(command.getKey(), command.getField()).map(value -> new NumericResponse<>(command, value)); })); diff --git a/src/test/java/org/springframework/data/redis/connection/DefaultStringRedisConnectionTests.java b/src/test/java/org/springframework/data/redis/connection/DefaultStringRedisConnectionTests.java index cd3a7c277..8b86c0d98 100644 --- a/src/test/java/org/springframework/data/redis/connection/DefaultStringRedisConnectionTests.java +++ b/src/test/java/org/springframework/data/redis/connection/DefaultStringRedisConnectionTests.java @@ -18,16 +18,7 @@ package org.springframework.data.redis.connection; import static org.junit.Assert.*; import static org.mockito.Mockito.*; -import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collections; -import java.util.HashMap; -import java.util.HashSet; -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Map; -import java.util.Properties; -import java.util.Set; +import java.util.*; import java.util.concurrent.TimeUnit; import org.junit.Before; @@ -1702,7 +1693,7 @@ public class DefaultStringRedisConnectionTests { @Test public void testExecute() { - doReturn("foo").when(nativeConnection).execute("something", (byte[][]) null); + doReturn("foo").when(nativeConnection).execute("something", new byte[0][]); actual.add(connection.execute("something")); verifyResults(Arrays.asList(new Object[] { "foo" })); }