[Wip} Use Unsafe rather than re_write_il #796

Closed
wants to merge 2 commits into
from

Projects

None yet

8 participants

@benaadams
Contributor

No description provided.

@davidfowl davidfowl commented on the diff Sep 10, 2016
src/System.Slices/System/PtrUtils.cs
@@ -2,16 +2,10 @@
// Licensed under the MIT license. See LICENSE file in the project root for full license information.
@davidfowl
davidfowl Sep 10, 2016 Collaborator

@jkotas Can you take a look here?

@omariom
Contributor
omariom commented Sep 10, 2016

@benaadams
Does it generate code of the same quality? Would be good to see perf numbers as well.

@omariom
Contributor
omariom commented Sep 10, 2016

Usage of AsPointer requires pinning.

@benaadams
Contributor

Usage of AsPointer requires pinning.

Might need some new methods in Unsafe then; can't use fixed on T and GCHandle.Alloc seems a bit heavyweight

@benaadams
Contributor
benaadams commented Sep 10, 2016 edited

@omariom could call through to another function byref?

What happens currently when the asm is inlined?

@jkotas
Member
jkotas commented Sep 10, 2016

Usage of AsPointer requires pinning

Right. And pinning would have a perf hit. It can be done without a perf hit using C# 7 locals and returns. Similar to dotnet/coreclr@0b9ba04.

@jkotas
Member
jkotas commented Sep 10, 2016

What happens currently when the asm is inlined?

The asm is operating with byrefs - it is what makes it GC safe.

@benaadams
Contributor

Alas dotnet/corefx#10540 with coreclr is currently out of my VS'15s league and VS 2015 finds them curious; so might have to pass the mantle on this one.

@benaadams
Contributor
benaadams commented Sep 10, 2016 edited

@jkotas do we need additional methods in Unsafe?

Currently have:
static !!T& AsRef<T>(void* source)

But I don't think Unsafe.AsRef<T>(Unsafe.AsPointer(ref o)) would work with GC?

So, additionally may need

// object, otherwise would need to chain 
// Unsafe.AsRef(Unsafe.As<T>(o))
static !!T& AsRef<T>(object source)` 
// heap structs
static !!T& AsRef<T>(T source)`
@jkotas
Member
jkotas commented Sep 10, 2016

What would these methods do - what would their implementation in IL be?

@omariom omariom commented on an outdated diff Sep 11, 2016
src/System.Slices/System/SpanExtensions.cs
{
- IntPtr count = PtrUtils.CountOfU<T, U>((uint)slice.Length);
- unsafe
- {
- // We can't compare IntPtrs, so have to resort to pointer comparison
- bool fits = (byte*)count <= (byte*)int.MaxValue;
- Contract.Requires(fits);
- countOfU = (int)count.ToPointer();
- }
+ long count = ((uint)slice.Length * Unsafe.SizeOf<T>()) / Unsafe.SizeOf<U>();
@omariom
omariom Sep 11, 2016 Contributor

slice.Length * Unsafe.SizeOf<T>() can overflow

@benaadams
Contributor

What would these methods do

Return what they point at as a ref return; though I now think that can be just as easily done with a ref function.

@mattwarren mattwarren and 1 other commented on an outdated diff Sep 12, 2016
src/System.Slices/System/PtrUtils.cs
- .locals([0] uint8 & addr)
- ldarg.0 // load the object
- stloc.0 // convert the object pointer to a byref
- ldloc.0 // load the object pointer as a byref
- ldarg.1 // load the offset
- add // add the offset
- ldarg.2 // load the index
- sizeof !!T // load size of T
- mul // multiply the index and size of T
- add // add the result
- ldobj !!T // load a T value from the computed address
- ret")]
- public static T Get<T>(object obj, UIntPtr offset, UIntPtr index) { return default(T); }
+ public static unsafe T Get<T>(object obj, UIntPtr offset, UIntPtr index)
+ {
+ return Unsafe.Read<T>(*(byte**)Unsafe.AsPointer(ref obj) + offset.ToUInt64() + index.ToUInt32() * Unsafe.SizeOf<T>());
@mattwarren
mattwarren Sep 12, 2016

Maybe a dumb question, but as you're replacing some in-line IL with 3 method calls, do all the Unsafe methods get inlined as well? (I know they're marked as assgressiveinlining, but have you verified they are?)

@benaadams
benaadams Sep 12, 2016 edited Contributor

The will do as the inliner thinks in IL. However this usage is invalid due to not being GC safe; need to use the ref return methods.

@benaadams benaadams closed this Sep 12, 2016
@benaadams benaadams reopened this Sep 13, 2016
@benaadams benaadams changed the title from Use Unsafe rather than re_write_il (+Update everything) to [Wip} Use Unsafe rather than re_write_il Sep 13, 2016
@benaadams benaadams Wip
b99bf64
@omariom
Contributor
omariom commented Sep 13, 2016 edited

@benaadams I suspect that casting to array and fixing it will make getters and setter very slow.

update: very very slow :)

@benaadams
Contributor
benaadams commented Sep 13, 2016 edited

Probably; its more a discovery on what Unsafe needs.

The ref Add methods don't quite work as its ref + a bit for the array offset; due to the aliasing with object

@omariom
Contributor
omariom commented Sep 13, 2016

@jkotas @benaadams
Is it even possible to replace the original trick Joe Dufy describes in his comment with ref operations?

@jkotas
Member
jkotas commented Sep 13, 2016 edited

The implementation of that trick is questionable. It is invalid IL according to the spec that is lucky to make it through the JIT - there is no implicit conversion from Object to byref.

If you run System.Slices on checked build of the JIT, you will see multiple asserts like Assertion failed 'genActualType(op1->TypeGet()) == TYP_BYREF && genActualTypeIsIntOrI(op2->TypeGet())' in 'System.PtrUtils:ElemOffset(ref):int' (IL size 14) because of that.

The implementation using S.R.CS.Unsafe and ref operations can look something like:

class RawData
{
    public byte Data;
}

...

public T this[int index]
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    get
    {
       Contract.RequiresInRange(index, (uint)Length);
       if (Object != null)
           // This should use Unsafe.Add overload with IntPtr argument that does not exist yet. It was discussed in https://github.com/dotnet/corefx/issues/10451, 
           // but I did not add it because of it was not clear whether it is useful, what it should do exactly, and what would be right name for it.
           // return Unsafe.Add<T>(ref Unsafe.AddByteOffset<T>(ref Unsafe.As<RawData>(Object).Data, Offset), index);

           // In the meantime, the int overload will work fine, except for very large arrays...
           return Unsafe.Add<T>(ref Unsafe.As<T>(ref Unsafe.Add<byte>(ref Unsafe.As<RawData>(Object).Data, (int)Offset)), index);
       else
           return Unsafe.Add<T>(ref Unsafe.AsRef<T>((void *)Offset), index);
    }
}

It has extra check for Object != null that is hard to get rid of without the invalid IL.

@benaadams
Contributor

CopyBlock also needs the by ref and plus a bit version.

@jkotas
Member
jkotas commented Sep 13, 2016

CopyBlock also needs the by ref and plus a bit version.

I think it is a good idea to add these overloads to S.R.CS.Unsafe in corefx now that we have a good use case for them.

@benaadams
Contributor

@jkotas not sure of good api. Something like?

static ref T Add<T>(ref object source, int byteInset, int elementOffset)

Are 3 types

  • object native inset 0 bytes
  • object T[] inset arrayStart bytes
  • object string inset stringStart bytes
@jkotas
Member
jkotas commented Sep 13, 2016

The API I had in mind was: static ref T AddByteOffset<T>(ref T source, IntPtr byteOffset).

The problem with static ref T Add<T>(ref object source, int byteInset, int elementOffset) is that it is not possible to implement it using valid IL in performant way. S.R.CS.Unsafe is limited to what can be implemented in valid IL.

@nietras
nietras commented Sep 14, 2016

static ref T AddByteOffset<T>(ref T source, IntPtr byteOffset)

👍 for adding this to Unsafe, perhaps we can review this in a separate issue or pull request.

@nietras
nietras commented Sep 14, 2016

@jkotas wait... instead of AddByteOffset would it not be better to just add overloads to Add for IntPtr instead of a AddByteOffset, which is less useful? We did talk about IntPtr support but choose not to since use cases might be few. Adding IntPtr overload for both Add and Subtract seems more useful and within API design.

@jkotas
Member
jkotas commented Sep 14, 2016

AddByteOffset which is less useful?

It is more useful for this case. If we add the Add method that takes elementOffset as IntPtr, there will need to be ~3 calls to Unsafe methods instead of just one in this case.

I think it is reasonable to say: int is used for element offsets - indices are ints pretty much everywhere in .NET; IntPtr is used for byte offsets - used for the super low-level code.

@nietras
nietras commented Sep 14, 2016

more useful for this case. If we add the Add method that takes elementOffset as IntPtr, there will need to be ~3 calls to Unsafe methods instead of just one in this case.

Useful perhaps, but also less general in scope I would think, and there still needs to be 2 calls as far as I can tell, and the As calls are noops after jitting right? So is the convience that important in the general case?

Adding Pinnable as we have discussed would help too, though, so RawData is not needed.

@benaadams
Contributor

Will need to capture the extra apis needed in a corefx Issue for Unsafe, then can close this?

@KrzysztofCwalina
Collaborator

@benaadams, what should we do with this one?

@benaadams
Contributor

Need to capture the missing methods in corefx for Unsafe then close.

@KrzysztofCwalina
Collaborator

BTW, the Unsafe class is available only in the most recent NetStandard, correct? I wanted System.Slices to work on as many versions as possible.

@jkotas
Member
jkotas commented Sep 30, 2016

BTW, the Unsafe class is available only in the most recent NetStandard, correct?

Unsafe class is OOB, supported all the way from NetStandard 1.0.

@davidfowl
Collaborator

If we can get this working, the build time for this repository will be much better 😄

@KrzysztofCwalina
Collaborator

@jkotas, ah good to know.

@benaadams benaadams deleted the benaadams:unsafe branch Oct 7, 2016
@benaadams
Contributor

Required functions are now in Unsafe thanks to @nietras 👍

@nietras
nietras commented Oct 9, 2016

Thanks, can I ask what the plan is regarding this? Are you going to wait
until C# 7 is released?

On Oct 7, 2016 10:51 PM, "Ben Adams" notifications@github.com wrote:

Required functions are now in Unsafe thanks to @nietras
https://github.com/nietras 👍


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#796 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AKTG7wE4xvvGVjvbchNc5LHuLuqGW-Faks5qxrC8gaJpZM4J5wdW
.

@KrzysztofCwalina
Collaborator

How do I use Unsafe.AsPointer with the Span's fields in corfxlab? The fields are readonly and so I cannot pass them in by-ref to AsPointer.

@jkotas
Member
jkotas commented Oct 10, 2016

Using Unsafe.AsPointer(ref ...) on Span fields does not sound right. What do you need to use the AsPointer for?

@KrzysztofCwalina
Collaborator

For example, I would like to remove UnsafeUtilities.Get. Wat S.R..CS.Unsafe APIs should I use instead?

@jkotas
Member
jkotas commented Oct 10, 2016 edited

The current implementation for UnsafeUtilities.Get contains invalid IL. If you run it through the debug build of the JIT, you will hit asserts (see #796 (comment) for details). It happens to generate a functional code today, but there are likely cases where the JIT will just blow up; or where the GC tracking information will be incorrect.

The Unsafe class does not contain the invalid IL, and so the approach needs adjustment. Here is an example how the Span indexer can be implemented using the recently added Unsafe methods:

class RawData
{
    internal T Data;
}

public T this[int index]
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    get
    {
        Contract.RequiresInRange(index, (uint)Length);
        return Unsafe.Add(ref Unsafe.AddByteOffset(ref Unsafe.As<RawData>(Object).Data, Offset), index)
    }
...
  • This implementation requires change in the meaning of the Offset field. It needs to be adjusted by the offset of RawData.Data field.
  • This implementation is not compatible with runtimes that do not internally store array elements inline (e.g. it would not work with disassociative arrays). The existing implementation in corefxlab has the same problem.
@nietras
nietras commented Oct 12, 2016

needs to be adjusted by the offset of RawData.Data field.

@jkotas regarding this, do we need to add unsafe code for finding these offsets? E.g. something like:

IntPtr ByteOffset<L, R>(ref L left, ref R right);

and will this also be useful instead of:

[ILSub(@"
            ldarg.0
            ldc.i4 0
            ldelema !!T
            ldarg.0
            sub
            ret")]
        internal static int ElemOffset<T>(T[] arr)
 { throw new Exception("ILSub did not execute."); }

Or do we need to add something similar like this to Unsafe. I though this element offset might be found with:

var a = new byte[10];
var offset = Unsafe.ByteOffset(ref a, ref a[0]);

but it may not be as performant due to bounds checking.

@jkotas
Member
jkotas commented Oct 12, 2016

Unsafe.ByteOffset(ref a, ref a[0]);

This would subtract address of a on the stack from address of first element of the array. Instead, it needs to be something like: Unsafe.ByteOffset(ref a[0], ref Unsafe.As<RawData>(a).Data);.

I agree that Unsafe.ByteOffset method should be useful (you can do the operation today, but it requires pinning). It may be a good idea to make both arguments to have same type - to make you write extra extra code to diff apples and oranges.

due to bounds checking

I do not think we need to worry about bounds checking here. This is done in Span<T> constructor that has to do some bounds checking already. It may be possible to structure it such that the JIT will eliminate bounds check for this one as redundant.

@nietras
nietras commented Oct 12, 2016

Unsafe.ByteOffset(ref a[0], ref Unsafe.As(a).Data);

Ha, so careful with the function signature, then mess up the example completely, yes that is correct.

it requires pinning

Yes, this was what I thought would be good to avoid.

Ok, added a issue for this addition to Unsafe dotnet/corefx#12593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment