Refactor fixed statement logic in System.Text
#118352
Conversation
dotnet-policy-service
bot
added
the
community-contribution
|
Tagging subscribers to this area: @dotnet/area-system-text-encoding |
|
@xtqqczze is this a blocker to .NET 10? The bar now is a little bit higher and if this is not a blocker we can delay this to the next release. CC @MihaZupan |
|
Definitely not a blocker for 10 |
|
I have converted the PR to be draft till the main will be open for .NET 11 changes. Thanks! |
|
Ready for review now |
There was a problem hiding this comment.
LGTM. Thanks.
Cc @stephentoub @jkotas in case they are interested in taking a quick look.
| @@ -93,7 +93,7 @@ public override unsafe int GetByteCount(char[] chars, int index, int count) | |||
| ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.chars, ExceptionResource.ArgumentOutOfRange_IndexCountBuffer); | |||
| } | |||
|
|
|||
| fixed (char* pChars = chars) | |||
| fixed (char* pChars = &MemoryMarshal.GetArrayDataReference(chars)) | |||
There was a problem hiding this comment.
We're trying to reduce the amount of unsafe logic in the core libraries, these types of micro-optimizations aren't really something I think we want to take.
It'd rather be better for the JIT to be able to avoid or elide any additional cost here from the GetPinnableReference logic in the case it knows chars is non-null.
There was a problem hiding this comment.
This is unsafe code both before and after this change. We use MemoryMarshal.GetArrayDataReference and GetPinnableReference micro-optimizations in many places, so I do not see a problem with using it in a few more places.
It'd rather be better for the JIT to be able to avoid or elide any additional cost here from the GetPinnableReference logic in the case it knows chars is non-null.
The JIT does that where possible. "Avoid handling the null case when the variable is known not be null." is not accurate description for majority of the codediff deltas. (There are a few places where it is impossible for the JIT to prove that the null case is unreachable, but these are just a small part of the delta.)
Majority of the codediff deltas are caused by pinning byref instead of object. Before this change, the code pinned object that is a bit more code and a bit friendlier to the GC. After this change, the code pins byref that is a bit less code and a bit less friendly to the GC.
There was a problem hiding this comment.
I do not see a problem with using it in a few more places.
Having said that, I agree that these micro-optimizations add clutter, and the benefit is non-measurable in most cases.
Avoid handling the null case when the variable is known not be null.
MihuBot/runtime-utils#1346