-
Notifications
You must be signed in to change notification settings - Fork 1.5k
BITFIELD and BITFIELD_RO feature #2107
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
68d6d79
1695639
125885a
5660713
bdb9128
7554d6d
5daca75
c0fe65d
3928113
b3c16c4
d3cc041
95eb393
9dae591
7c6f62f
5f07290
d87f66b
1392d20
a00aba5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
using System.Collections.Generic; | ||
|
||
namespace StackExchange.Redis; | ||
|
||
/// <summary> | ||
/// Builder for bitfield commands that take multiple sub-commands. | ||
/// </summary> | ||
public class BitfieldCommandBuilder | ||
{ | ||
private readonly LinkedList<RedisValue> _args = new LinkedList<RedisValue>(); | ||
private bool _eligibleForReadOnly; | ||
|
||
/// <summary> | ||
/// Builds a subcommand for a Bitfield GET, which returns the number stored in the specified offset of a bitfield at the given encoding. | ||
/// </summary> | ||
/// <param name="encoding">The encoding for the subcommand.</param> | ||
/// <param name="offset">The offset into the bitfield for the subcommand.</param> | ||
public BitfieldCommandBuilder Get(BitfieldEncoding encoding, BitfieldOffset offset) | ||
{ | ||
_eligibleForReadOnly = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. surely we can only ever set this to false, with it starting true? if we Get(...).Set(...).Get(...) we're not eligible for readonly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, definitely. |
||
_args.AddLast(RedisLiterals.GET); | ||
_args.AddLast(encoding.RedisValue); | ||
_args.AddLast(offset.RedisValue); | ||
return this; | ||
} | ||
|
||
/// <summary> | ||
/// Builds a Bitfield subcommand which SETs the specified range of bits to the specified value. | ||
/// </summary> | ||
/// <param name="encoding">The encoding of the subcommand.</param> | ||
/// <param name="offset">The offset of the subcommand.</param> | ||
/// <param name="value">The value to set.</param> | ||
public BitfieldCommandBuilder Set(BitfieldEncoding encoding, BitfieldOffset offset, long value) | ||
{ | ||
_eligibleForReadOnly = false; | ||
_args.AddLast(RedisLiterals.SET); | ||
_args.AddLast(encoding.RedisValue); | ||
_args.AddLast(offset.RedisValue); | ||
_args.AddLast(value); | ||
return this; | ||
} | ||
|
||
/// <summary> | ||
/// Builds a subcommand for Bitfield INCRBY, which increments the number at the specified range of bits by the provided value | ||
/// </summary> | ||
/// <param name="encoding">The number's encoding.</param> | ||
/// <param name="offset">The offset into the bitfield to increment.</param> | ||
/// <param name="increment">The value to increment by.</param> | ||
/// <param name="overflowHandling">How overflows will be handled when incrementing.</param> | ||
public BitfieldCommandBuilder Incrby(BitfieldEncoding encoding, BitfieldOffset offset, long increment, BitfieldOverflowHandling overflowHandling = BitfieldOverflowHandling.Wrap) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should be Increment for consistency with StringIncrement There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Makes sense. |
||
{ | ||
_eligibleForReadOnly = false; | ||
if (overflowHandling != BitfieldOverflowHandling.Wrap) | ||
{ | ||
_args.AddLast(RedisLiterals.OVERFLOW); | ||
_args.AddLast(overflowHandling.AsRedisValue()); | ||
} | ||
|
||
_args.AddLast(RedisLiterals.INCRBY); | ||
_args.AddLast(encoding.RedisValue); | ||
_args.AddLast(offset.RedisValue); | ||
_args.AddLast(increment); | ||
return this; | ||
} | ||
|
||
internal BitfieldCommandMessage Build(int db, RedisKey key, CommandFlags flags, RedisBase redisBase, out ServerEndPoint? server) | ||
{ | ||
var features = redisBase.GetFeatures(key, flags, out server); | ||
var command = _eligibleForReadOnly && features.ReadOnlyBitfield ? RedisCommand.BITFIELD_RO : RedisCommand.BITFIELD; | ||
return new BitfieldCommandMessage(db, flags, key, command, _args); | ||
} | ||
} | ||
|
||
internal class BitfieldCommandMessage : Message | ||
{ | ||
private readonly LinkedList<RedisValue> _args; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto List, although I think if possible we should skip an alloc here and go straight to an array; for now I'd settle for List-T, though |
||
private readonly RedisKey _key; | ||
public BitfieldCommandMessage(int db, CommandFlags flags, RedisKey key, RedisCommand command, LinkedList<RedisValue> args) : base(db, flags, command) | ||
{ | ||
_key = key; | ||
_args = args; | ||
} | ||
|
||
public override int ArgCount => 1 + _args.Count; | ||
|
||
protected override void WriteImpl(PhysicalConnection physical) | ||
{ | ||
physical.WriteHeader(Command, ArgCount); | ||
physical.Write(_key); | ||
foreach (var arg in _args) | ||
{ | ||
physical.WriteBulkString(arg); | ||
} | ||
} | ||
} | ||
|
||
/// <summary> | ||
/// The encoding that a sub-command should use. This is either a signed or unsigned integer of a specified length. | ||
/// </summary> | ||
public readonly struct BitfieldEncoding | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should have full struct equality impl - IEquatable, override GetHashCode, override Equals (via |
||
{ | ||
internal RedisValue RedisValue => $"{(IsSigned ? 'i' : 'u')}{Size}"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. seems uber-allocatey; I wonder if a
|
||
|
||
/// <summary> | ||
/// Whether the integer is signed or not. | ||
/// </summary> | ||
public bool IsSigned { get; } | ||
|
||
/// <summary> | ||
/// The size of the integer. | ||
/// </summary> | ||
public byte Size { get; } | ||
|
||
/// <summary> | ||
/// Initializes the BitfieldEncoding. | ||
/// </summary> | ||
/// <param name="isSigned">Whether the encoding is signed.</param> | ||
/// <param name="size">The size of the integer.</param> | ||
public BitfieldEncoding(bool isSigned, byte size) | ||
{ | ||
IsSigned = isSigned; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. validation on permitted sizes; 1-64I and 1-63U ? |
||
Size = size; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should there be some pre-rolled values here? |
||
} | ||
} | ||
|
||
/// <summary> | ||
/// An offset into a bitfield. This is either a literal offset (number of bits from the beginning of the bitfield) or an | ||
/// encoding based offset, based off the encoding of the sub-command. | ||
/// </summary> | ||
public readonly struct BitfieldOffset | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ditto struct equality |
||
{ | ||
/// <summary> | ||
/// Returns the BitfieldOffset as a RedisValue. | ||
/// </summary> | ||
internal RedisValue RedisValue => $"{(ByEncoding ? "#" : string.Empty)}{Offset}"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hmmm; can't cache this one - I wonder if RedisValue is hampering us here, and we should be using a custom internal struct with a custom writer.... meh, leave it like this for now, we can change that later - let's just get it correct for now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. note: this is unnecessarily allocatey in the raw offset case; should be IMO: |
||
|
||
/// <summary> | ||
/// Whether or not the BitfieldOffset will work off of the sub-commands integer encoding. | ||
/// </summary> | ||
public bool ByEncoding { get; } | ||
|
||
/// <summary> | ||
/// The number of either bits or encoded integers to offset into the bitfield. | ||
/// </summary> | ||
public long Offset { get; } | ||
|
||
/// <summary> | ||
/// Initializes a bitfield offset | ||
/// </summary> | ||
/// <param name="byEncoding">Whether or not the BitfieldOffset will work off of the sub-commands integer encoding.</param> | ||
/// <param name="offset">The number of either bits or encoded integers to offset into the bitfield.</param> | ||
public BitfieldOffset(bool byEncoding, long offset) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this should be |
||
{ | ||
ByEncoding = byEncoding; | ||
Offset = offset; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this have to be non-negative? |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,33 @@ | ||
using System; | ||
|
||
namespace StackExchange.Redis; | ||
|
||
/// <summary> | ||
/// Defines the overflow behavior of a BITFIELD command. | ||
/// </summary> | ||
public enum BitfieldOverflowHandling | ||
{ | ||
/// <summary> | ||
/// Wraps around to the most negative value of signed integers, or zero for unsigned integers | ||
/// </summary> | ||
Wrap, | ||
/// <summary> | ||
/// Uses saturation arithmetic, stopping at the highest possible value for overflows, and the lowest possible value for underflows. | ||
/// </summary> | ||
Saturate, | ||
/// <summary> | ||
/// If an overflow is encountered, associated subcommand fails, and the result will be NULL. | ||
/// </summary> | ||
Fail, | ||
} | ||
|
||
internal static class BitfieldOverflowHandlingExtensions | ||
{ | ||
internal static RedisValue AsRedisValue(this BitfieldOverflowHandling handling) => handling switch | ||
{ | ||
BitfieldOverflowHandling.Fail => RedisLiterals.FAIL, | ||
BitfieldOverflowHandling.Saturate => RedisLiterals.SAT, | ||
BitfieldOverflowHandling.Wrap => RedisLiterals.WRAP, | ||
_ => throw new ArgumentOutOfRangeException(nameof(handling)) | ||
}; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Linked list is usually suboptimal; honestly I think we should default to List-T here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Been a little while since I put this together so my recollection might be a bit off - IIRC my thinking was:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though now that I think of it, I suppose you are performing allocation each time you perform the addlast 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
frankly, linked-list just barely gets used - I'd sooner use a
List<T>
, but again I wonder whether this is actually aList<SomeUnionStructThatIsGetSetAndIncrby>
, so each element in the list is not an argument but a logical operation - this might also make it much easier to do very efficient single-shot operations, which I expect to be relatively common. Let me have a think here - I like the ideas in this PR, but I think we can iterate the API a little.