Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unsigned adapter handles #173

Closed

Conversation

@ChrisHegarty
Copy link
Member

@ChrisHegarty ChrisHegarty commented May 16, 2020

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 third such.

This change proposes to add a new method:
MemoryHandles::asUnsigned

When dealing with unsigned native data types it is often convenient to model them as
wider signed Java primitives (with appropriate magnitude checks). For example, it is convenient to model an unsigned short as a Java int to avoid dealing with negative values, which would be the case if modeled as a Java short. We do this all the time in the JDK implementation, and it even bleeds into the APIs sometimes.

To model this, I found myself reaching for MemoryHandles::filterValue to do the narrowing and widening, so that my layout class could expose the set of memory handles with appropriate carrier types that the higher-level java code was expecting. This worked fine, but Maurizio pointed out that this is something that the API should probably expose at a slightly higher level. Writing these widening and narrowing adapters using filterValue is not a lot of code, but easy to make mistakes and a place for bugs to hide.

A single static adapter factory, MemoryHandles::asUnsigned(VarHandle target, Class<?> adaptedType), (thanks Maurizio for the suggestion) would be sufficient to provide such functionality. It adapts a target var handle by narrowing incoming values and widening outgoing values, to and from the given type, respectively. When calling set on the resulting var handle, the incoming value (of type adaptedType) is converted by a narrowing primitive conversion and then passed to the target var handle. Conversely, when calling get on the resulting var handle, the returned value obtained from the target var handle is converted by an unsigned widening conversion before being returned to the caller.

We don't necessarily need to, or can, support all combinations, but there seems to be a sweet spot where Java longs and ints can be used to model unsigned char, unsigned short, and unsigned int, [1] which covers the majority of the use-cases. This also seems to align nicely with the primitive wrapper unsigned widening methods, e.g. Byte::toUnsignedInt, Byte::toUnsignedLong, etc.

I started this discussion as a PR so that hopefully the code changes can help convey the idea and inform readers.

Comments welcome.

[1] https://cr.openjdk.java.net/~chegar/foreign/asUnsigned.pdf


Progress

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

Reviewers

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 16, 2020

👋 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.

@openjdk openjdk bot added the rfr label May 16, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 16, 2020

@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 18, 2020

I don't think the source code generation is really needed. It seems like it should be enough to have a set of TO/FROM MethodHandles for each adaptation, and then have the if/else in asUnsigned pick the right pair to pass to filterValue.

Also, the whole implementation could be put in the MemoryHandles class. We do this for asAddressVarHandle as well. No need to put things in j.l.i IMO.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

I think the decisions in your document are sound. One point which is perhaps not stated clearly (but which we have discussed) was also the fact that char is a perfectly decent carrier when users want to work with 16 bits in an unsigned fashion, and that motivates some of the choices we made downstream (e.g. to just support int and long as adapted types).

After looking at the code some more, I agree with Jorn that the templating is perhaps unnecessary. We can go from byte/short/int to wider type using the unsigned helpers on the wrapper types. And we can go reverse by using MethodHandle::explicitCastArguments which does a cast conversion (which in case of primitives e.g. from long to int will do a narrowing, which is exactly what you want).

So I think there's a way for the code to set up some kind of table, and then just pick the adapter MHs from there. I think this will simplify the code quite a bit and remove the j.l.i dependency.

The templating was cute when I started, but as rows in the table were eliminated it no longer carries its own weight.

This update removes the templating. I decided to keep the implementation in its own class for now, so that we can see if we really want to merge it into MemoryHandles. Most of the implementation is not really sharable with other parts of MemoryHandles.
@ChrisHegarty
Copy link
Member Author

@ChrisHegarty ChrisHegarty commented May 18, 2020

Thanks @JornVernee and @mcimadamore for your comments.

The templating was cute when I started, but as rows in the table were eliminated it no longer carries its own weight.

This update removes the templating. I decided to keep the implementation in its own class for now, so that we can see if we really want to merge it into MemoryHandles. Most of the implementation is not really sharable with other parts of MemoryHandles.

Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Overall looks very good - some minor suggestions on the code

private static VarHandle intToUnsignedByte(VarHandle target) {
if (target.varType() != byte.class)
throw new InternalError("expected byte carrier, but got: " + target.varType());
return MemoryHandles.filterValue(target, INT_TO_BYTE, BYTE_TO_UNSIGNED_INT);
}

// int to short
private static VarHandle intToUnsignedShort(VarHandle target) {
if (target.varType() != short.class)
throw new InternalError("expected byte carrier, but got: " + target.varType());
return MemoryHandles.filterValue(target, INT_TO_SHORT, SHORT_TO_UNSIGNED_INT);
}

// long to byte
private static VarHandle longToUnsignedByte(VarHandle target) {
if (target.varType() != byte.class)
throw new InternalError("expected byte carrier, but got: " + target.varType());
return MemoryHandles.filterValue(target, LONG_TO_BYTE, BYTE_TO_UNSIGNED_LONG);
}

// long to short
private static VarHandle longToUnsignedShort(VarHandle target) {
if (target.varType() != short.class)
throw new InternalError("expected byte carrier, but got: " + target.varType());
return MemoryHandles.filterValue(target, LONG_TO_SHORT, SHORT_TO_UNSIGNED_LONG);
}
Copy link
Collaborator

@mcimadamore mcimadamore May 18, 2020

I'm not super sure of the added value of these - the check is essentially a no op, since you already have a switch where, based on the var handle carrier, you decide which method to call. So perhaps we can just inline the right MH filter inside the switch above?

}

private static void checkTargetWiderThanCarrier(Class<?> carrier, Class<?> target) {
if (Wrapper.forPrimitiveType(target).bitWidth() <= Wrapper.forPrimitiveType(carrier).bitWidth()) {
Copy link
Collaborator

@mcimadamore mcimadamore May 18, 2020

Note that MemoryHandles has already a routine to retrieve carrier size - maybe there's some reuse possible there

Copy link
Member

@JornVernee JornVernee left a comment

LGTM. I think it's a great usability feature!

@openjdk
Copy link

@openjdk openjdk bot commented May 18, 2020

@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 unsigned adapter handles

Reviewed-by: mcimadamore, jvernee
  • 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 have been 2 commits pushed to the foreign-memaccess branch:

  • 6450214: 8245227: VarHandle adaptation fails when non-crackable direct method handles are used as adapters
  • e2d2dba: Add benchmark to measure performance of VH adapters

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 645021438aae3c7ae2ea21695445d574787ac0f4.

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 (@mcimadamore, @JornVernee) 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).

@openjdk openjdk bot added the ready label May 18, 2020
@ChrisHegarty
Copy link
Member Author

@ChrisHegarty ChrisHegarty commented May 18, 2020

I am going to review and update the set of values in the test's data providers.

…t of values. Not all combinations are covered, but what is there should be sufficient.
Copy link
Collaborator

@mcimadamore mcimadamore left a comment

Great job

@ChrisHegarty
Copy link
Member Author

@ChrisHegarty ChrisHegarty commented May 19, 2020

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2020

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

@openjdk openjdk bot added the sponsor label May 19, 2020
@JornVernee
Copy link
Member

@JornVernee JornVernee commented May 19, 2020

/sponsor

@openjdk openjdk bot closed this May 19, 2020
@openjdk openjdk bot added integrated and removed sponsor labels May 19, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 19, 2020

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

  • 6450214: 8245227: VarHandle adaptation fails when non-crackable direct method handles are used as adapters
  • e2d2dba: Add benchmark to measure performance of VH adapters

Your commit was automatically rebased without conflicts.

Pushed as commit 3193db4.

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