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

8245459: Add support for complex filter value var handle adaptation #179

Closed

Conversation

@mcimadamore
Copy link
Collaborator

@mcimadamore mcimadamore commented May 20, 2020

It is sometimes necessary to apply a carrier transform on a VarHandle that is expressed in more than just a pair of unary function (e.g. from A->B and B->A). Sometimes the transform needs to act on some bit of external state.

It would be nice if MemoryHandles::filterValue would be enhanced to accept all functions of the kind:

(C... A) ->B
(C... B) -> A

So that the resulting VarHandle will apply carrier transform from A to B and will also add extra coordinate types (C...) which can be used by the transform filters.

To enhance MemoryHandles::filterValue this way regular MH adapters are not enough; while we can implement setters, by combining the filters with MethodHandles::collectArguments there is no way to implement the getters - since that would require a method handle transform which adapts a return value and adds some extra parameters to the adapter handle (which are then forwarded to the filter).

This patch adds the missing method handle primitive to do that; for now it's a package private method, namely MethodHandles::collectReturnValue. If people think that this could be useful, we can evaluate whether to open this up for method handle combinator API as well.

This required to add a new kind of lambda form using the LambdaFormEditor, which was a bit tricky to get right (we currently have no transforms which operates on both arguments and return values, which makes it tricky).

The implementation of MemoryHandles::filterValue is also a bit more complex, since if there are additional coordinates added by collecting arguments/return values, such additional coordinates might be added multiple times, so we need some steps to 'join' the extra coordinates (with a call to MethodHandle::permuteArguments) so that extra coordinate show up only once in the final adapter.

If the filter functions used for adapting are just unary functions, then the implementation falls back to the simpler behavior we had before, and the permutation step is skipped (in such cases, collectArguments == filterArguments and collectReturnValue == filterReturnValue).


Progress

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

Issue

  • JDK-8245459: Add support for complex filter value var handle adaptation

Reviewers

  • Paul Sandoz (psandoz - Committer)

Download

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

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented May 20, 2020

👋 Welcome back mcimadamore! 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 20, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented May 20, 2020

Webrevs

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Well done, that's tricky work with lambda forms.

* @param filter the filter method handle
* @return the adapter method handle
*/
/* package */ static MethodHandle collectReturnValue(MethodHandle target, MethodHandle filter) {

This comment has been minimized.

@PaulSandoz

PaulSandoz May 21, 2020
Member

Do you need to check that the type of last parameter of the filter function is equal to the return type of the target?
Filter parameters size constraints > 0
Can the filter return void?

This comment has been minimized.

@mcimadamore

mcimadamore May 21, 2020
Author Collaborator

yeah - ideally I'd need these checks - in reality the checks are performed on the VH side for now, and this is not exposed publicly, but I can add all required checks.

As for void, yes, for consistency with filterReturnValue we should eventually support that too - but it wasn't the goal of this patch to provide a full blown MH adapter; I have implemented what was necessary for the VH adaptation. I can add the other parts (but would prefer to deal with that separately).

@@ -5474,6 +5474,40 @@ private static void filterReturnValueChecks(MethodType targetType, MethodType fi
throw newIllegalArgumentException("target and filter types do not match", targetType, filterType);
}

/**
* Filter the return value of a target method handle with a filter function. The filter function is

This comment has been minimized.

@PaulSandoz

PaulSandoz May 21, 2020
Member

Suggested improvement to JavaDoc:

Adapts a target method handle by post-processing
its return value and additional arguments (if any) with a filter 
(another method handle).
The result of the filter is returned from the adapter.

If the target returns a value, the filter must accept that value as
as the trailing argument, with the additional arguments proceeding that.
If the target returns void, the filter must accept only the additional arguments.

The return type of the filter
replaces the return type of the target
in the resulting adapted method handle.

The trailing argument type of the filter (if any) must be identical to the
return type of the target.

The additional argument types of the filter (if any) are appended (in order) 
to the argument types of the target method handle and become the argument
types of the resulting adapted method handle.

Here is pseudocode for the resulting adapter. In the code,
{@code T}/{@code t} represent the result type and value of the
{@code target}; {@code V}, the result type of the {@code filter};
{@code A}/{@code a}, the types and values of the parameters and arguments
of the {@code target} as well as the resulting adapter.
{@code B}/{@code b}, the types and values of the additional parameters and arguments
of the {@code filter} as well as appended to the resulting adapter.
...

This comment has been minimized.

@mcimadamore

mcimadamore May 21, 2020
Author Collaborator

Thanks - I'll see how much work it is to add support of the void case - hopefully it won't be too bad.

@mlbridge
Copy link

@mlbridge mlbridge bot commented May 21, 2020

Mailing list message from Paul Sandoz on panama-dev:

Doh, missed that `MethodHandles.collectReturnValue` was package private!

As such I think is good to go.

Paul.

@openjdk
Copy link

@openjdk openjdk bot commented May 21, 2020

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

8245459: Add support for complex filter value var handle adaptation

Reviewed-by: psandoz
  • 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.

There are currently no new commits on the foreign-memaccess branch since the last update of the source branch of this PR. If another commit should be pushed before you perform the /integrate command, your PR will be automatically rebased. If you would like to avoid potential automatic rebasing, specify the current head hash when integrating, like this: /integrate 0dc2fed332fdfdbecbd4008c650d2f0ba77e1e6a.

➡️ To integrate this PR with the above commit message to the foreign-memaccess branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label May 21, 2020
@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 21, 2020

/integrate

@mcimadamore
Copy link
Collaborator Author

@mcimadamore mcimadamore commented May 21, 2020

Pushed as is for now - will consider completing the work on the MH side as a separate PR. Thanks!

@openjdk openjdk bot closed this May 21, 2020
@openjdk openjdk bot added integrated and removed ready labels May 21, 2020
@openjdk
Copy link

@openjdk openjdk bot commented May 21, 2020

@mcimadamore
Pushed as commit f72b574.

@openjdk openjdk bot removed the rfr label May 21, 2020
@mcimadamore mcimadamore deleted the mcimadamore:collectReturnValue branch Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.