Skip to content
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.

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

Move Span.NonPortableCast to MemoryMarshal and rename to Cast #24689

Closed
ahsonkhan opened this issue Jan 16, 2018 · 20 comments · Fixed by dotnet/corefx#26467
Closed

Move Span.NonPortableCast to MemoryMarshal and rename to Cast #24689

ahsonkhan opened this issue Jan 16, 2018 · 20 comments · Fixed by dotnet/corefx#26467
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory good first issue Issue should be easy to implement, good for first-time contributors
Milestone

Comments

@ahsonkhan
Copy link
Member

From https://github.com/dotnet/corefx/issues/26139#issuecomment-358090527:

Move and rename NonPortableCast to MemoryMarshal.Cast

Current API:

namespace System
{
    public static partial class MemoryExtensions
    {
        public static ReadOnlySpan<TTo> NonPortableCast<TFrom, TTo>(this ReadOnlySpan<TFrom> source) where TFrom : struct where TTo : struct { throw null; }
        public static Span<TTo> NonPortableCast<TFrom, TTo>(this Span<TFrom> source) where TFrom : struct where TTo : struct { throw null; }
    }
}

Proposed API:

namespace System.Runtime.InteropServices
{
    public static partial class MemoryMarshal
    {
        public static bool TryCast<TFrom, TTo>(ReadOnlySpan<TFrom> source, out ReadOnlySpan<TTo> output) where TFrom : struct where TTo : struct { throw null; }
        public static bool TryCast<TFrom, TTo>(Span<TFrom> source, out Span<TTo> output) where TFrom : struct where TTo : struct { throw null; }
    }
}

Edit: Removed this
Edit by @ianhays: Changed Cast to TryCast per discussion

cc @jkotas, @KrzysztofCwalina

@jkotas
Copy link
Member

jkotas commented Jan 16, 2018

The API on MemoryMarshal should not be an extension method.

@KrzysztofCwalina
Copy link
Member

@jkotas, what's the code we recommend people write when they want to use this API in a reusable library? i.e. How should they check if the cast is safe or not? If it turns out it's not, what should the fallback code do?

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

what's the code we recommend people write when they want to use this API in a reusable library?

Options:

  • Guarantee the alignment by pinning the managed memory or using unmanaged memory
  • Have platform checks, e.g. if I am on Intel, can use faster path that assumes misaligned memory access is ok

If it turns out it's not, what should the fallback code do?

Options:

  • Read one byte at a time. It is similar to what you do for BigEndian portability.
  • Use BinaryPrimitives helper methods.
  • Create aligned copy first

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jan 17, 2018

A couple more questions:

  1. Is it really as simple as ARM vs Intel? Based on my reading it's only a severe problem in ARM7 and older (which are not produced since 2006). On ARM7 (and even better 11), the unaligned reads seem to be 2x in worst case and in many cases have no impact.
  2. Wouldn't reading one byte at a time be a cure worse than the disease? It seems like it would be even slower in most cases. Similarly with the other suggestions.

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

Is it really as simple as ARM vs Intel?

For .NET Core today - yes. For Mono - the matrix is much larger.

ARM9 and older (which are not produced since 2006)

Are you sure that they are not produced? I think ARM stopped iterating on the base line designs sometime around 2006, but I believe these older ARM version are still produced in huge quantities, e.g. RPi0, Lego Mindstorm, ... .

On ARM11, the unaligned reads seem to be 2x in worst case and in many cases have no impact.

Unless you pass the unaligned memory to the OS, read floating point values from it, or perform interlocked operations on it - I believe all these fault even on latest ARM. (The list may be incomplete.)

Wouldn't reading one byte at a time be a cure worse than the disease?

It is case-by-case. Many places read one byte a time today and they work just fine. The reading one byte at a time is far from being a performance bottleneck in many cases.

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

For example, we have switched System.Reflection.Metadata to read one byte a time in most places to make it portable and uses special paths in a few bulkier places : dotnet/corefx#16583 . The performance impact of this change was non-measurable.

@KrzysztofCwalina
Copy link
Member

From what I read, ARM7 can in general read unaligned, but some read instructions cannot. ARM9+ all instructions support unaligned reads, but the feature can be turned on/off (at processor level) and so software cannot assume any given settings. Supposedly most OSes have the setting on, but some don't.

@KrzysztofCwalina
Copy link
Member

KrzysztofCwalina commented Jan 17, 2018

I kind of wish we made it possible to write

Span<byte> bytes = ...
if(!bytes.CanCast<int>()){
    bytes = bytes.ToArray();
}
Span<int> ints = bytes.Cast<int>(); // fails is misaligned on all processors

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

Why not bool TryCast(Span<TFrom> from, out Span<TTo> to) ?

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

ARM9+ all instructions support unaligned reads

I do not think it is the case. My copy of the ARM manual has a big table in chapter A3.2.1 Unaligned data access that shows what works and what does not.

@ahsonkhan
Copy link
Member Author

Why not bool TryCast(Span<TFrom> from, out Span<TTo> to) ?

How would we discern when to return false (i.e. whether the platform supports misaligned memory access) without significant perf regression?

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

For slow span, it would be slow like everything else. For fast span, it would need to be special crafted in the runtime per platform to be fast.

@jkotas
Copy link
Member

jkotas commented Jan 17, 2018

Just noticed a problem with this:

if(!bytes.CanCast<int>()){
    bytes = bytes.ToArray();
}
Span<int> ints = bytes.Cast<int>(); 

To be portable, you would rather want allocate array of the destination type and then copy the bytes to it. The array of bytes may not have sufficient alignment for the destination type.

@KrzysztofCwalina
Copy link
Member

I like the idea of TryCast.

@ianhays
Copy link
Contributor

ianhays commented Jan 18, 2018

I like TryCast too. Updated the proposal.

@KrzysztofCwalina
Copy link
Member

so, the idiomatic code would be:

Span<int> ints;
if(!bytes.TryCast(out ints)){
    ints = new int[bytes.Length/sizeof(int)];
    bytes.Slice(0, ints.Length * sizeof(int)).CopyTo(ints.AsBytes()); 
}

As a side note, I again wish out Copyto copied as much as it could and returned the number of items copied as opposed to demanding the right fit, i.e.

Span<int> ints;
if(!bytes.TryCast(out ints)){
    ints = new int[bytes.Length/sizeof(int)];
    bytes.CopyTo(ints.AsBytes); 
}

@ianhays
Copy link
Contributor

ianhays commented Jan 19, 2018

It wouldn't be an extension method if its in MemoryMarshal though, so an example would be more like:

Span<int> ints;
if(!MemoryMarshal.TryCast<byte, int>(bytes, out ints)){
    ints = new int[bytes.Length/sizeof(int)];
    bytes.Slice(0, ints.Length * sizeof(int)).CopyTo(ints.AsBytes()); 
}

@ianhays
Copy link
Contributor

ianhays commented Jan 19, 2018

Edit: removed.

@terrajobst
Copy link
Member

terrajobst commented Jan 19, 2018

Video

Looks good as proposed. We can add a more specialized API that handles the else case if we we want to. Only difference from the proposal is to leave it on MemoryExtensions as the API is now safe.

namespace System
{
    public static partial class MemoryExtensions
    {
        public static bool TryCast<TFrom, TTo>(this ReadOnlySpan<TFrom> source, out ReadOnlySpan<TTo> output) where TFrom : struct where TTo : struct;
        public static bool TryCast<TFrom, TTo>(this Span<TFrom> source, out Span<TTo> output) where TFrom : struct where TTo : struct;
    }
}

We'll also leave the NonPortableCast API but we'll move it MemoryMarshal. We need to make sure it's not an extension method, under which case we can rename it to just Cast.

@ianhays
Copy link
Contributor

ianhays commented Jan 19, 2018

I split the TryCast addition to https://github.com/dotnet/corefx/issues/26465 to make tracking easier.

This issue will track just the move and rename of NonPortableCast.

ianhays referenced this issue in dotnet/corefx Jan 29, 2018
Consume Span moves
- [Move Span.DangerousCreate to MemoryMarshal.CreateSpan](#26139)
- [Move ReadOnlySpan.DangerousCreate to MemoryMarshal.CreateReadOnlySpan](#26139)
- [Move Span.NonPortableCast to MemoryMarshal.Cast](#26368)
- [Move ReadOnlySpan.NonPortableCast to MemoryMarshal.Cast](#26368)
- [Add ToString override to Span and ReadOnlySpan](#26295)
- [Add ToEnumerable function to MemoryMarshal that takes a Memory](#24854)
@msftgits msftgits transferred this issue from dotnet/corefx Jan 31, 2020
@msftgits msftgits added this to the 2.1.0 milestone Jan 31, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Memory good first issue Issue should be easy to implement, good for first-time contributors
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants