Sorted set new commands: ZDIFF, ZDIFFSTORE, ZINTER, ZINTERCARD, and ZUNION #2075
Conversation
NickCraver
changed the title
|
@slorello89 I've touched this a lot, so would appreciate a third set of eyes if you get a chance! |
There was a problem hiding this comment.
@NickCraver just have some questions about the efficiency vs maintainability bits of the Message Creation.
|
|
||
| private Message GetSortedSetCombineCommandMessage(SetOperation operation, RedisKey[] keys, double[]? weights, Aggregate aggregate, bool withScores, CommandFlags flags) | ||
| { | ||
| var command = operation switch |
There was a problem hiding this comment.
Probably want to move this logic to the SetOperation enum as an extension method.
There was a problem hiding this comment.
👍 This makes sense with an arg - done!
| } | ||
|
|
||
| var i = 0; | ||
| var values = new RedisValue[1 + keys.Length + |
There was a problem hiding this comment.
How much more expensive would it be to initialize a List with this capacity and using Add/AddRange as opposed to an array and then having to track i, this is a bit. . . feels like we're reinventing the wheel here? List<T>(int) is effectively just doing the same initialization, since it's initialized with the correct capacity it shouldn't over allocate nor have to dynamically reallocate.
There was a problem hiding this comment.
It's allocate more than double. At the very lease we're resizing without sizes, but also: look inside Message to see a .ToArray() which is going to copy as well. I agree it's not awesome to do things manually, but 2x+ allocations aren't awesome either :)
| if (keys == null) throw new ArgumentNullException(nameof(keys)); | ||
|
|
||
| var i = 0; | ||
| var values = new RedisValue[1 + keys.Length + (limit > 0 ? 2 : 0)]; |
There was a problem hiding this comment.
Same comment as above.
|
@slorello89 made some tweaks (good suggestions!) - done! |
Covers:
ZDIFF
ZDIFFSTORE
ZINTER
ZINTERCARD
ZUNION
#2055