Discuss introducing an @OpenJ9IntrinsicCandidate annotation for JIT accelerated APIs

[fjeremic]

Inspired by [1] OpenJDK currently annotates methods which have intrinsics with a @HotSpotIntrinsicCandidate annotation in the Java code to warn the reader that the method they are looking at has special consideration in the JVM.

I'm finding that keeping track of methods which we accelerate in OpenJ9 to be hard as the list is rather huge [2]. This is problematic when looking at some of the clean-room implemented classes in OpenJ9 such as String, Class, Thread, etc. where having JIT intrinsic for performance is quite common to inform the reader to be careful about modifying the signatures and semantics of such methods.

I wanted to open up a discussion on the possibility of introducing an @OpenJ9IntrinsicCandidate annotation that will be placed on all methods in classes implemented by OpenJ9 that the JIT may accelerate.

The only downside I see is that we will not have such annotation on methods the JIT may accelerate which we get from OpenJDK, but we can't modify those sources at the moment anyway so it is not much for a cause for concern.

[1] https://bugs.openjdk.java.net/browse/JDK-8076112
[2] https://github.com/eclipse/openj9/blob/365ef6a59642bd6bee98be7ce3eb2d20adf17a3f/runtime/compiler/codegen/J9RecognizedMethodsEnum.hpp

[andrewcraik]

I'm not a fan of code annotations - they become stale over time, you have to update the Java source when the optimizer changes, and the fact they will be limited to a very small subset of the class library makes me even more dubious. The list of recognized methods is a fairly good starting point for the things which have special handling. What specific problems are you hoping this annotation would address?
Edit: Note - I am open to an argument for them, I'm just not up front a fan of the idea.

[fjeremic]

What specific problems are you hoping this annotation would address?

As a concrete example, looking at #1681 a less informed reader reading the implementation of java.lang.String.indexOf may not realize the JIT will do acceleration underneath the hood. Similarly for hashCode, toLower, toUpper, etc. etc. the list is huge.

I've also encountered several cases personally where we do acceleration on x86 for example but we don't do it on Z so we're missing a performance opportunity which is hard to identify just reading the Java code. e.x. some code which is critical for performance of some workload was accelerated on x86 in the JIT but not on Z. I'm reading the Java code in the area and I would have never known a particular method may have special handling.

[andrewcraik]

I wonder though if your specific problem - ie a method not being accelerated or not having a specific place to look for a method would be better handled by some codegen refactoring to make the handling of these methods more common and centralized and easier to audit rather than imposing a double maintenance problem? This is just me thinking out loud - an annotation that isn't updated is probably more misleading than the current state of affairs in my opinion.

[fjeremic]

This is just me thinking out loud - an annotation that isn't updated is probably more misleading than the current state of affairs in my opinion.

We should add tests for this. Changing signatures or moving/removing accelerated methods should be a performance bug that we should take special consideration of before merging. There should not be stale recognized method signatures in j9method.cpp. I'm sure there is today even.

[andrewcraik]

Right, but some platforms can't do some accelerations and we may not do them under certain circumstances. Because special handling can be case specific I don't know that such a black and white vision of acceleration is really achievable.

[fjeremic]

Right, but some platforms can't do some accelerations and we may not do them under certain circumstances. Because special handling can be case specific I don't know that such a black and white vision of acceleration is really achievable.

Whether or not we do acceleration is not a precursor to whether we recognize the method in the JIT. The annotation is "IntrinsicCandidate" so it signifies it is possible for the JIT to do something special with the given method, not guarantee it.

We recognize methods regardless of whether the platform supports intrinsics for the particular method.

[DanHeidinga]

@fjeremic Are these meant to be runtime visible annotations or runtime invisible (compile time) annotations? The root of my question is whether these are meant to be documentation and a warning when modifying the code or will the JIT be processing this annotation?

[fjeremic]

The root of my question is whether these are meant to be documentation and a warning when modifying the code or will the JIT be processing this annotation?

My initial proposal would be documentation and warning only.

[DanHeidinga]

Sounds reasonable to me. This would help everyone modifying the code and reviewing it to know when there's a related intrinsic that may also need updating.

[gita-omr]

FYI: Vector API library uses @IntrinsicCandidate