Skip to content

Avoid Unsafe.As for Memory<T> and ReadOnlyMemory<T> conversion #111023

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Jan 29, 2025
Merged

Avoid Unsafe.As for Memory<T> and ReadOnlyMemory<T> conversion #111023

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e09c892
Use `Unsafe.BitCast`to cast `Memory<T>` and `ReadOnlyMemory<T>`
xtqqczze Jan 2, 2025
cefb56b
remove use of `Unsafe`
xtqqczze Jan 2, 2025
355d14a
Merge branch 'main' into BitCast-Memory
xtqqczze Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 1 addition & 4 deletions src/libraries/System.Private.CoreLib/src/System/Memory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,6 @@ namespace System
[DebuggerDisplay("{ToString(),raw}")]
public readonly struct Memory<T> : IEquatable<Memory<T>>
{
// NOTE: With the current implementation, Memory<T> and ReadOnlyMemory<T> must have the same layout,
// as code uses Unsafe.As to cast between them.

// The highest order bit of _index is used to discern whether _object is a pre-pinned array.
// (_index < 0) => _object is a pre-pinned array, so Pin() will not allocate a new GCHandle
// (else) => Pin() needs to allocate a new GCHandle to pin the object.
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this not result in worse code generation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes looks like code generation is worse: MihuBot/runtime-utils#855.

Copy link
Contributor Author

@xtqqczze xtqqczze Jan 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could revert to e09c892.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why change anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code correctness.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

an internal constructor that does the direct field assignments instead (which is notably what Span and ReadOnlySpan do).

@tannergooding I tried that approach in cefb56b but it resulted in substantial regressions: MihuBot/runtime-utils#855

Copy link
Member

@tannergooding tannergooding Jan 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tannergooding, is the resulting regression being tracked in an issue somewhere so that we can ensure it's addressed?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/// <summary>
/// Returns an empty <see cref="Memory{T}"/>
Expand Down
9 changes: 3 additions & 6 deletions src/libraries/System.Private.CoreLib/src/System/ReadOnlyMemory.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,12 @@ namespace System
[DebuggerDisplay("{ToString(),raw}")]
public readonly struct ReadOnlyMemory<T> : IEquatable<ReadOnlyMemory<T>>
{
// NOTE: With the current implementation, Memory<T> and ReadOnlyMemory<T> must have the same layout,
// as code uses Unsafe.As to cast between them.

// The highest order bit of _index is used to discern whether _object is a pre-pinned array.
// (_index < 0) => _object is a pre-pinned array, so Pin() will not allocate a new GCHandle
// (else) => Pin() needs to allocate a new GCHandle to pin the object.
private readonly object? _object;
private readonly int _index;
private readonly int _length;
internal readonly object? _object;
internal readonly int _index;
internal readonly int _length;

internal const int RemoveFlagsBitMask = 0x7FFFFFFF;

Expand Down
2 changes: 1 addition & 1 deletion src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/MemoryMarshal.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ ref Unsafe.As<T, byte>(ref GetReference(span)),
/// as <see cref="Memory{T}"/> but only used for reading to store a <see cref="ReadOnlyMemory{T}"/>.
/// </remarks>
public static Memory<T> AsMemory<T>(ReadOnlyMemory<T> memory) =>
Unsafe.As<ReadOnlyMemory<T>, Memory<T>>(ref memory);
new Memory<T>(memory._object, memory._index, memory._length);

/// <summary>
/// Returns a reference to the 0th element of the Span. If the Span is empty, returns a reference to the location where the 0th element
Expand Down
Loading