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

8254354: Add a withInvokeExactBehavior() VarHandle combinator #843

Closed
wants to merge 19 commits into from

Conversation

JornVernee
Copy link
Member

@JornVernee JornVernee commented Oct 23, 2020

Hi,

This patch adds an asExact() combinator to VarHandle, that will return a new VarHandle that performs exact type checks, similar to MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, which can lead to performance degradation.

This is implemented using a boolean flag in VarForm. If the flag is set, the exact type of the invocation is checked against the exact type in the VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.

Other than that, there is also an asGeneric() combinator added that does the inverse operation (thanks to Rémi for the suggestion). I've also added The @Hidden annotation to the VarHandleGuards methods, as well as a type-checking helper method called from the generic invocation lambda form, so that the stack trace we get points at the location where the VarHandle is being used.

Thanks,
Jorn

CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Testing

Linux x64 Linux x86 Windows x64 macOS x64
Build ✔️ (5/5 passed) ✔️ (2/2 passed) ✔️ (2/2 passed) ✔️ (2/2 passed)
Test (tier1) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed) ✔️ (9/9 passed)

Issue

  • JDK-8254354: Add a withInvokeExactBehavior() VarHandle combinator

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/843/head:pull/843
$ git checkout pull/843

@bridgekeeper
Copy link

bridgekeeper bot commented Oct 23, 2020

👋 Welcome back jvernee! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 23, 2020
@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@JornVernee The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-compiler

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added hotspot-compiler hotspot-compiler-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Oct 23, 2020
@JornVernee
Copy link
Member Author

/label remove hotspot-compiler

@openjdk openjdk bot removed the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 23, 2020
@openjdk
Copy link

openjdk bot commented Oct 23, 2020

@JornVernee
The hotspot-compiler label was successfully removed.

@mlbridge
Copy link

mlbridge bot commented Oct 23, 2020

@forax
Copy link
Member

forax commented Oct 23, 2020

Looks good to me.

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This approach does not work for reference types, since they are erased to Object, and then exact checking will be performed on the erased reference types.

For example try this:

import java.lang.invoke.MethodHandles;
import java.lang.invoke.VarHandle;

public class Test {
    int x;

    public static void main(String[] args) throws Throwable {
        VarHandle x = MethodHandles.lookup().findVarHandle(Test.class, "x", int.class);
        VarHandle ex = x.asExact();
        Test t = new Test();
        ex.set(t, 1);
    }
}

Which results in:

Exception in thread "main" java.lang.invoke.WrongMethodTypeException: expected (Object,int)void but found (Test,int)void
	at Test.main(Test.java:11)

Exact type checking requires that match be performed on the VH access mode method type and the exact symbolic method type, something like:

    final static Object guard_L_L(VarHandle handle, Object arg0, VarHandle.AccessDescriptor ad) throws Throwable {
        if (handle.vform.exact && handle.accessModeType(ad.type) != ad.symbolicMethodTypeExact) {
            throw new WrongMethodTypeException("expected " + handle.vform.methodType_table_exact[ad.type] + " but found "
                    + ad.symbolicMethodTypeExact);
        }

Then it's precisely the same as MH.invokeExact, rather than MH.invoke.

A VarForm is a resource shared across many instances of the same kind of VarHandle, so cannot be used for exact matching, except in specific scenarios e.g. access on a primitive array.

@JornVernee
Copy link
Member Author

@PaulSandoz Thanks. I initially tested this with memory access VarHandles, which don't erase the receiver type. e.g.

MemoryLayout layout = MemoryLayout.ofSequence(10, JAVA_INT);
VarHandle vh = layout.varHandle(int.class, MemoryLayout.PathElement.sequenceElement());
vh = vh.asExact();
try (MemorySegment segment = MemorySegment.allocateNative(layout)) {
    for (int i = 0; i <10; i++) {
        vh.set(segment.baseAddress(), i, i);
    }
}

Will result in:

Exception in thread "main" java.lang.invoke.WrongMethodTypeException: expected (MemoryAddress,long,int)void but found (MemoryAddress,int,int)void
	at java.base/java.lang.invoke.VarHandleGuards.guard_LII_V(VarHandleGuards.java:915)
	at main.Main.main(Main.java:18)

Which led me to believe the approach would work for other reference types. But, I suppose the MethodTypes fed to memaccess VarForms are non-erased as an exception rather than a rule.

I'll update the patch and sharpen the tests to check that the actual expected type is correct (per the exception message).

…atter are shared. Use handle.accessModeType to get the exact type of the VarHandle.
@openjdk openjdk bot removed the rfr Pull request is ready for review label Oct 23, 2020
@openjdk openjdk bot added the rfr Pull request is ready for review label Oct 23, 2020
@JornVernee
Copy link
Member Author

JornVernee commented Oct 23, 2020

@PaulSandoz I've implemented your suggestion, by moving the exact flag to VarHandle itself. FWIW, the VH::accessModeType method took an AccessMode value as an argument, and the AccessDescriptor only stored the ordinal, so I added an @Stable array of values to AccessMode to map from ordinal to enum value. But, maybe it makes more sense to just store the AccessMode in the AccessDescriptor directly? (instead of the ordinal). Not sure what the original motivation was for only storing the ordinal.

I've also sharpened the tests to check the exception message. Do you think the testing is sufficient? (Note that I did not add tests to the template files since only a select set of argument type conversions causes the WMTE we're looking for. So, that's why I created a new test file).

FWIW, there seems to have been a bug in the implementation of IndirectVarHandle::accessModeTypeUncached, where it was using the VarHandle's type as the receiver argument (unlike all the other impls). I've fixed this by passing null instead, and also removed a workaround for this problem in VarHandles::maybeAdapt.

@JornVernee
Copy link
Member Author

/csr needed

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label Oct 24, 2020
@openjdk
Copy link

openjdk bot commented Oct 24, 2020

@JornVernee has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@JornVernee please create a CSR request and add link to it in JDK-8254354. This pull request cannot be integrated until the CSR request is approved.

JornVernee added 2 commits Oct 26, 2020
- Make isExact() public
@JornVernee
Copy link
Member Author

JornVernee commented Oct 26, 2020

I've updated the javadoc, and added two benchmarks that show the existing discrepancy between an exact and a generic use of a VarHandle, as well as showing that an exact VarHandle is as fast as a generic VarHandle for an exact invocation. (1 benchmark for normal Java field access, and 1 benchmark for the foreign memory-access API).

Benchmark                                                             Mode  Cnt   Score   Error  Units
o.o.b.java.lang.invoke.VarHandleExact.exact_exactInvocation           avgt   30   0.729 □ 0.010  ns/op
o.o.b.java.lang.invoke.VarHandleExact.generic_exactInvocation         avgt   30   0.735 □ 0.019  ns/op
o.o.b.java.lang.invoke.VarHandleExact.generic_genericInvocation       avgt   30  14.696 □ 0.498  ns/op
o.o.b.jdk.incubator.foreign.VarHandleExact.exact_exactInvocation      avgt   30   2.274 □ 0.012  ns/op
o.o.b.jdk.incubator.foreign.VarHandleExact.generic_exactInvocation    avgt   30   2.278 □ 0.015  ns/op
o.o.b.jdk.incubator.foreign.VarHandleExact.generic_genericInvocation  avgt   30  24.759 □ 0.367  ns/op

@PaulSandoz
Copy link
Member

PaulSandoz commented Oct 26, 2020

The direct use of the enum ordinal is because HotSpot accessing it from the enum value is (or was) not optimal in C2.

You can avoid the addition of the stable array by doing the following:

    public final MethodType accessModeType(AccessMode accessMode) {
        return accessModeType(accessMode.at.ordinal());
    }

    @ForceInline
    final MethodType accessModeType(int accessModeOrdinal) {
        TypesAndInvokers tis = getTypesAndInvokers();
        MethodType mt = tis.methodType_table[accessModeOrdinal];
        if (mt == null) {
            mt = tis.methodType_table[accessModeOrdinal] =
                    accessModeTypeUncached(accessModeOrdinal);
        }
        return mt;
    }

    final MethodType accessModeTypeUncached(int accessModeOrdinal) {
        return accessModeTypeUncached(AccessMode.values()[accessModeOrdinal]);
    }

It's a little odd going back and forth between the ordinal and the enum value, but it's all on the slow uncached path, and it avoids some duplication of code.

I'll take a closer looks at the other areas and respond in another comment.

@PaulSandoz
Copy link
Member

We can clarify the new methods and tie them closer to method handle semantics. I suggest the names asExactInvoker and asInvoker, referencing MethodHandles.exactInvoker and MethodHandles.invoker respectively. (The term "generic" is really reserved for erased method types, and otherwise used internally as the generic invoker.)

The VarHandle documentation already specifies that:

 * <p>
 * Invocation of an access mode method behaves as if an invocation of
 * {@link MethodHandle#invoke}, where the receiving method handle accepts the
 * VarHandle instance as the leading argument.  More specifically, the
 * following, where {@code {access-mode}} corresponds to the access mode method
 * name:
 * <pre> {@code
 * VarHandle vh = ..
 * R r = (R) vh.{access-mode}(p1, p2, ..., pN);
 * }</pre>
 * behaves as if:
 * <pre> {@code
 * VarHandle vh = ..
 * VarHandle.AccessMode am = VarHandle.AccessMode.valueFromMethodName("{access-mode}");
 * MethodHandle mh = MethodHandles.varHandleExactInvoker(
 *                       am,
 *                       vh.accessModeType(am));
 *
 * R r = (R) mh.invoke(vh, p1, p2, ..., pN)
 * }</pre>
 * (modulo access mode methods do not declare throwing of {@code Throwable}).
...

We will need to adjust this section, i can help, but first let us reach agreement on this approach.

@JornVernee
Copy link
Member Author

I think asInvoker and asExactInvoker make sense if you think of a VarHandle as an invoker of MethodHandles (though this is more of an implementation detail). But, I feel like the name asInvoker isn't obvious enough for what it does. i.e. it's not that obvious from the name that this removes the exactness.

If asGeneric is problematic, maybe just asNonExact works?

@JornVernee
Copy link
Member Author

I've uploaded another revision that has the suggested javadoc changes, courtesy of Paul. We also decided to rename asExact() and asGeneric() to withInvokeExactBehaviour() and withInvokeBehaviour() (with links to the relevant javadoc sections).

Copy link
Member

@PaulSandoz PaulSandoz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. There is just one difference between the spec and implementation. The spec states:

If this VarHandle already has invoke{-exact} behaviour this VarHandle is returned.

I prefer this behaviour, but feel free to update the spec if you like e.g. If ... already has XXX then a new VH with exactly the same behaviour as this VH is returned.

I just realized i used "behaviour", the english spelling. We should use the US spelling, "behavior", which is more commonly used throughout the JDK documentation.

JornVernee added 2 commits Oct 30, 2020
- Return same VarHandle instance when instance is already exact/non-exact
@JornVernee JornVernee changed the title 8254354: Add an asExact() VarHandle combinator 8254354: Add an withInvokeExactBehavior() VarHandle combinator Oct 30, 2020
@JornVernee JornVernee changed the title 8254354: Add an withInvokeExactBehavior() VarHandle combinator 8254354: Add a withInvokeExactBehavior() VarHandle combinator Oct 30, 2020
@openjdk openjdk bot removed the csr Pull request needs approved CSR before integration label Nov 3, 2020
@openjdk
Copy link

openjdk bot commented Nov 3, 2020

@JornVernee This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8254354: Add a withInvokeExactBehavior() VarHandle combinator

Reviewed-by: psandoz, chegar

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 40 new commits pushed to the master branch:

  • a7f4691: 8244088: [Regression] Switch of Gnome theme ends up in deadlocked UI
  • bd3e65b: 8256052: Remove unused allocation type from fieldInfo
  • 6d8acd2: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
  • 643969a: 8255822: Zero: improve build-time JVMTI handling
  • 6ae5e5b: 8221404: C2: Convert RegMask and IndexSet to use uintptr_t
  • 6555996: 8253600: G1: Fully support pinned regions for full gc
  • 97d6e4a: 8256046: Shenandoah: Mix-in NULL_PTR in non-strong ShLRBNode's type
  • a1d4b9f: 8256009: Remove src/hotspot/share/adlc/Test/i486.ad
  • 3455fa9: 8256050: JVM crashes with -XX:+PrintDeoptimizationDetails
  • e6df13e: 8256054: C2: Floating-point min/max operations on vectors intermittently produce wrong results for NaN values
  • ... and 30 more: https://git.openjdk.java.net/jdk/compare/688b10b97055ec83361aa7645a707204d35dd3af...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

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

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Nov 3, 2020
JornVernee added 2 commits Nov 6, 2020
- Fix typo
- add specification about return value of hasInvokeExactBehaviour
@JornVernee
Copy link
Member Author

@ChrisHegarty Thanks for the comments. I've implemented them.

Copy link
Member

@ChrisHegarty ChrisHegarty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

The test could be sprinkled with a number of assertions related to the invocation behaviour, a.k.a hasInvokeExactBehavior; either the newly specified default behaviour assertFalse(vh. hasInvokeExactBehavior()), or post switching to exact: assertTrue(vh. hasInvokeExactBehavior())

@JornVernee
Copy link
Member Author

@ChrisHegarty Good suggestion! Added.

@JornVernee
Copy link
Member Author

/integrate

@openjdk openjdk bot closed this Nov 10, 2020
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Nov 10, 2020
@openjdk
Copy link

openjdk bot commented Nov 10, 2020

@JornVernee Since your change was applied there have been 43 commits pushed to the master branch:

  • d6f1463: 8233332: Need to create exploded tests covering all forms of modules
  • f2a0bf3: 8256017: Remove unused elapsedTimer constructor
  • 7d4e86b: 8138588: VerifyMergedCPBytecodes option cleanup needed
  • a7f4691: 8244088: [Regression] Switch of Gnome theme ends up in deadlocked UI
  • bd3e65b: 8256052: Remove unused allocation type from fieldInfo
  • 6d8acd2: 8256066: Tests use deprecated TestNG API that is no longer available in new versions
  • 643969a: 8255822: Zero: improve build-time JVMTI handling
  • 6ae5e5b: 8221404: C2: Convert RegMask and IndexSet to use uintptr_t
  • 6555996: 8253600: G1: Fully support pinned regions for full gc
  • 97d6e4a: 8256046: Shenandoah: Mix-in NULL_PTR in non-strong ShLRBNode's type
  • ... and 33 more: https://git.openjdk.java.net/jdk/compare/688b10b97055ec83361aa7645a707204d35dd3af...master

Your commit was automatically rebased without conflicts.

Pushed as commit 0a41ca6.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

openjdk-notifier bot referenced this pull request Nov 10, 2020
@JornVernee JornVernee deleted the Exact_VarHandle branch Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core-libs core-libs-dev@openjdk.org integrated Pull request has been integrated
4 participants