Adding MemoryHandle AddOffset to fix Memory.Retain impl #24323

ahsonkhan · 2017-09-29T00:29:43Z

Fixes https://github.com/dotnet/corefx/issues/24317

AddOffset throws if PinnedPointer is null. Should it fail silently and do nothing?
Would it be better to create a setter for PinnedPointer instead of the AddOffset API?

cc @pakrym, @KrzysztofCwalina, @stephentoub, @davidfowl, @jkotas

stephentoub · 2017-09-29T00:38:37Z

src/System.Memory/ref/System.Memory.cs

@@ -178,7 +178,8 @@ public unsafe struct MemoryHandle : IDisposable
    {
        public MemoryHandle(IRetainable owner, void* pinnedPointer = null,  System.Runtime.InteropServices.GCHandle handle = default(System.Runtime.InteropServices.GCHandle))  { throw null; }
        public void* PinnedPointer { get { throw null; } }
-        public void Dispose()  { throw null; }
+        public void AddOffset(int offset) { throw null; }


Has this been API reviewed? It feels strange to me to mutate the MemoryHandle in this fashion, and we have Unsafe.Add in case someone wants to work with the pointer. If this is purely to help with the implementation of Memory, maybe it should just be internal?

If this is purely to help with the implementation of Memory, maybe it should just be internal?

I agree. I think this could be an internal API that is only needed by Memory.

@KrzysztofCwalina, do you see a scenario where we would need to expose the AddOffset API publically?

(And if for some reason its functionality must be public, I'd prefer if we just made PinnedPointer settable.)

Yes, the scenario is I use OwnedMemory and MemoryHandle directly, without using Memory, e.g. I implement my own Memory-like factory for spans. If we don't like AddOffset, we should add OwnedMemory.Pin overload that takes offset to the added to the pointer inside the handle.

Having said that, we need to fix this bug today, and so possibly an internal API is a better change short term so that we don't have to rush the API review.

Having said that, we need to fix this bug today, and so possibly an internal API is a better change short term so that we don't have to rush the API review.

The CI system is still down it seems. I am not sure if we can get this fix merged today.
cc @mmitche

stephentoub · 2017-09-29T00:49:53Z

src/System.Memory/src/System/Memory.cs

@@ -193,6 +193,7 @@ public unsafe MemoryHandle Retain(bool pin = false)
                if (_index < 0)
                {
                    memoryHandle = ((OwnedMemory<T>)_arrayOrOwnedMemory).Pin();
+                    memoryHandle.AddOffset((_index & RemoveOwnedFlagBitMask) * Unsafe.SizeOf<T>());


It feels wrong to me to expose a public method like AddOffset for mutating the handle. I think I'd prefer either:

Just have AddOffset be internal, or replace it with an internal SetPointer and use Unsafe.Add to compute the new one

Create a new MemoryHandle with the inputs from the one returned by Pin, and just don't dispose the first one.

I agree, creating new memory handle sound like much better idea and does not require any API change.

ahsonkhan · 2017-09-29T01:12:12Z

@dotnet-bot test this please

KrzysztofCwalina · 2017-09-29T01:56:45Z

looks good.

Separately, please file a bug to change the Pin APIs to take an index (or some other solution to let code not using Memory do the offset.

ahsonkhan · 2017-09-29T02:09:16Z

@dotnet-bot test Linux x64 Release Build
@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

ahsonkhan · 2017-09-29T02:09:55Z

@dotnet-bot test UWP CoreCLR x64 Debug Build

ahsonkhan · 2017-09-29T02:40:28Z

Need to merge dotnet/coreclr#14248 before CI here will be green/this PR can be merged.

KrzysztofCwalina · 2017-09-30T00:19:47Z

@dotnet-bot test Linux x64 Release Build

ahsonkhan · 2017-09-30T00:54:59Z

@KrzysztofCwalina, I believe we need the following PR to get merged before the CI will pass:
#24349

jkotas · 2017-09-30T01:00:22Z

#24349 won't help in its current state. It is not updating CoreCLR.

ahsonkhan · 2017-09-30T01:05:12Z

#24349 won't help in its current state. It is not updating CoreCLR.

Ah yes, good point.
The coreclr version just got updated in the versions repo ~11 minutes ago:
dotnet/versions@a330a6b#diff-5f6099c37f777c410c4397b3f1e38870

How long does it usually take to trigger a PR after a version gets updated in the version repo?

ahsonkhan · 2017-10-02T19:38:59Z

@dotnet-bot test Linux x64 Release Build

ahsonkhan · 2017-10-02T19:39:20Z

@dotnet-bot test Windows x64 Debug Build
@dotnet-bot test Windows x86 Release Build

ahsonkhan · 2017-10-02T20:00:33Z

@dotnet-bot test OSX x64 Debug Build
@dotnet-bot test UWP CoreCLR x64 Debug Build

stephentoub · 2017-10-06T03:09:17Z

src/System.Memory/ref/System.Memory.cs

@@ -178,7 +178,8 @@ public unsafe struct MemoryHandle : IDisposable
    {
        public MemoryHandle(IRetainable owner, void* pinnedPointer = null,  System.Runtime.InteropServices.GCHandle handle = default(System.Runtime.InteropServices.GCHandle))  { throw null; }
        public void* PinnedPointer { get { throw null; } }
-        public void Dispose()  { throw null; }
+        internal void AddOffset(int offset) { throw null; }


Why does an internal method need to be in the ref?

I suppose it doesn't.

I will remove internal methods in the ref in other places as well.

https://github.com/dotnet/corefx/blob/master/src/System.Memory/ref/System.Memory.cs#L131

https://github.com/dotnet/corefx/blob/master/src/System.Memory/ref/System.Memory.cs#L203

etc.

Are internal constructors treated uniquely? I see a lot of internal constructors in the S.Runtime ref.
https://github.com/dotnet/corefx/blob/master/src/System.Runtime/ref/System.Runtime.cs#L209

There are some other places that have internal APIs in the ref. Should they be there?
https://github.com/dotnet/corefx/blob/master/src/System.Threading/ref/System.Threading.cs#L139

There are some other places that have internal APIs in the ref. Should they be there?

The only reason internal methods would need to be there is if some other library has InternalsVisibleTo access and is accessing them. We've tried to eliminate all such usage, though maybe there are still a few stragglers.

Wouldn't the InternalsVisibleTo apply only to the source projects? Why would that require internals to be listed in the reference assembly?

I mainly see InternalsVisibleTo for the test project related to the library itself: https://github.com/dotnet/corefx/search?p=1&q=InternalsVisibleTo&type=&utf8=%E2%9C%93

But in some cases (like the one I linked), I don't see any library that is accessing this internal API (other than the test):
https://github.com/dotnet/corefx/blob/master/src/System.Threading/ref/System.Threading.cs#L139

public partial class HostExecutionContext : System.IDisposable { public HostExecutionContext() { } public HostExecutionContext(object state) { } protected internal object State { get { throw null; } set { } } public virtual System.Threading.HostExecutionContext CreateCopy() { throw null; } public void Dispose() { } public virtual void Dispose(bool disposing) { } }

However, if I remove it from the ref, I get APICompat error:
error : MembersMustExist : Member 'System.Threading.HostExecutionContext.State.get()' does not exist in the implementation but it does exist in the contract.

Are internal constructors treated uniquely

Yes. The C# compiler generates an implicit public constructor if you do not have any explicit constructor. If you do not want to have a public constructor, you need to have internal one - it is the purpose of the internal constructors in S.Runtime ref.

However, if I remove it from the ref,

What did you remove, just the internal keyword, or the whole property? "protected internal" means "protected or internal", not "protected and internal", so a "protected internal" property is visible outside of the assembly as protected.

If you do not want to have a public constructor, you need to have internal one - it is the purpose of the internal constructors in S.Runtime ref.

Got it. That makes sense.

What did you remove, just the internal keyword, or the whole property? "protected internal" means "protected or internal", not "protected and internal", so a "protected internal" property is visible outside of the assembly as protected.

I had removed the whole property (thinking it was "and"). Thanks for the clarification.

* Adding MemoryHandle AddOffset to fix Memory.Retain impl * Adding AddOffset to System.Runtime ref. * Make the AddOffset API internal.

…x#24323) * Adding MemoryHandle AddOffset to fix Memory.Retain impl * Adding AddOffset to System.Runtime ref. * Make the AddOffset API internal. Commit migrated from dotnet/corefx@b9676be

Adding MemoryHandle AddOffset to fix Memory.Retain impl

e9aaff1

dnfclas added the cla-already-signed label Sep 29, 2017

ahsonkhan mentioned this pull request Sep 29, 2017

Fixing Retain to return MemoryHandle with correct PinnedPointer dotnet/coreclr#14248

Merged

Adding AddOffset to System.Runtime ref.

7c31efb

stephentoub reviewed Sep 29, 2017

View reviewed changes

Make the AddOffset API internal.

e164b02

karelz added the area-System.Memory label Oct 2, 2017

karelz assigned ahsonkhan Oct 2, 2017

ahsonkhan merged commit b9676be into dotnet:master Oct 2, 2017

ahsonkhan deleted the FixRetainBug branch October 2, 2017 21:18

stephentoub reviewed Oct 6, 2017

View reviewed changes

karelz added this to the 2.1.0 milestone Oct 11, 2017

ahsonkhan mentioned this pull request Oct 17, 2017

Remove methods marked as internal from reference assembly #24700

Merged

ahsonkhan mentioned this pull request Jan 31, 2020

Change OwnedMemory Pin to take an optional integer offset dotnet/runtime#24126

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding MemoryHandle AddOffset to fix Memory.Retain impl #24323

Adding MemoryHandle AddOffset to fix Memory.Retain impl #24323

ahsonkhan commented Sep 29, 2017 •

edited

Loading

stephentoub Sep 29, 2017 •

edited

Loading

ahsonkhan Sep 29, 2017

stephentoub Sep 29, 2017

KrzysztofCwalina Sep 29, 2017 •

edited

Loading

ahsonkhan Sep 29, 2017 •

edited

Loading

stephentoub Sep 29, 2017

pakrym Sep 29, 2017

ahsonkhan commented Sep 29, 2017

KrzysztofCwalina commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

KrzysztofCwalina commented Sep 30, 2017

ahsonkhan commented Sep 30, 2017 •

edited

Loading

jkotas commented Sep 30, 2017

ahsonkhan commented Sep 30, 2017

ahsonkhan commented Oct 2, 2017

ahsonkhan commented Oct 2, 2017

ahsonkhan commented Oct 2, 2017

stephentoub Oct 6, 2017

ahsonkhan Oct 6, 2017

stephentoub Oct 6, 2017 •

edited

Loading

ahsonkhan Oct 6, 2017

jkotas Oct 6, 2017 •

edited

Loading

stephentoub Oct 6, 2017

ahsonkhan Oct 6, 2017 •

edited

Loading

Adding MemoryHandle AddOffset to fix Memory.Retain impl #24323

Adding MemoryHandle AddOffset to fix Memory.Retain impl #24323

Conversation

ahsonkhan commented Sep 29, 2017 • edited Loading

stephentoub Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KrzysztofCwalina Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

ahsonkhan Sep 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan commented Sep 29, 2017

KrzysztofCwalina commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

ahsonkhan commented Sep 29, 2017

KrzysztofCwalina commented Sep 30, 2017

ahsonkhan commented Sep 30, 2017 • edited Loading

jkotas commented Sep 30, 2017

ahsonkhan commented Sep 30, 2017

ahsonkhan commented Oct 2, 2017

ahsonkhan commented Oct 2, 2017

ahsonkhan commented Oct 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkotas Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahsonkhan Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

ahsonkhan commented Sep 29, 2017 •

edited

Loading

stephentoub Sep 29, 2017 •

edited

Loading

KrzysztofCwalina Sep 29, 2017 •

edited

Loading

ahsonkhan Sep 29, 2017 •

edited

Loading

ahsonkhan commented Sep 30, 2017 •

edited

Loading

stephentoub Oct 6, 2017 •

edited

Loading

jkotas Oct 6, 2017 •

edited

Loading

ahsonkhan Oct 6, 2017 •

edited

Loading