Consider dropping HCR extensions?

[jdmpapin]

That is, allowing the addition and removal of methods and static fields.

I'm curious what the motivation/use case is for these extensions, and whether it might be possible to drop them.

---

For context, I've been working on a change (#17850) that interacts with class redefinition via MemberName, and the HCR extensions really make matters a lot more complicated when it comes to the invoke and reflect packages. It's not clear what the behaviour is supposed to be, especially when methods are removed.

In trying to clarify the expected behaviour for myself I found that certain interactions are simply broken:

1. Method instances are not fixed up. They can then call the wrong method or even just segfault.
2. VirtualHandle (J9 invoke) is not fixed up either, with similar consequences.
3. DirectHandle (J9 invoke) has its J9Method substituted according to methodPairs, which is only populated for fast HCR. Leaving DirectHandle instances to call the old method bodies is a conceivable choice, but it doesn't look intentional (IMO).
4. The existing code that tries to fix up MemberName.vmtarget fails to kick in because when it does a lookup into classHashTable the key (originalRAMClass) is uninitialized, so at the moment direct dispatches will unintentionally call the old implementation even with fast HCR.

In my PR, I've fixed (4) - which has to change because I'm changing the meaning of the vmindex field - but only to an extent. The MemberName is fixed up properly whenever the J9JNIMethodID is, which I believe is any time the VM finds a corresponding method (with the same name and signature) in the redefined class. But if the method is removed, then we're out of luck.

---

Looking for more information online I found https://github.com/eclipse-openj9/openj9/pull/13566#issuecomment-926840082:

The HCR extensions are not well supported and are rarely used, even by debuggers.

And in #771 OpenJDK was found to allow addition/removal of private methods. It was consequently modified to stop doing so.

[DanHeidinga]

I believe we've been hesitant to remove the extensions in the past due to being unable to confirm whether they were stilled used or not. There was at least one(?) product that was supposedly enabling the extensions to support enhanced debugging. I'm not sure if it still exists....

@gacholio has always advocated for removing the extensions. He or @pshipton may have a better sense of how or even if they are still used.

[gacholio]

AFAIK, RAD (Rational Application Developer?) had automated testing of the extensions (with no evidence any customer cared) and this was the sole business reason to maintain this code path. I suspect that product is 10 years dead, so I'm in favour of removing the extensions. The supporting code will still exist in previous revisions if we ever need to reinstate it.

We have had years of confusion about the various HCR modes and disparate terminology associated with them. A reset would be appropriate.

[gacholio]

If extensions are enabled, that implies FSD (when the JIT is enabled). Any HCR in FSD mode discards the entire code cache, so there's no possibility of error - after HCR no existing compiled code will be executed.

[gacholio]

Also note that in non-fast (extended) HCR mode, all of the constant pools are reset to their unresolved state (or possibly re-resolved in place) so that should take care of the invokes after HCR.

[jdmpapin]

in non-fast (extended) HCR mode, all of the constant pools are reset

Right, I saw that, so when a method is removed we'll just get NoSuchMethodError the next time we try to invoke. The real trouble is with invocations that go through some kind of reflective object (e.g. Method, MethodHandle) and therefore don't use the constant pool. I guess the most analogous behaviour would be for those to throw NoSuchMethodError on the next invocation as well? It would also still be weird though because for reflective objects like that, the step that corresponds to resolution is the creation of the object, which is explicitly separate from the invocation. If the method disappears, now we have an object that in a sense never should have been created

[jdmpapin]

I didn't look much into the interaction between adding/removing static fields (as opposed to methods) and reflection, but that has the same fundamental complications, and I expect it's probably also broken in some way

Also, it occurs to me now that in addition to java.lang.reflect and java.lang.invoke, native code could run into a problem directly with J9JNIMethodID or J9JNIFieldID if it holds onto the jmethodID or jfieldID of a member that's been removed

[gacholio]

@tajila What do we need to do to move this forward? Ideally, the extensions would be removed for all java levels, or else we will need to continue maintaining the code (though without adding support for new features if we support extensions only up to a particular level).

[pshipton]

Normally we announce deprecation and remove the feature at a later time. To remove for all versions, I suggest we update the doc to announce the deprecation, wait a couple of release cycles for any feedback, if none then disable the feature by default but provide an option to re-enable it, wait at least a year, then remove it.

[jdmpapin]

Has this deprecation been announced?

[tajila]

AFAIK nothing has been anounced, but with JDK22 coming this would be a good time to start.

@gacholio Looking at the code it seems like extensions aren't explicitly enabled? If so, we could introduce a cmdline option to explcitily turn it on in JDK22 (with warning message saying its going away).

[tajila]

Ideally, the extensions would be removed for all java levels,

@pshipton have we done something like this before? Im personally not against it, we could use the aproach above. But I wonder if this breaks a precedent.

[pshipton]

It's not something we usually do, but it may have happened in the past for something minor. If we are cautious about removing it, prepared that it may not be able to be removed from older versions, perhaps it can be done. The first thing to do is open a docs issue and announce deprecation, ideally in time for jdk22, then it can at least be disabled in jdk23 and forward. We can state the intent is to remove it from jdk8+ in the future and ask anyone who is concerned to provide feedback.

[gacholio]

There's almost no value to us in removing a feature only in certain JDK levels. We can avoid adding new support for special cases, but unless it's removed entirely, we continue to have to maintain it.

[pshipton]

We continue to have to maintain it only until the versions that support it go out of service, which will happen eventually.

[jdmpapin]

Related: #19554, eclipse-openj9/openj9-docs#1352