RFR: 8266936: Add a finalization JFR event [v10]

[Coleen Phillimore]

On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund <mgronlun at openjdk.org> wrote:

>> Greetings,
>> 
>> Object.finalize() was deprecated in JDK9. There is an ongoing effort to replace and mitigate Object.finalize() uses in the JDK libraries; please see https://bugs.openjdk.java.net/browse/JDK-8253568 for more information. 
>> 
>> We also like to assist users in replacing and mitigating uses in non-JDK code.
>> 
>> Hence, this changeset adds a periodic JFR event to help identify which classes are overriding Object.finalize().
>> 
>> Thanks
>> Markus
>
> Markus Grönlund has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - localize
>  - cleanup
>  - FinalizerStatistics

I have some general comments and questions about this code.

src/hotspot/share/services/classLoadingService.cpp line 80:

> 78: 
> 79: size_t ClassLoadingService::compute_class_size(InstanceKlass* k) {
> 80:   // lifted from ClassStatistics.do_class(Klass* k)

Can you remove this line? I think that function is gone now.

src/hotspot/share/services/finalizerService.cpp line 44:

> 42:     _ik(ik),
> 43:     _objects_on_heap(0),
> 44:     _total_finalizers_run(0) {}

Is this hashtable for every InstanceKlass that defines a finalizer?  How many are there generally?  Why couldn't this be a simple hashtable like ResourceHashtable (soon to be renamed) which you can write in only a few lines of code?

src/hotspot/share/services/finalizerService.cpp line 114:

> 112: 
> 113: static inline void added() {
> 114:   set_atomic<inc>(&_count);

Why isn't Atomic::inc() good enough here? It's used everywhere else.

src/hotspot/share/services/finalizerService.cpp line 123:

> 121: static inline uintx hash_function(const InstanceKlass* ik) {
> 122:   assert(ik != nullptr, "invariant");
> 123:   return primitive_hash(ik);

If the hash function is primitive_hash, this hash is good enough to not need rehashing.  The only reason for the rehashing for symbol and string table was that the char* had a dumb hash that was faster but could be hacked, so it needed to become a different hashcode.  This doesn't need rehashing.

src/hotspot/share/services/finalizerService.cpp line 485:

> 483: void FinalizerService::purge_unloaded() {
> 484:   assert_locked_or_safepoint(ClassLoaderDataGraph_lock);
> 485:   ClassLoaderDataGraph::classes_unloading_do(&on_unloading);

Since you remove entries on unloading, I don't see any reason to have any concurrent cleanup.
You do however need in the lookup code, some code that doesn't find the InstanceKlass if !ik->is_loader_alive()

-------------

PR: https://git.openjdk.java.net/jdk/pull/4731