smiklosovic commented on code in PR #4785:
URL: https://github.com/apache/cassandra/pull/4785#discussion_r3187271681


##########
src/java/org/apache/cassandra/cql3/statements/SelectStatement.java:
##########
@@ -815,11 +814,21 @@ private ReadQuery getSliceCommands(QueryOptions options, 
ClientState state, Colu
         if (filter == null || filter.isEmpty(table.comparator))
             return ReadQuery.empty(table);
 
-        List<DecoratedKey> decoratedKeys = new ArrayList<>(keys.size());
-        for (ByteBuffer key : keys)
+        List<DecoratedKey> decoratedKeys;
+        if (keys.size() == 1) // reduce allocations in collections for keys
         {
+            ByteBuffer key = keys.get(0);
             QueryProcessor.validateKey(key);
-            
decoratedKeys.add(table.partitioner.decorateKey(ByteBufferUtil.clone(key)));
+            decoratedKeys = 
Collections.singletonList(table.partitioner.decorateKey(key));
+        }
+        else
+        {
+            decoratedKeys = new ArrayList<>(keys.size());
+            for (ByteBuffer key : keys)
+            {
+                QueryProcessor.validateKey(key);

Review Comment:
   What is some key is invalid? Then we would allocate 
`ArrayList<>(keys.size())` unnecessarily because validateKey() throws and this 
whole method fails.
   
   What about
   
       for (ByteBuffer key : keys)
           QueryProcessor.validateKey(key);
   
       decoratedKeys = new ArrayList<>(keys.size());
   
       for (ByteBuffer key : keys)
           decoratedKeys.add(table.partitioner.decorateKey(key));
   
   This would be 2 iterator allocations and 1 decoratedKeys allocation when all 
keys are valid versus current 1 decorated keys validation every time and 1 
iterator allocation even when some key is invalid.
   
   I think that your approach is better if there is bigger probability that all 
keys are valid which I think is true.
   
   Maybe some dynamic resizing of that decoratedKeys list would be possible 
too? Like we would optimistically created e.g. half of keys.size() at first and 
then we would inflate it more ... dunno. I think it might be kept as you did it 
now.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]


---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to