Add ISpanParsable on to GrainId #8565
Conversation
Romanx
force-pushed
the
feature/span-parsable-grain-ids
branch
from
bb6c6f7 to
723cd7d
Compare
JsonConverter now uses span parsing path so tests added to verify this works.
Romanx
force-pushed
the
feature/span-parsable-grain-ids
branch
from
3024268 to
79adcab
Compare
|
@gfoidl Added tests for the Not sure if the tests are in the right place but it seemed like the best fit. If not I'm happy to move them. |
There was a problem hiding this comment.
For me the tests seem fine, but I'll let @ReubenBond decide on this.
| ? checked((int)reader.ValueSequence.Length) | ||
| : reader.ValueSpan.Length; | ||
|
|
||
| Span<char> buf = stackalloc char[valueLength]; |
There was a problem hiding this comment.
- perf-wise stack allocation to a constant (and then slicing) is faster than using a variable -- especially if
[SkipLocalsInit]is used, as then it's essentially only a (stack) pointer-move + the stack-cookie as safety guard valueLengthcould be large enough to get a stack overflow, so we take any measures against this in that public api
Thus I'd write this as
Span<char> buf = valueLength <= 128
? (stackalloc char[128]).Slice(0, valueLength)
: new char[valueLength];The allocation of the char-array will be on the cold-path, but we get safety for potential stack overflows.
There was a problem hiding this comment.
Sounds good I'll make that change now, I can see this could be a problem if we get a large GrainId keyed value so the allocation is probably sensible to avoid a stack overflow.
There was a problem hiding this comment.
the allocation is probably sensible to avoid a stack overflow
Exactly.
There could also be some malicious code that produces large valueLengths, so DoS could happen, etc.
When we assume that valueLength is quite often > 128, so the threshold could be 256 or instead of allocating the char-array one could be rented from the ArrayPool<char>. But I think the length will be rather short, so that allocation is OK, as it's only on the rare taken path which acts as security measure against SO.
Avoids possible stack overflow and speed in the hot path
| @@ -97,45 +98,56 @@ public void GrainIdShouldEncodeAndDecodePrimaryKeyGuidCorrectly() | |||
| } | |||
| } | |||
|
|
|||
| [Fact, TestCategory("SlowBVT"), TestCategory("Identifiers")] | |||
| public void GrainId_ToFromPrintableString() | |||
| [Theory, TestCategory("SlowBVT"), TestCategory("Identifiers")] | |||
There was a problem hiding this comment.
I wonder why we put these (obviously quick) tests in SlowBVT
|
@Romanx are you running 7.x in production at the moment and is this impacting performance for you? If so, we can create a new release. |
|
We are running 7.x in production but it's not impacting performance. I was writing a library that wraps over GrainId and wanted to parse it and had a span so noticed this and thought it'd be a nice improvement 👍 |
While working on a client library I noticed that
GrainIdwas notISpanParsablebut it did have implementations for parsing from a string.This adds the interface and makes the existing string parse methods conform with the
IParsableinterface too.Adding as draft as I need to check if some unit tests need to be added for this.
Microsoft Reviewers: Open in CodeFlow