8160821: VarHandle accesses are penalized when argument conversion is required

[liach]

Since access descriptor is created for each VH operation site, we can optimistically cache the adapted method handle in a site if the site operates on a constant VH. Used a C2 IR test to verify such a setup through an inexact VarHandle invocation can be constant folded through (previously, it was blocked by asType)

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8160821: VarHandle accesses are penalized when argument conversion is required (Enhancement - P3)(⚠️ The fixVersion in this issue is [27] but the fixVersion in .jcheck/conf is 26, a new backport will be created when this pr is integrated.)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/28585/head:pull/28585
$ git checkout pull/28585

Update a local copy of the PR:
$ git checkout pull/28585
$ git pull https://git.openjdk.org/jdk.git pull/28585/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 28585

View PR using the GUI difftool:
$ git pr show -t 28585

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/28585.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back liach! 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]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

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

• build
• core-libs
• graal
• hotspot

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.

[liach]

java.lang.invoke tests all pass. New benchmark results for VarHandleExact:

Benchmark                                 Mode  Cnt  Score   Error  Units
VarHandleExact.exact_exactInvocation      avgt   30  0.380 ± 0.007  ns/op
VarHandleExact.generic_exactInvocation    avgt   30  0.389 ± 0.008  ns/op
VarHandleExact.generic_genericInvocation  avgt   30  0.384 ± 0.008  ns/op

Submitting internal CI runs.

[mlbridge]

Webrevs

• 03: Full - Incremental (d49ad129)
• 02: Full - Incremental (7bcdcbf3)
• 01: Full - Incremental (886d3918)
• 00: Full (522cbe9d)

[liach]

/cc hotspot-compiler

[openjdk]

@liach
The hotspot-compiler label was successfully added.

[liach]

Since I removed the return type dropping VarHandle bypass, TestGetAndAdd became affected because it can no longer access the x86 assembly. Updated the Java calling convention to fix it.

[iwanowww]

Can you elaborate, please, how this method is intended to behave?

[liach]

When this is compiled, constant will become either 1 for constant VH and 2 for non-constant VH. So for constant VH, this becomes a stable read. For a non-constant VH, this becomes getMethodHandle(mode).asType(...), equivalent to before.

[iwanowww]

What's the purpose of constant == MethodHandleImpl.CONSTANT_YES and constant != MethodHandleImpl.CONSTANT_NO checks then?

[liach]

Indeed, I should move the adaptedMh read into constant == MethodHandleImpl.CONSTANT_YES block.

constant != MethodHandleImpl.CONSTANT_NO prevents capturing any further if the VH is known non-constant; we keep this branch in constant case in case the adapted MH is not ready when we know the VH is constant.

[iwanowww]

I still have a hard time reasoning about state transitions of the cache.

1. Why do you limit successful cache read (cache != null) to constant vh case (constant == MethodHandleImpl.CONSTANT_YES)?

2. Why do you avoid cache update in non-constant case (constant != MethodHandleImpl.CONSTANT_NO)? What happens if it runs compiled adaptedMethodHandle method?

[liach]

I am assuming that if C2 determines this vh is not a constant, we can drop it. Is that a right way to move along, or could C2 transition from "not a constant" to "is a constant" during the phases?

[iwanowww]

Sorry, I still don't understand how it is intended to work. Why does MethodHandleImpl.isCompileConstant(vh) == true imply that the cached value is compatible with the constant vh?

// Keep capturing - vh may suddenly get promoted to a constant by C2

Capturing happens outside compiler thread. It is not affected by C2 (except when it completely prunes the whole block).

So, either any captured adaptation is valid/compatible or there's a concurrency issue when C2 kicks in and there's a concurrent cache update happening with incompatible version.

[liach]

any captured adaptation is valid/compatible

Yes, if vh is a constant, any captured adaptation from vh.getMethodHandle(mode).asType(symbolicMethodTypeInvoker) is valid/compatible.

For thread safety, MethodHandle supports safe publication, so I think we are fine publishing this way.

[JornVernee]

Looking at this, I'm not sure we can assume that we only see one mode and type when the VH is constant. There seems to be a lot of non-local reasoning involved.

For example, you could have a var handle invoker created with MethodHandless::varHandleInvoker, which is cached, so the AccessDescriptor can be shared among many different use sites. For an individual use-site, the receiver VH may well be a constant, but that doesn't mean that the cache isn't polluted by the var handle from another use site, as far as I can tell.

The thread safety issue comes from a C2 thread racing to read the lastAdaption cache vs another Java thread writing to the cache. AFAICS, this race is still possible even when vh is a compile time constant.

[JornVernee]

I think even without using an invoker, you could end up in a similar situation if you have something like:

static Object m(VarHandle vh) {
    return vh.get();
}

Which is called by several different threads. At some point this method may be inlined into one of its callees, where vh then becomes a constant. But at the same time, other threads are still writing to the cache.

[iwanowww]

I still find it confusing, especially tri-state logic part.

For background, isCompileConstant was introduced as part of LF sharing effort to get rid of Java-level profiling in optimized code. The pattern is was designed for was:

  if (isCompileConstant(...)) {
    return ...;
  } else {
    ... // do some extra work (either in interpreter, C1, or not-fully-optimized version in C2) 
  }

In this patch, you don't follow that pattern and aadd new state (CONSTANT_PENDING) to distinguish interpreter/C1 from C2. What's the motivation? Why do you want to avoid cache updates coming from C2-generated code?

[liach]

After consulting with @iwanowww, I realized the non-constant status cannot be determined, that the C2 compiled method can even transition from 0 to 1, so I am simplifying this code to only handle the constant case. It seems the getAndAdd IR test no longer fails with this change, and I removed a lot of other redundant changes.

I updated the VarHandleExact benchmark added by @JornVernee, and added a case of dropping return values by changing access mode to getAndAdd consistently. Now they have the following performance numbers:

Benchmark                                        Mode  Cnt  Score   Error  Units
VarHandleExact.exact_exactInvocation             avgt   30  3.843 ± 0.062  ns/op
VarHandleExact.generic_exactInvocation           avgt   30  3.797 ± 0.049  ns/op
VarHandleExact.generic_genericInvocation         avgt   30  3.757 ± 0.034  ns/op
VarHandleExact.generic_returnDroppingInvocation  avgt   30  3.754 ± 0.026  ns/op

[iwanowww]

What about "all-generic" case ( { generic.getAndAdd(data, 42); })?

[liach]

I can change the generic_genericInvocation to an all-generic case.