Skip to content
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

Add Lease and non-boxing versions of Execute(Async) #2844

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

kevin-montrose
Copy link
Contributor

I've got a scenario where I need to do a ton of .Execute(Async) calls, and the lack of ways to avoid allocations are hurting.

So I've added some.

Added methods (generally on both IServer and IDatabase(Async), as paired with existing Execute(Async) methods):

  • ExecuteLease(Async)
    • Returns a Lease<byte>? (or Task<Lease<byte>?>) - documented as only allowing simple string, bulk string, and integer returns per-LeaseProcessor behavior
  • ExecuteLeaseExplicit(Async)
    • Identical to the non-params ExcuteLease(Async) methods except rather than taking an ICollection<object> it takes an ICollection<RedisKey> and an ICollection<RedisValue>
    • I introduced a new Message accordingly
    • The RedisKey values are not passed, they are only used to determine slots when cluster is enabled
    • Because I cannot identify the key values in args, the key prefixed classes throw NotSupportExceptions if ExecuteLeaseExplicit(Async) methods are called

I'm not married to these exact methods, but they seem the simplest way to get Execute-ish functionality without boxing and while reusing result allocations.

@mgravell
Copy link
Collaborator

Hey Monty; what's your timescales? Basically, I have a side branch that completely rewrites the fundamental writer/parser core, and one of the secondary aims is to enable zero-alloc custom commands from external callers.

Assuming you're coming at this with your work hat on, we could perhaps use yourself as a 1st party client to give me an excuse to merge that work on company time.

@NickCraver NickCraver requested a review from Copilot January 28, 2025 14:03
@kevin-montrose
Copy link
Contributor Author

Hey Monty; what's your timescales? Basically, I have a side branch that completely rewrites the fundamental writer/parser core, and one of the secondary aims is to enable zero-alloc custom commands from external callers.

Assuming you're coming at this with your work hat on, we could perhaps use yourself as a 1st party client to give me an excuse to merge that work on company time.

@mgravell It's short but not imminent - second week of February would be ideal.

To expand on the use case, this is a benchmarking tool. I suspect the functionality that is being Execute'd is already fast enough, but cannot prove it. Based on profiling, I believe my extra tail latency is allocation / GC related.

@kevin-montrose
Copy link
Contributor Author

{5209E2F6-F36C-4B20-BD83-FCFE5C45D591}
I thought we were friends @NickCraver .

@mgravell
Copy link
Collaborator

Considerations:

  1. this overlaps hugely with some separate work we have planned re the IO core; I propose we hold this PR for now, pending on that, which we can explore directly
  2. on the API: the keys/values thing (to avoid boxing) is confusing and does not directly map to the underlying API - and in particular is so similar in shape, but different in operation, to ScriptEvaluate, that it could cause significant problems; this also makes prefixing untenable (as we don't know which args are keys)
  3. we would prefer ReadOnlyMemory<T> over ICollection<T> (for example, like Memory optimization for ScriptEvaluate #2843)
  4. your scenario seems to be perf critical, making the separate work even more relevant

We're discussing this directly (off-GitHub), and will keep doing so.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants