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

Markus Grönlund mgronlun at openjdk.java.net
Mon Sep 13 11:04:58 UTC 2021


On Wed, 8 Sep 2021 17:37:20 GMT, Coleen Phillimore <coleenp at openjdk.org> wrote:

>> Markus Grönlund has updated the pull request incrementally with three additional commits since the last revision:
>> 
>>  - localize
>>  - cleanup
>>  - FinalizerStatistics
>
> 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.

Thanks Coleen for taking a look at this.

It is used internally by notify_class_loaded() and notify_class_unloaded(); therefore I will change it to have internal linkage.

> 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?

This hashtable holds a FinalizerEntry for every InstanceKlass that provides a non-empty finalizer method and have allocated at least one object. How many there are in general depends on the application. A ResourceHashtable does not have the concurrency property required, as lookups and inserts will happen as part of object allocation.

> 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.

Our Atomic implementation does not support CAS of a 64-bit value on 32-bit platforms (compiles but does not link).

> 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.

Thank you for pointing that out. I had assumed this to be a part of the syntactic contract for using the ConcurrentHashTable. My wrong assumption was because the SymbolTable (which I used as a model) seemed to pass a "rehash_warning" bool to the accessor APIs (get, insert). However, looking more closely at the signatures in ConcurrentHashTable, that bool is a "grow_hint", not "rehash" specifically. Thanks again; I will remove the rehash support code.

> 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()

"Since you remove entries on unloading, I don't see any reason to have any concurrent cleanup."
Thank you, that is true. The only concurrent work required will be to grow the table.

"You do however need in the lookup code, some code that doesn't find the InstanceKlass if !ik->is_loader_alive()"

A precondition is that the code doing the lookup hold the ClassLoaderDataGraph_lock or is at a safepoint. Is that still the case? Would not purge_unloaded() take out the InstanceKlass from the table, as part of unloading, before !ik->is_loader_alive() is published to the outside world?

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

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


More information about the core-libs-dev mailing list