S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte #13848

nietras · 2016-11-21T08:28:48Z

Adds CopyBlock/CopyBlockUnaligned to Unsafe for ref parameters as discussed in https://github.com/dotnet/corefx/issues/13427

Implements the generic versions of this, if this is not wanted, it can be changed to e.g. ref byte. Caller must know NOT to call this with a T containing references.

@jkotas @atsushikan @benaadams @jamesqo @omariom

UPDATE: For API review, basically the choice is whether to add any of the below or perhaps both. Or in any shape that FXDC finds suitable. The below has been chosen to fit existing API definitions. The functionality is needed for Span<T> and other ref based scenarios for e.g. optimal performance.

void CopyBlock<T>(ref T destination, ref T source, int elementCount);
void CopyBlockUnaligned<T>(ref T destination, ref T source, int elementCount);

OR

void CopyBlock(ref byte destination, ref byte source, uint byteCount);
void CopyBlock(ref byte destination, ref byte source, UIntPtr byteCount);
void CopyBlockUnaligned(ref byte destination, ref byte source, uint byteCount);
void CopyBlockUnaligned(ref byte destination, ref byte source, UIntPtr byteCount);

I will change the implementation once I get word of the result of the API review. If needed I can create a separate issue for API review, but since this change is rather small I am hoping to keep it here.

Personally, I think adding both would be great. Not the least since the generic version covers the most common use case for the work I do, and because it fits Span<T>.

jamesqo · 2016-11-21T16:53:56Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.xml

@@ -186,6 +194,15 @@
            <param name="source">The source address to copy from.</param>
            <param name="byteCount">The number of bytes to copy.</param>
        </member>
+        <member name="M:System.Runtime.CompilerServices.Unsafe.CopyBlockUnaligned``1(``0@,``0@,System.Int32)">
+            <summary>
+            Copies elements from the source address to the destination address.


Nit: use a comma. Also, the specified?

jamesqo · 2016-11-21T16:54:40Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.xml

@@ -168,6 +168,14 @@
            <param name="source">The source address to copy from.</param>
            <param name="byteCount">The number of bytes to copy.</param>
        </member>
+        <member name="M:System.Runtime.CompilerServices.Unsafe.CopyBlock``1(``0@,``0@,System.Int32)">
+            <summary>
+            Copies elements from the source address to the destination address.


Should this be the specified number of elements?

karelz · 2016-11-21T17:02:37Z

We need API approval first: see https://github.com/dotnet/corefx/blob/master/Documentation/project-docs/api-review-process.md

@atsushikan @AlexGhiondea @joperezr can you please bring it for API review if you agree with the API shape?

jkotas · 2016-11-21T17:13:59Z

Implements the generic versions of this, if this is not wanted, it can be changed to e.g. ref byte. Caller must know NOT to call this with a T containing references.

I am not sure whether this trap is a good idea. I would be more confortable with non-generic version that takes ref byte.

ghost · 2016-11-21T17:24:03Z

If you change it to "ref byte", we'll need overloads that take long or ulong "counts". Span's element length is restricted to 2gb, but its byte length isn't.

jkotas · 2016-11-21T17:31:37Z

The existing CopyBlock overloads that take void* have length as either uint or UIntPtr. The new ones that would take ref byte should match them.

…ination, ref T source, int elementCount)

…ith ref byte versions and two overloads for byteCount. Add similar InitBlock/InitBlockUnaligned for (ref byte startAddress, byte value, uint/UIntPtr byteCount).

nietras · 2016-11-29T20:04:30Z

UPDATE: API has not been approved yet!
I have updated per API approved by FXDC in https://github.com/dotnet/corefx/issues/13427

@jkotas @jamesqo @benaadams @omariom please have a look. One thing is whether "address" is a suitable parameter name for refs e.g. is "startAddress" a good name? Would "startLocation" be better.

jkotas · 2016-11-29T21:59:41Z

I have updated per API approved by FXDC

Sorry for the misunderstanding. The comment I have added was just my response to offline question on whether I am fine with the API. I do not think they had time to look at this one yet - ready-for-review label was not flipped.

"startAddress" a good name?

It is not ideal, but I would opt for consistency with the void* overload.

karelz · 2016-11-29T22:10:59Z

I do not think they had time to look at this one yet - ready-for-review label was not flipped.

That's correct, we had only 30 mins this week (I learned yesterday). Next Tuesday we will have 2 hours. Sorry for the delay.

nietras · 2016-11-30T07:37:44Z

[@jkotas] Sorry for the misunderstanding.

Yeah, I completely misunderstood that :)

@karelz A @dotnet/fxdc would be nice to have for directing comments at the FXDC team, in fact more team mentions would be nice for a lot of things e.g. codegen, jit, etc. :) No problem, the code can easily be changed, biggest time consumer is getting repo sync'ed and running build.cmd which takes a long long time...

It is not ideal, but I would opt for consistency with the void* overload.

Yes that is why I kept the name. So fine with me.

karelz · 2016-11-30T17:22:54Z

@nietras How exactly do you think that team names (incl. fxdc) would help? What is missing in current issue workflow or api design workflow?

We have area experts/owners monitoring each area. While it is not perfect system, it is IMO much better than to rely on team mentions. I grant you team names would be useful on occasion (however it is rather rare).

fxdc intentionally leaves discussions to meetings (GH/email discussions tend to create lots of back-and-forth and are not super efficient ... disclaimer: It is something I am still getting used to myself and am not 100% sure I fully agree with).
We expect area experts/owners to drive the first-level API approval and then submit to fxdc (see API review process).

jkotas · 2016-11-30T17:45:10Z

How exactly do you think that team names (incl. fxdc) would help?

One would be able to find out who are the actual human beings that form fxdc, making the api approval process more transparent.
One would be able to address the group - e.g. in the above example that caused the confusion, I would be able to write "@dotnet/fxdc proposal LGTM" that makes it clear that I am telling something to fxdc. Having an alias for the group also allows one to do better filtering.

For example, CoreCLR has "dotnet/jit-contrib" that has people working on the JIT. There are area experts for register allocator, inliner, exceptions, etc. But it is useful to have a way to address all people that work on the JIT.

fxdc intentionally leaves discussions to meetings

Having alias does not prevent you from having in person meetings. It is complementary.

offline discussions tend to create lots of back-and-forth

You meant online discussions, right?

nietras · 2016-12-01T10:25:58Z

I would be able to write "@dotnet/fxdc proposal LGTM" that makes it clear that I am telling something to fxdc

Precisely, this was what triggered my request.

fxdc intentionally leaves discussions to meetings

I think this is completely understandable you don't have to look far (HashCode https://github.com/dotnet/corefx/issues/8034 ;) to see that these discussions can keep going for a long time, at some point the key persons responsible for this, need to make a decision.

However, the process would then be that an API proposal is reviewed/drafted for some duration, until a point where the API is thought final enough that FXDC could examine it and then you mention FXDC to ask them to review it in a meeting. So... as @jkotas says:

Having alias does not prevent you from having in person meetings. It is complementary.

I agree completely with this. It is a means to allow easier flagging and collaboration. Also as a newbie it is often hard to know who are who and when to mention or not. with @dotnet you would get intellisense and it would be easier to mention or find out who could be mentioned for this.

I understand this is a two edged sword. It might result in way too much mentioning (something a newbie is always concerned about since you don't want to cause to much trouble). Ideally, it would have been good if a team mention could be guarded as well, so only say a select group of people could mention `@dotnet/fxdc" for example.

nietras · 2016-12-07T11:07:25Z

@karelz I assume this wasn't reviewed at the last FXDC meeting? A PR for Span<T>.Fill(T value) from me would depend on this also, like Clear().

karelz · 2016-12-07T17:04:09Z

@nietras yep, we didn't have all information in API review yesterday to make the call. We plan to grab @jkotas next Tuesday and review it with him together.
Sorry for the delay.

nietras · 2016-12-13T18:29:03Z

@jkotas will you merge this if you think it is good?

cc @karelz

jkotas · 2016-12-13T19:14:16Z

We have just noticed that the IL spec says that the size for these IL instructions is unsigned int32. Do the overloads that take native int actually work well?

nietras · 2016-12-13T19:23:10Z

The tests passed, when I made the PR. Not sure about the asm generated though.

nietras · 2016-12-13T20:20:51Z

Note that no tests go beyond uint number if bytes. But tests work on 64-bit with UIntPtr.

mellinoe · 2016-12-13T20:24:53Z

Should we keep those (native int) overloads even though the specification does not mention them? It's not clear to me that it's valid, and other implementations might not allow it.

mellinoe · 2016-12-13T20:25:40Z

Note that there does seem to be an existing CopyBlock overload that was added a couple months ago which also uses a UIntPtr size parameter. We should consider removing it as well.

nietras · 2016-12-13T20:56:18Z

Currently there are the following using UIntPtr:

void CopyBlock(void* destination, void* source, native unsigned int byteCount)
void CopyBlockUnaligned(void* destination, void* source, native unsigned int byteCount)
void InitBlock(void* startAddress, uint8 'value', native unsigned int byteCount)
void InitBlockUnaligned(void* startAddress, uint8 'value', native unsigned int byteCount)

Sounds reasonable to remove them if not supported by the standard. Not sure if any depend on them yet.

jkotas · 2016-12-13T22:17:41Z

Yes, I think it would be a good idea to remove them.

They have not shipped in a stable package yet, so it is safe to remove them.

… taking UIntPtr as byteCount.

nietras · 2016-12-14T21:11:25Z

@jkotas removed UIntPtr overloads both new and existing.

jkotas · 2016-12-14T21:41:18Z

src/System.Runtime.CompilerServices.Unsafe/src/System.Runtime.CompilerServices.Unsafe.xml

+            <param name="source">The source address to copy from.</param>
+            <param name="byteCount">The number of bytes to copy.</param>
+        </member>
+        <member name="M:System.Runtime.CompilerServices.Unsafe.CopyBlockUnaligned(System.Byte@,System.Byte@,System.UIntPtr)">


The xml doc file has the docs for the UIntPtr versions. Could you please remove them there too?

… for CopyBlock*.

nietras · 2016-12-15T08:19:26Z

@jkotas not sure how I missed those xml docs, but hope I got them all now. PTAL.

jkotas · 2016-12-15T14:22:03Z

Thank you!

…ckUnaligned for ref byte (dotnet/corefx#13848) * S.R.CS.Unsafe: Add CopyBlock<T>/CopyBlockUnaligned<T> for (ref T destination, ref T source, int elementCount) * S.R.CS.Unsafe: Remove new and existing InitBlock/CopyBlock* overloads taking UIntPtr as byteCount. Commit migrated from dotnet/corefx@60084eb

dnfclas added the cla-already-signed label Nov 21, 2016

jamesqo reviewed Nov 21, 2016

View reviewed changes

karelz added the area-System.Runtime.CompilerServices label Nov 21, 2016

karelz assigned nietras Nov 21, 2016

karelz added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Nov 21, 2016

karelz assigned AlexGhiondea and joperezr Nov 21, 2016

jkotas assigned ghost and unassigned AlexGhiondea and joperezr Nov 21, 2016

AlexGhiondea added the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Nov 21, 2016

nietras changed the title ~~S.R.CS.Unsafe: Add CopyBlock<T>/CopyBlockUnaligned<T> for (ref T destination, ref T source, int elementCount)~~ [WIP] S.R.CS.Unsafe: Add CopyBlock<T>/CopyBlockUnaligned<T> for (ref T destination, ref T source, int elementCount) Nov 21, 2016

nietras added 2 commits November 29, 2016 19:49

S.R.CS.Unsafe: Add CopyBlock<T>/CopyBlockUnaligned<T> for (ref T dest…

21e02df

…ination, ref T source, int elementCount)

S.R.CS.Unsafe: Fix xml doc per feedback, change unit test to use short.

f3e98f7

nietras force-pushed the unsafe-cpblk-ref branch from 6f574a6 to f3e98f7 Compare November 29, 2016 18:49

S.R.CS.Unsafe: Replacing generic CopyBlock<T>/CopyBlockUnaligned<T> w…

fdb0eae

…ith ref byte versions and two overloads for byteCount. Add similar InitBlock/InitBlockUnaligned for (ref byte startAddress, byte value, uint/UIntPtr byteCount).

nietras changed the title ~~[WIP] S.R.CS.Unsafe: Add CopyBlock<T>/CopyBlockUnaligned<T> for (ref T destination, ref T source, int elementCount)~~ S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte Nov 29, 2016

karelz removed the api-needs-work API needs work before it is approved, it is NOT ready for implementation label Nov 29, 2016

nietras changed the title ~~S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte~~ [WIP] S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte Nov 30, 2016

karelz modified the milestone: 1.2.0 Dec 6, 2016

nietras mentioned this pull request Dec 8, 2016

Add Span Fill and Clear methods dotnet/coreclr#8498

Closed

nietras changed the title ~~[WIP] S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte~~ S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte Dec 13, 2016

nietras changed the title ~~S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte~~ [WIP] S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte Dec 13, 2016

S.R.CS.Unsafe: Remove new and existing InitBlock/CopyBlock* overloads…

3cec098

… taking UIntPtr as byteCount.

jkotas reviewed Dec 14, 2016

View reviewed changes

S.R.CS.Unsafe: Remove xml docs referring to removed UIntPtr overloads…

d33231b

… for CopyBlock*.

jkotas changed the title ~~[WIP] S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte~~ S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte Dec 15, 2016

jkotas removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Dec 15, 2016

jkotas approved these changes Dec 15, 2016

View reviewed changes

jkotas merged commit 60084eb into dotnet:master Dec 15, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte #13848

S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte #13848

nietras commented Nov 21, 2016 •

edited

Loading

jamesqo Nov 21, 2016 •

edited

Loading

jamesqo Nov 21, 2016

karelz commented Nov 21, 2016 •

edited

Loading

jkotas commented Nov 21, 2016

ghost commented Nov 21, 2016

jkotas commented Nov 21, 2016 •

edited

Loading

nietras commented Nov 29, 2016 •

edited

Loading

jkotas commented Nov 29, 2016

karelz commented Nov 29, 2016

nietras commented Nov 30, 2016

karelz commented Nov 30, 2016 •

edited

Loading

jkotas commented Nov 30, 2016

nietras commented Dec 1, 2016

nietras commented Dec 7, 2016

karelz commented Dec 7, 2016

nietras commented Dec 13, 2016

jkotas commented Dec 13, 2016

nietras commented Dec 13, 2016

nietras commented Dec 13, 2016

mellinoe commented Dec 13, 2016

mellinoe commented Dec 13, 2016 •

edited

Loading

nietras commented Dec 13, 2016

jkotas commented Dec 13, 2016

nietras commented Dec 14, 2016

jkotas Dec 14, 2016 •

edited

Loading

nietras commented Dec 15, 2016

jkotas commented Dec 15, 2016

S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte #13848

S.R.CS.Unsafe: Add CopyBlock/CopyBlockUnaligned and InitBlock/InitBlockUnaligned for ref byte #13848

Conversation

nietras commented Nov 21, 2016 • edited Loading

jamesqo Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

jamesqo Nov 21, 2016

Choose a reason for hiding this comment

karelz commented Nov 21, 2016 • edited Loading

jkotas commented Nov 21, 2016

ghost commented Nov 21, 2016

jkotas commented Nov 21, 2016 • edited Loading

nietras commented Nov 29, 2016 • edited Loading

jkotas commented Nov 29, 2016

karelz commented Nov 29, 2016

nietras commented Nov 30, 2016

karelz commented Nov 30, 2016 • edited Loading

jkotas commented Nov 30, 2016

nietras commented Dec 1, 2016

nietras commented Dec 7, 2016

karelz commented Dec 7, 2016

nietras commented Dec 13, 2016

jkotas commented Dec 13, 2016

nietras commented Dec 13, 2016

nietras commented Dec 13, 2016

mellinoe commented Dec 13, 2016

mellinoe commented Dec 13, 2016 • edited Loading

nietras commented Dec 13, 2016

jkotas commented Dec 13, 2016

nietras commented Dec 14, 2016

jkotas Dec 14, 2016 • edited Loading

Choose a reason for hiding this comment

nietras commented Dec 15, 2016

jkotas commented Dec 15, 2016

nietras commented Nov 21, 2016 •

edited

Loading

jamesqo Nov 21, 2016 •

edited

Loading

karelz commented Nov 21, 2016 •

edited

Loading

jkotas commented Nov 21, 2016 •

edited

Loading

nietras commented Nov 29, 2016 •

edited

Loading

karelz commented Nov 30, 2016 •

edited

Loading

mellinoe commented Dec 13, 2016 •

edited

Loading

jkotas Dec 14, 2016 •

edited

Loading