Add MemorySegment::fill

[ChrisHegarty]

Hi,

As part of feedback on the Foreign Memory API (when experimenting with
its usage internally in the JDK), a small number of potential usability
enhancements could be made to the API. This is the second such.

This change proposes to add a new method:
MemorySegment::fill

Which fills a value into the given memory segment. Fill can be useful to
initialize or reset the memory of a segment, similar(ish) to memset.

There are obviously different ways to provide such functionality, e.g.
accepting a fill value with a bit width greater than 8 bits, but on
balance this single method will likely satisfy the majority of simple
use-cases. Other more advanced initialization scenarios would likely be
served better from an interaction with the Vector API, or some such.

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed

Reviewers

• Jim Laskey (jlaskey - Committer) ⚠️ Review applies to e66ab85
• Jorn Vernee (jvernee - Committer) ⚠️ Review applies to e66ab85
• Paul Sandoz (psandoz - Committer) ⚠️ Review applies to af894de
• Maurizio Cimadamore (mcimadamore - Committer)

Download

$ git fetch https://git.openjdk.java.net/panama-foreign pull/161/head:pull/161
$ git checkout pull/161

[bridgekeeper]

👋 Welcome back chegar! A progress list of the required criteria for merging this PR into foreign-memaccess will be added to the body of your pull request.

[mlbridge]

Webrevs

• 02: Full - Incremental (1a8ed18)
• 01: Full - Incremental (af894de)
• 00: Full (e66ab85)

[JimLaskey]

Not "fill with null" but "fill null".

[JornVernee]

LGTM!

[openjdk]

@ChrisHegarty This change now passes all automated pre-integration checks, type /integrate in a new comment to proceed. After integration, the commit message will be:

Add MemorySegment::fill

Reviewed-by: jlaskey, jvernee, psandoz, mcimadamore

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /issue command.

Since the source branch of this PR was last updated there has been 1 commit pushed to the foreign-memaccess branch:

• 667f7f0: Simplify MemoryScope implementation to use StampedLock

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid automatic rebasing, please merge foreign-memaccess into your branch, and then specify the current head hash when integrating, like this: /integrate 667f7f0288bc79736c50cdfc2e10945168f1a124.

As you do not have Committer status in this project, an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JimLaskey, @JornVernee, @PaulSandoz, @mcimadamore) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

[PaulSandoz]

Nice and simple, but i think there is an open design question here.

Notice that the bulk copy operation resides on MemoryAddress. Fill is a little like a copy, e.g. if expanded later to fill one segment repeatedly with another (if the destination segment is smaller than the source then it's a truncated copy).

I think we need to consider grouping bulk operations in the same place.

[mcimadamore]

I think we need to consider grouping bulk operations in the same place.

I suggested the location to Chris. Since the method operates on segment it felt right to add it on MemorySegment. If we add it on MemoryAddress it will look a tad odd IMHO, since it works on segments.

We could tweak fill to work on MemoryAddress, but that means that the user has to pass an address and a length (which is what a segment is).

Or we could make fill an instance method on segment.

I'm not sure there's one answer that's better than the others here - but I don't have any strong feelings one way or another, so I'm open to suggestions.

[ChrisHegarty]

I agree with the logic of why this method fits better in MemorySegment, rather than MemoryAddress.

Fill seems more like a "utility", rather than an operation on the segment itself, so I marginally favour a static method.

[PaulSandoz]

I think copy is a little odd where it is located, for the same reasons argued if fill was located on MemoryAddress, since both really operate on segments.

Using MemoryAddress affords some flexibility in terms of offset selection but that can also be achieved by slicing segments.

As separate issues perhaps consider moving or duplicating copy on MemorySegment and exposing a fill accepting a source segment?

[mlbridge]

Mailing list message from John Rose on panama-dev:

On May 13, 2020, at 9:40 AM, Chris Hegarty <chegar at openjdk.java.net> wrote:

On Wed, 13 May 2020 16:25:30 GMT, Maurizio Cimadamore <mcimadamore at openjdk.org> wrote:

Nice and simple, but i think there is an open design question here.

Notice that the bulk `copy` operation resides on `MemoryAddress`. Fill is a little like a copy, e.g. if expanded later
to fill one segment repeatedly with another (if the destination segment is smaller than the source then it's a
truncated copy). I think we need to consider grouping bulk operations in the same place.

I think we need to consider grouping bulk operations in the same place.

I suggested the location to Chris. Since the method operates on segment it felt right to add it on MemorySegment. If we
add it on MemoryAddress it will look a tad odd IMHO, since it works on segments.
We could tweak fill to work on MemoryAddress, but that means that the user has to pass an address and a length (which
is what a segment is).
Or we could make fill an instance method on segment.

I'm not sure there's one answer that's better than the others here - but I don't have any strong feelings one way or
another, so I'm open to suggestions.

I agree with the logic of why this method fits better in MemorySegment, rather than MemoryAddress.

Fill seems more like a "utility", rather than an operation on the segment itself, so I marginally favour a static
method.

We have an odd legacy from Java arrays that we are working with.
Arrays::fill is used to pack a scalar into (a slice of) an array.
But there?s no Arrays::copy. Instead everybody uses System::arraycopy.

For Java arrays, some other designs would seem to be better,
in some hypothetical better world:

1. Add a static overloaded method Array::copy which provides
strongly-typed aliases for System::arraycopy (and deprecate that).

2. Add virtual methods to each array type which turn the receiver into
a source or destination operand: maybe copyIn, copyOut, fillIn.

3. Have a span type which reies a slice of such an array. Add the
virtual methods of 2. (This seems doable in Valhalla.)

I list these out because the MemorySegment work could use these
design patterns more readily than Java could for native arrays.

So, native array : MemorySegment :: System.arraycopy : (what?)
and native array : MemorySegment :: Arrays.fill : (what?)

(I suppose this game can be played with other ju.Arrays methods.)

? John

[mcimadamore]

Thought a bit more about this.

There's an additional angle to the problem of segment vs. address which we didn't discuss. While all segments have a base address, the reverse is not true: if an address is unchecked (e.g. created from MemoryAddress::ofLong) it has no backing segment and can therefore not be dereferenced.

Because of this, one could argue that the signature of the MemoryAddress::copy is overly general, as it allows both checked and unchecked addresses, while only clearly making sense for the former.

A way out would be again, to express copy in terms of segment - and, as John points out, the starting point could be to look at System::arraycopy - e.g. assuming that MemorySegment ~= array:

static void copy(MemorySegment src, long srcOffset, MemorySegment dst, long dstOffset, long length)

This is all and well; now we can only pass segments to the routine, which rules out the unchecked address issue. But still feels a bit redundant - after all, we can easily express offsets in terms of segment slices (which was pointed out by Paul at some point). So we can actually do this:

static void copy(MemorySegment src, MemorySegment dst)

where dst is assumed to be bigger (or equal) than src, where both starting offsets are assumed to be zero and where the length of the copy is assumed to be the length of the src segment.
Since now the signature is expressed in terms of segment, it makes sense to move the method under MemorySegment too.

This is nice, but I think that once we view this method as operating on segments, I think it makes sense to turn them into proper instance methods, and just do:

void copyTo(MemorySegment dst)

Can we do the same with fill? I think so:

void fill(byte value)

Very simple (instance) method.

And, looking a bit ahead, we will want to have a functionality equivalent to Arrays.mismatch - which we can also express as an instance method:

int mismatch(MemorySegment that)

So my recommendation would be for:

1. moving all methods under MemorySegment
2. express all signatures in terms of MemorySegment
3. make them instance methods (rather than static)

This should lead to a more self-consistent, and discoverable API. Thoughts?

[ChrisHegarty]

@mcimadamore I really like this suggestion (instance methods on MemorySegment). Especially given unchecked memory addresses, your proposal to move copy to MemorySegment, will result in the API point being more obviuosly correct by design ( less potential to use incorrectly ). And as you mention, we probably want to get this right now, since mismatch is next on my todo list ;-) ( And mismatch will not work on unchecked addresses )

This design assumes slices are relatively cheap, given that more slicing will be needed to interact with API points, but that should probably be ok.

[PaulSandoz]

Shifting over copy to an instance method on MemorySegment so it can be with its friend fill, is i think the right move. Other friends can join later.

For arrays we never really had any choice (static vs. instance), although i still hold out hope someday we can make arrays implement a richer array'ish interface.

[rose00]

As you might guess from my leading comments, I very much like the idea of
putting side-effecting fill and copy methods onto MS, not statics as in ju.Arrays.
Using instance methods instead of static methods helps the reader make a
mental model of "where it all happens" — in the receiver of the method.

The problem with static methods is all the arguments "look alike", and so
there's a perennial problem of deciding which is the source and which is
the sink. This is not an issue with fill since you know the scalar is the
source, but it's a big problem with copy (cf. System.arraycopy).

If a MS has a utility method for sending data somewhere else, it shouldn't
be called copy but rather copyRange (as in ju.Arrays) or copyOut
or copyTo. I have a very mild preference for copyFrom for the normal
polarity of a copy, which copies into the receiver, but that would tend to
drag in fillFrom or fillWith, which looks like overkill.

My $0.02.

[mlbridge]

Mailing list message from Maurizio Cimadamore on panama-dev:

On 14/05/2020 21:50, John R Rose wrote:

If a MS has a utility method for sending data somewhere else, it shouldn't
be called `copy` but rather `copyRange` (as in ju.Arrays) or `copyOut`
or `copyTo`. I have a very mild preference for `copyFrom` for the normal
polarity of a copy, which copies into the receiver, but that would tend to
drag in `fillFrom` or `fillWith`, which looks like overkill.

Agree - in my comment I think I specifically used `copyTo` and the
argument was the destination. `copy` becomes ambiguous if we use the
instance method style. `copyRange` seems similarly ambiguous. So, for me
it's either `copyFrom` (receiver is the destination) or `copyTo`
(receiver is the thing you want to copy onto the other segment).

I'm not too worried that other methods don't have suffixes, since they
are not ambiguous. Consistency is important, but I think it's one aspect
of the decision. The most important aspect is how readable the code is.
And I think we have made some strides in this discussion to make some
improvements on that front.

[ChrisHegarty]

The instance method ( for fill ) feels much more intuitive ( as can be seem from the test updates ). The moving and renaming of copy will be done in a subsequent PR.

[mcimadamore]

Very good sir, thanks.

[ChrisHegarty]

/integrate

[openjdk]

@ChrisHegarty
Your change (at version 1a8ed18) is now ready to be sponsored by a Committer.

[mcimadamore]

/sponsor

[openjdk]

@mcimadamore @ChrisHegarty The following commits have been pushed to foreign-memaccess since your change was applied:

• 667f7f0: Simplify MemoryScope implementation to use StampedLock

Your commit was automatically rebased without conflicts.

Pushed as commit 2349db7.