On Wed, 8 Sep 2021 17:37:20 GMT, Coleen Phillimore <cole...@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
