On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund <mgron...@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