Avoid Unsafe.As for Memory<T> and ReadOnlyMemory<T> conversion
#111023
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-memory |
| @@ -187,7 +184,7 @@ internal Memory(object? obj, int start, int length) | |||
| /// Defines an implicit conversion of a <see cref="Memory{T}"/> to a <see cref="ReadOnlyMemory{T}"/> | |||
| /// </summary> | |||
| public static implicit operator ReadOnlyMemory<T>(Memory<T> memory) => | |||
| Unsafe.As<Memory<T>, ReadOnlyMemory<T>>(ref memory); | |||
| new ReadOnlyMemory<T>(memory._object, memory._index, memory._length); | |||
There was a problem hiding this comment.
Does this not result in worse code generation?
There was a problem hiding this comment.
Yes looks like code generation is worse: MihuBot/runtime-utils#855.
There was a problem hiding this comment.
I could revert to e09c892.
There was a problem hiding this comment.
an internal constructor that does the direct field assignments instead (which is notably what
SpanandReadOnlySpando).
@tannergooding I tried that approach in cefb56b but it resulted in substantial regressions: MihuBot/runtime-utils#855
There was a problem hiding this comment.
I tried that approach in cefb56b but it resulted in substantial regressions: MihuBot/runtime-utils#855
In general we're trying to move away from unsafe code unless its absolutely necessary.
The regressions here might be something that is simple to handle in the JIT, in which case us fixing that would be preferred. At a glance, nothing here really looks "substantial"; rather its a regression of 2400 bytes across the entirety of the BCL (several in things like enumerators and other scenarios that are a bit less likely to occur in practice). The size regressions mostly look due to an unnecessary copy being made as part of the process, which is something that promotion should likely be handling so @jakobbotsch might be the right person to loop in.
Since span is doing the "right things" here, its possible that this is just a pessimization due to their being 3 fields or due to 1 of the fields being a GC ref.
There was a problem hiding this comment.
@tannergooding, is the resulting regression being tracked in an issue somewhere so that we can ensure it's addressed?
There was a problem hiding this comment.
@stephentoub looking at the latest (MihuBot/runtime-utils#956) I don't see any substantive regressions.
There's wins and losses both ways, some of the "regressions" are from additional inlining, some of the "improvements" are from no longer inlining.
Actually looking at the diffs, the JIT looks to be generally doing better things around tracking the locals as expected. Any negatives would also apply to scenarios like Span<T> and is tracked by the general "first class struct" work that's been done incrementally for several years:
https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/jit/first-class-structs.md, etc
We can log a more explicit issue if anything shows up in the perf regressions, but I expect this is just general improvements.
There was a problem hiding this comment.
cefb56b to
e09c892
Compare
|
@xtqqczze could you update the PR title to reflect the actual change being made now? @stephentoub could you give this a second look, its now following the same pattern as is used by |
Unsafe.BitCastto cast Memory<T> and ReadOnlyMemory<T>Unsafe.As for Memory<T> and ReadOnlyMemory<T> conversion
* main: (31 commits) More native AOT Pri-1 test tree bring up (dotnet#111994) Fix BigInteger outerloop test (dotnet#111841) JIT: Run 3-opt once across all regions (dotnet#111989) JIT: Check for profile consistency throughout JIT backend (dotnet#111684) [JIT] Add legacy extended EVEX encoding and EVEX.ND/NF feature to x64 emitter backend (dotnet#108796) [iOS][globalization] Fix IndexOf on empty strings on iOS to return -1 (dotnet#111898) System.Speech: Use intellisense xml from dotnet-api-docs (dotnet#111983) [mono][mini] Disable inlining if we encounter class initialization failure (dotnet#111754) [main] Update dependencies from dotnet/roslyn (dotnet#111946) Update dependencies from https://github.com/dotnet/arcade build 20250129.2 (dotnet#111996) Try changing the ICustomQueryInterface implementation to always return NotHandled instead of Failed to defer back to the ComWrappers impl. (dotnet#111978) Combined dependency update (dotnet#111852) Replace OPTIMIZE_FOR_SIZE with feature switch (dotnet#111743) Fix failed assertion 'FPbased == FPbased2' (dotnet#111787) Add remark to `ConditionalSelect` (dotnet#111945) JIT: fix try region cloning when try is nested in a handler (dotnet#111975) Use IRootFunctions in Tensor.StdDev (dotnet#110641) Remove zlib dependencies from Docker containers (dotnet#111939) Avoid `Unsafe.As` for `Memory<T>` and `ReadOnlyMemory<T>` conversion (dotnet#111023) Cleanup membarrier portability (dotnet#111943) ...
No description provided.