BITFIELD and BITFIELD_RO feature

[slorello89]

Adding BITFIELD and BITFIELD_RO support as part of #2055.

Couple notes about this command

1. It's variadic in the allowable sub-commands, so I provided a single command for each sub-command, and then split out the sub-commands into their own structures and allowed you to arbitrarily create an array of sub-commands you want to execute.
2. The literal return-type of single sub-commands are an array of integers, however I abstracted that away and just have them yield a single 64 bit signed integer, which meant some updates to handle multi-bulk return-types in the single-value processor for long and long?
3. This is smart enough to switch between BITFIELD and BITFIELD_RO if the RO command is available and it determines that the all the commands are read-only. My question though, is there a better way to check for BITFIELD_RO availability, I just do a check against the version for 6.2.0 which is when BITFIELD_RO was added.
4. The BITFIELD command's encodings top out at i64/u63, so an i64 should be a suitable return type regardless of what the encoding type is.

[NickCraver]

@slorello89 Haven't forgotten about this, just also unlikely to get so much time-wise here before trip next week. Overall want I want to try is instead of an array feeding into the method, we can have each operation as a struct (internal, not exposed). Then using a builder model (actually on the API) we can build up the command to pass in. Ultimately having a linked list of structs, each with a ref to the next via internal interface or something (need to play - just spitballing).

Then all we're creating is a single object for the command - it could even be based on Message and control the writing internally. Given the audience for this almost certainly cares about performance I'd like to make it as efficient as reasonably possible but need a few hours to hack at that idea. If you wanna give it a stab by all means! Just didn't want this hanging without an update :)

[slorello89]

Hey @NickCraver - no worries - I'm aware you were on break (I feel bad you were taking any time out at all to look at this stuff).

Just so I'm clear what you're looking for. For the variadic command, you want to convert the command classes -> structs, remove them from the public API, and use a builder to create a linked list of those structs under the hood and use the builder in the public API:

long?[] StringBitfield(RedisKey key, BitfieldCommandBuilder builder, CommandFlags flags = CommandFlags.None);

Couple other questions:

1. Are you ok with the non-variadic commands? Internally they would be converted to use the structs rather than the classes
2. Are you ok with the Signedness & BitfieldOverflowHandling enums?

[NickCraver]

Yeah that looks roughly what I'm guessing at - I think the enums themselves are fine (given they'll be args somewhere either way) - can revisit naming in the end, don't need to get it perfect to figure out the API surface here!

[slorello89]

@NickCraver - what do you think of this?

I blasted out the classes/structs for the sub-commands and let the builder stand on its own. There didn't seem that much sense to have the individual structures broken out with the builder in place, the non-variadic ones could have used them I suppose but then that's an awful lot of repeated code and it seemed a bit redundant to have the builder initialize a bunch of structs when it could just maintain the values independently.

It maintains a LinkedList of the values that you're going to pass into Redis and the Build message builds the command message.

Inside the write for the message, it IS using a foreach, so it's incurring a bit of extra cost to allocate the enumerator, perhaps the better approach is to just use the CommandKeyValuesMessage and the LinkedList of arguments to an array when initializing the message (those writes are being done in one of the more performance-sensitive areas right?)

[NickCraver]

Playing with the builder in https://github.com/StackExchange/StackExchange.Redis/tree/craver/bitfield-exp - I think after trying this out having all the newed up struct is odd and I think we can mostly make them an implementation detail instead. Will poke more tomorrow I hope!

[shacharPash]

@NickCraver So what is actually missing here? I would be happy to complete what is missing so that this PR can be merged

[NickCraver]

@slorello89 I don't think there's something missing so much as the current API is extremely heavy and we never got back to designing what this should look like in .NET. I don't have an ETA on getting back to it currently.

[mgravell]

added thoughts, but overall I agree that the API looks weird with the custom structs; I wonder if we can hide those and just take more args on the methods; @NickCraver thoughts on how we resurrect this?

[slorello89]

@mgravell - merged main and pushed up.

With structs vs extra args, I guess my main thought was it would be a fair number of args - SET would go from 3->5, INCRBY would go from 3/4->5/6, GET would go from 2->4. So it's right around the threshold where I'd consider introducing new structures. But to your and Nick's point, it is a fair amount of allocation for something that is meant to be quite lightweight.

So think just nix those structures, move to LinkedList->List, some minor name changes?

[mgravell]

I want to play with an alternative API before we get too carried away. This feels pretty complex at the moment. What I have in mind is something like:

public readonly struct BitfieldOperation
{
  long offset, value
  SomeInternalEnum : byte opType
  byte encoding // low 6 == width
  // high 2 == by bit/enc, signed/unsigned
  public static BitfieldOperation Get(long offset,
      byte width, bool unsigned = false, offsetByBit = false)
    public static BitfieldOperation Set(long offset,
      byte width, bool unsigned = false, offsetByBit = false)
   public static BitfieldOperation Increment(long offset,
      byte width, BitfieldOverflow overflow = /* Todo */, bool unsigned = false, offsetByBit = false)

  // Other bits not shown
}

With the main bitfield methods taking either a single BitfieldOperation or an array.

Internally, we can check the BitfieldOperation if the operand(s) to compute RO. No need for the builder, list, or packs of RedisValue - we should also be able to have each operation say "I contribute this many args", so we can do efficient write

But more importantly, it allows simple usage at the call site, i.e.

dB.Bitfield(key, [ BitfieldOperation.Get(...), BitfieldOperation.Set(...) ])

Thoughts?

[slorello89]

Seems pretty sensible, I think my only thing would be to initialize the offset/encoding as their proper RedisValues in Get Set Increment (can toss validation errors there) WDYT?

[mgravell]

I think my only thing would be to initialize the offset/encoding as their proper RedisValues

It seems to me likely that we can avoid that entirely - ultimately we don't need a RedisValue here, IMO

[slorello89]

@mgravell - Isn't it going to be implicitly cast to a bulk string when it's written to the socket anyway?

[mgravell]

@slorello89 yes and no; we have power to do anything we like there - we could, for example, stackalloc a small chunk, write the prefix+value to that, and write that combination to the output, zero allocs. Or possibly zero copy. It doesn't need to be a string, necessarily

[slorello89]

@mgravell - I updated the API per your comments. An additional thought:

I find these signatures a bit weird: BitfieldOperation Set(long offset, byte width, long value, bool offsetByBit = true, bool unsigned = false) - because it disconnects the offset/encoding parts of the command. A more 'sensible' one to me would be BitfieldOperation Set(long offset, bool offsetByBit, byte width, bool unsigned, long value) - I know in this case they'd no longer be optional parameters, but should they be? Idk just a thought.

[slorello89]

@mgravell that's fair - but is there anywhere currently that's being done (in the context of writing out to the pipeline?)

[NickCraver]

@slorello89 It took me a while to get over the array allocations on this for a lightweight API until I realized: "users are probably calling it the same way over and over"...in which case with proper usage that shouldn't be a problem, we just need examples of where they're re-using the same array passed in every time. It's really hard to judge the optional params because there's zero usage of this today to go by.

Is there any chance you have examples from other projects, that are calling this API set and what they're usually doing? I'd argue: if they're used most of the time, we can just not make them optional at all (we can even intelligently pack the struct better).

This and other things has made me realize just how much we're lacking a samples section (keeps coming up with logging and event handlers), that would be equally helpful in evaluating PR/API additions too. Not asking for that here, just saying: top of mind and likely a good prototype approach once that's in place.

[slorello89]

@NickCraver - looking across the client ecosystem it looks like there are a couple of approaches to this API:

1. Just send a bunch of strings and figure it out yourself e.g. jedis, go-redis
2. Builder pattern with arbitrary arguments. e.g. redis-py
3. Strongly-typed builder pattern e.g. lettuce or redisson

I can't say I've seen many usages of this command in the wild, and I've only ever heard a few people ask for the command in .NET (I've pointed them to the ad-hoc API) - but I suspect people tend to lean on the builder patterns where available.

Examples are always nice, we are working on getting examples for us to use in code snippets in redis.io across the language communities (the issue that got the ball rolling on this again is part of that effort). See an example of these snippets here