Initial POC commit for adding ReadOnlyMemory overloads to StringSet o… - #2221
Initial POC commit for adding ReadOnlyMemory overloads to StringSet o…#2221Stabzs wants to merge 1 commit into
Conversation
Sorry, something went wrong.
| /// <remarks> | ||
| /// <seealso href="https://redis.io/commands/mset"/>, | ||
| /// <seealso href="https://redis.io/commands/msetnx"/> | ||
| /// </remarks> |
There was a problem hiding this comment.
possibly worth a warning about FireAndForget being dangerous here if the ReadOnlyMemory is going to be recycled/reused?
Sorry, something went wrong.
There was a problem hiding this comment.
Definitely seems wise, thank you. I'll add warnings for all of the new methods taking ReadOnlyMemory. If the approach looks good overall, I'll continue expanding the surface area.
Sorry, something went wrong.
|
|
||
| for (int i = 0; i < outer.Length; ++i) | ||
| { | ||
| inner[i] = ToInner(outer.Span[i]); |
There was a problem hiding this comment.
Accessing .Span is relatively expensive; this should be done outside the loop and resused.
Sorry, something went wrong.
There was a problem hiding this comment.
The ++i is very unusual for C#, note
Sorry, something went wrong.
There was a problem hiding this comment.
Makes sense, thanks! I'll make the changes to cache all call sites for .Span.
The ++i was copied from line 801 of the diff. I assumed that was intentional and copied it here. Is that an incorrect usage?
Sorry, something went wrong.
|
Not wrong, just atypical. Consistency/copy-paste is a valid reason here!
…On Sat, 27 Aug 2022, 00:09 Stabzs, ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/StackExchange.Redis/KeyspaceIsolation/WrapperBase.cs
<#2221 (comment)>
:
> @@ -804,6 +807,25 @@ internal WrapperBase(TInner inner, byte[] keyPrefix)
}
}
+ protected ReadOnlyMemory<KeyValuePair<RedisKey, RedisValue>> ToInner(ReadOnlyMemory<KeyValuePair<RedisKey, RedisValue>> outer)
+ {
+ if (outer.Length == 0)
+ {
+ return outer;
+ }
+ else
+ {
+ KeyValuePair<RedisKey, RedisValue>[] inner = new KeyValuePair<RedisKey, RedisValue>[outer.Length];
+
+ for (int i = 0; i < outer.Length; ++i)
+ {
+ inner[i] = ToInner(outer.Span[i]);
I'll make the changes to cache all call sites for .Span.
The ++i was copied from line 801 of the diff. I assumed that was
intentional and copied it here. Is that an incorrect usage?
—
Reply to this email directly, view it on GitHub
<#2221 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAEHMDLPK2KLL5VTQUUT43V3FFC7ANCNFSM566WTIXA>
.
You are receiving this because you commented.Message ID:
***@***.***>
|
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
…perations.