Add MemberName finalizer to mark clazz for MemberName list pruning

[jdmpapin]

This prevents a memory leak in the maintenance of per-class lists used to find affected MemberName objects in case of class redefinition.

This depends on eclipse-openj9/openj9#19187

[jdmpapin]

22, 21, and 17 to follow

[keithc-ca]

finalize() methods are deprecated; this should use a "cleaner" - see java.lang.ref.Cleaner.

[keithc-ca]

See also https://openjdk.org/jeps/421.

[jdmpapin]

I'll create a Cleaner and register with it on paths that call native methods of MethodHandleNatives that could add the MemberName to the per-class list. That's init, resolve, and (in Java 17) getMembers

[jdmpapin]

I found that Cleaner was noticeably harder to use than a finalizer for this use case:

• There's no easy way to make it apply to every instance. MemberName has many constructors, one of which is empty and commented to say that the VM will initialize the object, so it's not clear whether in that case that constructor even runs.
• The cleaning action can't even be created until the class is known anyway (without moving the clazz field into the cleaning action entirely). This will make getMembers in Java 17 especially annoying: the cleaning actions can't be created before the call because clazz hasn't been set yet, but it looks like it's possible for the native code to initialize one or more MemberName instances and then throw while trying to initialize another.
• It was easy to accidentally prevent some MemberName instances from being reclaimed and thereby also prevent anonymous classes from being unloaded.

On top of that, it takes more code to accomplish the same thing.

For these reasons, if possible I'd like to stick with using a finalizer at least in all of the JDK versions where we can be sure that finalization is not being removed.

That said, I worked through those problems in an attempt to future-proof this code for jdknext (at least), and I came up with this commit.

The reason I haven't updated this PR is that the mere presence of the following line is causing some test failures for me:

private static final Cleaner CLEANER = Cleaner.create();

In particular, thrstatetest_0 and thrstatetest_1 say "ALL TESTS COMPLETED AND PASSED" but then fail with "Failed to destroy JVM". And VmArgumentTests_0 fails testMemoryLeaks.

Please note that the logic in this PR is an important component of eclipse-openj9/openj9#19187, which I'm trying to get into 0.44.

[jdmpapin]

I should mention that I doubt that I would be able to debug these test failures in time, and I also doubt that I'm the right one to debug them anyway

[jdmpapin]

If it did and the class got unloaded before the cleaning action ran, then we'd have a dangling pointer

[keithc-ca]

There are only three places that write to the clazz field and only via init() or MethodHandleNatives.init() might it be non-null. Those would seem to be the only places where registering a cleaner is necessary.

It feels risky to embark on the path to a solution involving a cleaner at this late stage approaching the 0.44.0 release. I'm ready to accept the "finalizer" approach if we can agree that it should be a priority to switch to using a cleaner as soon as possible.

[jdmpapin]

There are only three places that write to the clazz field and only via init() or MethodHandleNatives.init() might it be non-null. Those would seem to be the only places where registering a cleaner is necessary.

Yes, I believe I did work out the places where it's necessary to create the cleaning action. Rather than look for everywhere clazz is set, I looked for everywhere the MemberName might get added to the per-class list (from eclipse-openj9/openj9#19187) that would need to be pruned. So that's just MethodHandleNatives.init() and MethodHandleNatives.resolve() (in Java 21+).

My main points here are:

1. The code using a Cleaner was harder to write and is harder to understand than the finalizer.
2. Merely creating a Cleaner causes some test failures that I'm ill-equipped to deal with myself.
3. Yes, finalization is deprecated for removal as of Java 18, but that means that it won't be removed in Java 17, and it's quite possible that we could be confident that it won't be removed in 21 (or maybe even 22) either.

None of that is to say that I love the finalizer though 😅

It feels risky to embark on the path to a solution involving a cleaner at this late stage approaching the 0.44.0 release. I'm ready to accept the "finalizer" approach if we can agree that it should be a priority to switch to using a cleaner as soon as possible.

I agree that it should be a priority to get rid of the finalizer soon. From discussion with @tajila, @babsingh, @amicic, and @dmitripivkine, there should be a better fix that modifies the GC to eliminate this finalization step and the separate pruning pass over the list. There are different potential design points for how that should work, but none of them include finalization or a cleaner. Instead, the GC will do the necessary cleanup directly. I'll leave it up to the GC and VM teams to decide whether it's worth switching to a cleaner before that point, only for the cleaner to itself be replaced. (However, I wouldn't personally recommend switching to a cleaner in versions where finalization will remain enabled, since IMO for this particular code it's not an improvement.)

[keithc-ca]

A solution that needs neither finalization nor a cleaner sounds good (as long as it doesn't suffer from excessive delay recovering resources). If we think it will arrive relatively soon (say before the next release) then I would agree we don't need to pursue the cleaner approach.

Cleaners can yield more timely recovery of resources than an approach that depends on running finalization code.

[tajila]

We are in the process of designing a solution that introduces a new mechanism to enable the creation of a weak object list managed by the GC. This will allow us to create a per class member name list that is automatically pruned by the GC. The complexity of this solution means we have no chance of getting it into 0.44 (which is important as stakeholders are waiting for known issues to be addressed). So we have gone with the solution in this PR for the time being.

As Devin stated, the long term solution will not rely on finalization or cleaners, both of which have issues.

[tajila]

@keithc-ca Do you have further comments?

[keithc-ca]

Do you have further comments?

As I reviewed this again today (I had been waiting for eclipse-openj9/openj9#19187 to be merged), I am a little puzzled that the finalizer provides the Class object (instead of the MemberName object) in the native call: why shouldn't more precise information (the MemberName that has become unreachable) be preferred?

[keithc-ca]

Jenkins test sanity alinux jdknext

[jdmpapin]

I am a little puzzled that the finalizer provides the Class object (instead of the MemberName object) in the native call: why shouldn't more precise information (the MemberName that has become unreachable) be preferred?

I'm passing only the class because that's all that's needed. I would tend (or at least try) not to pass more information than necessary unless there's some particular benefit to doing so

That decision has already been helpful in this PR: it helped me to try Cleaner when requested. Even though we're not proceeding with Cleaner at the moment, it saved me time. If I had passed the MemberName instead, then I'd have had to change the native method at that point

[keithc-ca]

My point is that by passing the class, the native code has to consider all MemberName objects that relate to the specified class, when each call relates to just one MemberName object (it's more than is needed). Additional calls for other MemberName objects will be a waste of time if an earlier call has already dealt with the current MemberName object.

The native method is new so I don't understand what "change" your referring to.

[jdmpapin]

The native code just flags the class so that we know (later) to look through its list for defunct entries. As it stands, it wouldn't be much help to know the specific MemberName instance, since that would still require us to search through the list to find the corresponding entry. It's true that with some other changes, we could make use of the knowledge of the particular MemberName instance. For example, if the per-class list were doubly linked, and if we added a pointer from the MemberName to its list node, then we could just go straight to it and cut it out of the list in constant time. But eclipse-openj9/openj9#19187 was the design we landed on to get a fix into the release quickly based on what I already had working. I was focusing not on how to improve the temporary fix, but rather on eliminating the memory leak in the agreed-upon way and on testing (that fixup still works properly, that redefinition sees a large speedup, that the lists really do get pruned, and that I'm not preventing class unloading, all together with broader routine pre-PR testing)

The native method is new so I don't understand what "change" your referring to.

I went through the exercise to change what I had in this PR to use Cleaner. Ultimately I didn't update the PR itself to make use of that, but I did write the code to get it to a certain level of functionality. Even though neither this PR nor eclipse-openj9/openj9#19187 was merged at the time, that was a change compared to what I had written and tested at that point. If the new native method had originally been written to take MemberName rather than Class, then that exercise would have been more work because I'd have also had to modify the (not yet merged) native method to stop taking MemberName. Since it takes Class, I was able to use it as-is

[jdmpapin]

that would still require us to search through the list to find the corresponding entry

In fact, without additional state to correlate them, we wouldn't even be able to recognize the corresponding list node, since by the time the finalizer is running, the weak reference will have already been cleared

[keithc-ca]

In fact, without additional state to correlate them, we wouldn't even be able to recognize the corresponding list node, since by the time the finalizer is running, the weak reference will have already been cleared

I don't think use of a weak reference in the cleaner approach was appropriate, but as all this is going away (soon!), I wasn't going to worry about that.