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 #13566 (comment):

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

[c-koell]

I want to bring some feedback into this discussion. As i can see it's maybe a little bit late but we (DevTeam with about 50 People) have recently realized what it means to no longer have Extended HCR.

I have posted a question #21040 and reallized that this feature have been deactivated.
Our team became increasingly dissatisfied with the development until we realized why the usual HCR feature was not working.
We have now activated the Extended HCR and the mood rose again when developing.

If we would only have a basic HCR (changes in method body), our productivity during development decreases enormously. We have a lot of different applications running on openliberty. Dependening on the size of the application a (re)start will take some time. It's a key feature for us to have the ability to change the code and test it without to restart every time the application server.

We have discussed the planned remove from the extended hcr feature in our team and we all agree that we must then look for an alternative. There are of course some tools out there (Jrebel..) to adress it, but apart from cost we would like to say that HCR from OpenJ9 is exactly what we actually need.

There will certainly be technical reasons for the removal of this feature, but we won't be the only ones who will miss this feature.

FYI @babsingh, @pshipton @jdmpapin @gacholio @tajila

[tajila]

I want to bring some feedback into this discussion. As i can see it's maybe a little bit late but we (DevTeam with about 50 People) have recently realized what it means to no longer have Extended HCR.

This feedback is certainly helpful. We have been unsure whether anyone was using the feature which is why we added the cmdline option. We will consider this feedback as we plan for future releases.

[jdmpapin]

The following is just my individual two cents, relatively off the cuff, so not any kind of concrete plan...

I see that the extended redefinition capability is useful, and it would be helpful to have it. If I had to guess at this point, I'd imagine that most people using the feature don't know that they're doing so, like in this instance. But IMO the current implementation is untenable, and we should still get rid of it.

The best way that I can think of to do this in the future would involve preprocessing the new bytecode to make it better match the existing bytecode. For example, if methods are reordered, put them back in the original order. If a method is removed, copy the original method into the new bytecode, or maybe add a definition that throws an exception instead. Similarly for static fields. Any newly added private or static methods could be moved to be last, though allowing that would be more involved because it would require us to make ramMethods expandable, e.g. by changing it from an array into a linked list of arrays. Similarly for newly added static fields and ramStatics (at least if we want to insist for future Unsafe purposes that the existing statics shouldn't move, and I think we do). Adding instance fields or virtual methods would still be a no-go because it would change the instance or vtable layout and size, except maybe in the case of adding an override of a method that already exists in a base class.

Side note: What is going on with UIs in this area? It seems that (for at least some of them) they just allow redefinition to fail silently and leave the user guessing at what happened. Why is our extended HCR warning the only explicit indication that the user gets of a redefinition failure?

[jdmpapin]

An alternative way to make things (e.g. ramMethods, ramStatics, even vtables) expandable without moving the existing elements, if we'd be OK imposing a limit on how many new elements could be added, might be to deliberately over-allocate them by some amount.

Though even if we did this for vtables, there would still be potential issues with adding new virtual methods, since a subclass might already have a method that should now override the new method, but it would have already been assigned a different vtable index. Maybe it would be OK if we were to check that that specific situation doesn't occur? (and fail if it does)