Part 1 Proposal for JDK-8264594

[leerho]

Thank you for your explanation, and yes, I would like to hear from others
as well.

If I may rewrite your options with more detail:

   1. copyIntoArrayRaw(segment, offsetBytes, long[] dstArray,
   dstIndexLongs, dstCopyLengthLongs)
   2. copyIntoArrayRaw(segment, offsetBytes, long[] dstArray,
   dstIndexLongs, dstCopyLengthLongs)
   And
   copyIntoArrayLogical(segment, srcOffsetLongs, long[] dstArray,
   dstIndexLongs, dstCopyLengthLongs)
   3. copyIntoArrayLogical(segment, srcOffsetLongs, long[] dstArray,
   dstIndexLongs, dstCopyLengthLongs)
   //User has option to write compound statement like:
   copyIntoArrayLogical(segment.asSlice(srcOffsetBytes, copyLengthBytes),
   0, long[] dstArray, dstIndexLongs, dstCopyLengthLongs)
   // I think we discussed a while back that the asSlice would also require
   a length.


For those of us that must deal with complex structs that contain arrays
(not always aligned), the 3rd alternative is extra pain.  I would certainly
be happy with either 1 or 2.

Lee.


On Wed, Jun 9, 2021 at 2:48 PM Maurizio Cimadamore <
maurizio.cimadamore at oracle.com> wrote:

>
> On 09/06/2021 22:15, leerho wrote:
>
> I'm sorry, I must be dense :-)  I don't understand your previous email.
>
>    - Why would one want to avoid a bulk copy? If you have to do a copy, a
>    bulk copy is the fastest way to do it.
>    (i.e., by "bulk" I mean a low level copy a la System.arrayCopy(...),
>    which at the lowest level is just moving bytes,
>    and by that criteria, both of your statements above are "bulk" copies
>    ... :-)   ).
>
> Hard to parse sentence "avoid the need bulk copy **with raw byte offset**"
>
> E.g. as in: you can use slicing to effectively have a way to support bulk
> copy with raw offsets even if you just have bulk copy primitives with
> logical indices.
>
>
>    - Your "logical" rewrite is still specifying the offset in bytes as is
>    the "Raw".  I thought this whole discussion was to create an overloaded
>    "logical" API that would specify the segment offset in array units (in this
>    case, longs).
>
> No, the logical rewrite is accepting an index, which in my example
> happened to be zero. But if the example said "1", it would have meant
> offset of "8" and so forth.
>
>
>    - Both of your statements are missing a length parameter, which would
>    be in longs.
>
> ok
>
> Please, please,  don't discard the portion of the API that allows easy
> copying from/to arrays with a segment offset specified in bytes! I will
> gladly write the "logical" overload methods, if that is what you feel you
> need.  If they are there I will use them, but only rarely.
>
> I'm not discarding anything - I'm merely considering alternate options for
> the design of this API.
>
> The main point I've been trying to get across in this entire thread, is
> that there is an issue when it comes to having mismatched logical vs.
> physical offsets in the same API method. From here, we could go in several
> ways:
>
> * provide only physical offset for segments and live with the mismatch
> (this is your opening proposal)
> * provide both logical and physical-accepting overloads (this is what I
> proposed few weeks ago)
> * provide only logical indices for segments, and ask users to resort to
> slicing when they want to copy things at unaligned offsets (this is what I
> proposed today)
>
> As stated elsewhere, I think I'm ok with starting this exploration at the
> first stop (physical only). But I was mostly trying to proactively shake
> the design space, to see if some other alternative came up, in case the
> logical/physical mismatch doesn't work out great in real world use cases.
>
> I'd also like others (Uwe?) to weigh in, as I'd expect different
> frameworks to have different expectations in what a "Right" API for copying
> arrays would look like. If we are to add something, we should try to find a
> balanced solution.
>
> Maurizio
>
>
>
>
> *****
> Because I don't have your new "copyFrom" I'm going to temporarily mock it
> so I can test the byte-order version as I have the byte-order version of
> the test code ready to test.  (It is a completely different rewrite of the
> test code I've given you so far.)
>
> Lee.
>
>
> On Wed, Jun 9, 2021 at 4:53 AM Maurizio Cimadamore <
> maurizio.cimadamore at oracle.com> wrote:
>
>> Thanks for the background. I think my question was ill-posed, as I
>> realize that even in the cases you describe, you could always avoid the
>> need for a bulk copy with raw byte offset by doing slicing and copy.
>>
>> e.g.
>>
>> copyIntoArrayRaw(segment, byteOffset, long[], arrayIndex)
>>
>> can be rewritten as:
>>
>> copyIntoArrayLogical(segment.asSlice(byteOffset), 0, long[], arrayIndex)
>>
>>
>> So, if we find that having a physical (segment offset) vs. logical (array
>> offset) mismatch is too confusing, we *could* fall back to slicing and
>> using logical offsets only.
>>
>> Maurizio
>>
>>
>> On 08/06/2021 17:54, leerho wrote:
>>
>> Question: do you have concrete use cases where you'd like to copy
>>> contents of a double[] at non-aligned positions inside a segment?
>>
>>
>> Absolutely!   In fact, specifying the offset in bytes is probably the
>> most common.  This is because many of our structs start with a preamble of
>> a mix of byte, short, int, and float fields, and then followed by several
>> data array regions.  For example an array of ints followed by an array of
>> longs, which in some cases may not be unit aligned with the base of the
>> segment.  This can be particularly true of structs that are defined by
>> other users where alignment cannot be guaranteed.   In fact, one of our
>> customers has years of historical data (petabytes) that are not at all byte
>> aligned.  So to assume that everything is neatly byte aligned would be a
>> big mistake.  Unfortunately, many engineers that have created files of data
>> over the years did so without any awareness of the need for
>> byte-alignment.
>>
>> Lee.
>>
>>
>> On Tue, Jun 8, 2021 at 7:00 AM Maurizio Cimadamore <
>> maurizio.cimadamore at oracle.com> wrote:
>>
>>> Hi Lee,
>>> thanks for this, I'll try to put together a PR in the next week or so.
>>>
>>> Re. overloads I was thinking about how the ByteBuffer API goes about
>>> them, and, in I see that that API doesn't have the same issues, given that
>>> ByteBuffer only provides bulk copy for byte arrays. If you want to bulk
>>> copy a double[] you have to convert to a DoubleBuffer first and then copy
>>> (and then use logical indices for the buffer too).
>>>
>>> Question: do you have concrete use cases where you'd like to copy
>>> contents of a double[] at non-aligned positions inside a segment?
>>>
>>> I'm asking because, in principle we could leave the methods in
>>> CopyMemory cover the aligned, logical indices only part of the API. And ask
>>> the user to fall back to MemorySegment::copyFrom if they really really want
>>> unaligned (which should be uncommon?).
>>>
>>> Cheers
>>> Maurizio
>>> On 08/06/2021 00:55, leerho wrote:
>>>
>>> Maurizio,
>>> I have updated MemoryCopy and TestMemoryCopy by extending the variable
>>> names to be even more clear, for example:
>>>
>>>    - srcArr -> srcArray
>>>    - srcSeg -> srcSegment
>>>    - srcCopyLen -> srcCopyLengthInts
>>>    - srcIndex -> srcIndexLongs
>>>    - Etc.
>>>
>>> I also added a comment in the class javadoc describing the difference
>>> between variables with the word "index" and ones with the word "offset",
>>> etc.
>>>
>>> I have looked at what it would take to add overloads that refer to the
>>> segment offset in array element units.  It can be done, but the names of
>>> these new methods will be kinda ugly or awkward:
>>>
>>>    - copyFromArrayAtIndex -- confusing since the array uses index units
>>>    already
>>>    - copyAtIndexFromArray -- not much better
>>>    - copyFromArrayWithIndex -- nah
>>>    - copyFromArrayToSegementUsingIndex -- a little more clear, but
>>>    really awkward
>>>
>>> If you have any better ideas, I am open.  (But IMHO, I don't think we
>>> need this :) )
>>>
>>> Please let me know what the next step should be :)
>>>
>>> Thanks!
>>>
>>> Lee.
>>>
>>> On Fri, Jun 4, 2021 at 7:09 AM leerho <leerho at gmail.com> wrote:
>>>
>>>> Maurizio,
>>>> Thank you for your reply.  I agree with your observation in the
>>>> difference between the single item case and the bulk copy.  I guess I
>>>> became so familiar with how offsets are handled in Unsafe where the single
>>>> element and bulk copy cases the references to memory are always in bytes,
>>>> it seamed so simple and natural.
>>>>
>>>> In my experience anyway, reading and writing data to off-heap almost
>>>> always involves a mix of preamble elements and bulk copy arrays where it
>>>> was necessary to track the offset in bytes, where the multibyte elements
>>>> may not be correctly aligned to the base of the allocated segment, or God
>>>> forbid, the arrays.
>>>>
>>>> What is most important is getting this into the JDK, and if I have to
>>>> add more overloads, I will :)
>>>>
>>>> Lee.
>>>>
>>>> On Fri, Jun 4, 2021 at 2:34 AM Maurizio Cimadamore <
>>>> maurizio.cimadamore at oracle.com> wrote:
>>>>
>>>>>
>>>>> On 04/06/2021 01:32, leerho wrote:
>>>>>
>>>>> When I first saw all the logical index overloads in MemoryAccess I
>>>>> shuddered.  It is a lot of API clutter as far as I’m concerned.  But just
>>>>> because we made that mistake with MemoryAccess doesn’t mean we have to
>>>>> continue propagating it.
>>>>>
>>>>> I'm sorry, I just don't see that as being a "mistake". Code like this:
>>>>>
>>>>> ```
>>>>> for (int i = 0 ; i < limit ; i++) {
>>>>>    long val = MemoryAccess.getLongAtIndex(segment, i);
>>>>>    ...
>>>>> }
>>>>> ```
>>>>>
>>>>> Is _extremely_ common. While it's true that memory segment is an
>>>>> unbiased array of bytes, there are times you want to access it in a more
>>>>> structured form - and saying "you can't do that" because it adds overloads
>>>>> doesn't seem like a great solution to me.
>>>>>
>>>>>
>>>>> Also I have explicitly named the two offsets very differently to try
>>>>> to make it clear that they are very different.  I guess I could go further
>>>>> and name the “srcIndex” -> “srcIndexLongs” for the long array case, for
>>>>> example.  But since “index” is used elsewhere as the logical index I
>>>>> assumed that this would not be necessary.
>>>>>
>>>>> But I would much rather use longer, more explicit names than propagate
>>>>> API clutter.
>>>>>
>>>>> I was thinking overnight that, perhaps, the memory copy and the single
>>>>> element cases are different. When you do single access (see above) you do
>>>>> that frequently in a loop - and you often get elements of the same type. As
>>>>> stated above, asking developers to work in terms of offsets seems a bit
>>>>> odd, when they are really accessing a flat array of element type T.
>>>>>
>>>>> In the case of bulk copy, I don't think there's the same degree of
>>>>> pressure - you will probably call the bulk copy at the beginning, or at the
>>>>> end of your computation (or both ends!) - and having an offset shift is
>>>>> more tolerable.
>>>>>
>>>>> So, I'm open to evaluating something more minimal, and see how far we
>>>>> can get with it.
>>>>>
>>>>> Maurizio
>>>>>
>>>>>
>>>>> Lee
>>>>>
>>>>> On Thu, Jun 3, 2021 at 1:26 PM Maurizio Cimadamore <
>>>>> maurizio.cimadamore at oracle.com> wrote:
>>>>>
>>>>>> Thanks.
>>>>>>
>>>>>> While I agree that a segment is just a bunch of bytes, I think also
>>>>>> think that having an API which takes two indices at the same time, one
>>>>>> logical and one physical is kind of evil. I bet that a lot of developers
>>>>>> will get that wrong, at least the first time.
>>>>>>
>>>>>> What you say about segments being an array of bytes, and thus not
>>>>>> needing index-based overloads can also be said for MemoryAccess class - but
>>>>>> I believe that the index-based accessors in the are quite popular.
>>>>>>
>>>>>> But overall, my worry is not performances, or symmetry with respect
>>>>>> to other APIs - my main worry is what I said above: that people will just
>>>>>> assume that indices are logical for both the segment and the array - so
>>>>>> overloads with clearly different names should help quite a bit IMHO.
>>>>>>
>>>>>>
>>>>>> Maurizio
>>>>>> On 03/06/2021 20:50, leerho wrote:
>>>>>>
>>>>>> Maurizio,
>>>>>> Here is the MemoryCopy class (w/o byte swap) and the TestMemoryCopy
>>>>>> class (w/o testing byte swap).
>>>>>>
>>>>>> I copied from the TestMemoryAccess class in that I noticed that there
>>>>>> is no package statement.  I am not sure what @SuppressWarnings are allowed
>>>>>> in your test environment, I suspect I should remove all of them.
>>>>>>
>>>>>> When the MemorySegment copyFrom(MemorySegment, MemoryLayout,
>>>>>> ByteOrder) becomes available I can add the tests for that.
>>>>>>
>>>>>> *IMHO*
>>>>>>
>>>>>> I feel strongly that there is no need to additionally overload these
>>>>>> methods with the segment offset specified in array index units.  It is so
>>>>>> trivial to convert from one to the other and it can be done easily in the
>>>>>> method argument with a simple multiply by 2, 4 or 8. This should compile
>>>>>> down to a simple shift, which becomes a single cycle CPU instruction.  So
>>>>>> this is not a performance issue.
>>>>>>
>>>>>> Furthermore, it is best that the user becomes accustomed to thinking
>>>>>> of a segment fundamentally as an array of bytes.  Once a segment is loaded
>>>>>> with some primitive array, the segment loses the context of the type of the
>>>>>> array it was loaded with; in effect, a kind of "type erasure" similar to
>>>>>> Java's generics.  This is one of the reasons that MemorySegments can be so
>>>>>> powerful.
>>>>>>
>>>>>>
>>>>>> I have been programming with the "MemorySegment" concept for a number
>>>>>> of years now and find that keeping in mind that segments are just bytes is
>>>>>> very useful.
>>>>>>
>>>>>>
>>>>>> Let me know what you think.
>>>>>>
>>>>>> Lee.
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, May 31, 2021 at 6:56 AM Maurizio Cimadamore <
>>>>>> maurizio.cimadamore at oracle.com> wrote:
>>>>>>
>>>>>>> Hi Lee,
>>>>>>> this looks good.
>>>>>>>
>>>>>>> One thing I note is that there's an ambiguity as to whether the
>>>>>>> segment index is expressed as a logical index, or a raw byte offset. Your
>>>>>>> snippet does the latter. If we want to follow MemoryAccess, perhaps that
>>>>>>> calls for 2 overloads (copyFromArrayAtIndex/copyFromArrayAtOffset), as I
>>>>>>> imagine both could be useful, depending on the case?
>>>>>>>
>>>>>>> The javadoc will have to say something when the segment being used
>>>>>>> is backed by the very array that is the source/target of the copy (we have
>>>>>>> some text like that in MemorySegment::copyFrom).
>>>>>>>
>>>>>>> I think it would be helpful to progress further with this, add the
>>>>>>> remaining templates (w/o ByteOrder, for now) test and see how it works in
>>>>>>> practice.
>>>>>>>
>>>>>>> I will add (or just file a simple PR, so that you can just borrow
>>>>>>> from it - should be a single method) something to do the segment copy with
>>>>>>> swap.
>>>>>>>
>>>>>>> Thanks!
>>>>>>> Maurizio
>>>>>>> On 28/05/2021 22:37, leerho wrote:
>>>>>>>
>>>>>>> Maurizio,
>>>>>>>
>>>>>>> Again sorry about the delay.
>>>>>>>
>>>>>>> Attached is a template proposal for the MemoryCopy class.
>>>>>>>
>>>>>>> I can't complete this without your proposed new copyFrom(...) method
>>>>>>> in MemorySegment. As it is written, it should work, but without the byte
>>>>>>> swap capability.
>>>>>>>
>>>>>>> I can complete the rest of the primitives like this template, if you
>>>>>>> would like with javadocs.  I could also start writing tests, but without
>>>>>>> the byte-swap.
>>>>>>>
>>>>>>> Let me know what would be most helpful.
>>>>>>>
>>>>>>> Cheers,
>>>>>>>
>>>>>>> Lee
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On Thu, May 20, 2021 at 11:36 AM leerho <leerho at gmail.com> wrote:
>>>>>>>
>>>>>>>> Of course!
>>>>>>>>
>>>>>>>> On Thu, May 20, 2021 at 9:42 AM Maurizio Cimadamore <
>>>>>>>> maurizio.cimadamore at oracle.com> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 20/05/2021 17:13, leerho wrote:
>>>>>>>>>
>>>>>>>>> I am not sure if the *dstSegSlice* requires the *srcCopyLen*. I
>>>>>>>>> would hope that it is smart enough to realize that the input length is
>>>>>>>>> smaller than the given offset minus the segment size.
>>>>>>>>>
>>>>>>>>> asSlice has an overload that just takes an offset and infers the
>>>>>>>>> resulting size from there.
>>>>>>>>>
>>>>>>>>> But that doesn't seem what you want here - as you want the slice
>>>>>>>>> to have a specific size (the size of the input array).
>>>>>>>>>
>>>>>>>>> MemorySegment::copyFrom wants the two segments to have the same
>>>>>>>>> size, so I think you need that.
>>>>>>>>>
>>>>>>>>> In terms of performance, there's no difference between
>>>>>>>>> asSlice(offset) and asSlice(offset, size) - you have to create a new
>>>>>>>>> segment anyway.
>>>>>>>>>
>>>>>>>>> Maurizio
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>> From my cell phone.
>>>>>
>>>>> --
>>>> From my cell phone.
>>>>
>>>