Re: RFR: 8266936: Add a finalization JFR event [v10]
On Tue, 31 Aug 2021 08:45:54 GMT, Erik Gahlin wrote: >> Markus Grönlund has updated the pull request incrementally with three >> additional commits since the last revision: >> >> - localize >> - cleanup >> - FinalizerStatistics > > src/java.base/share/classes/java/lang/ref/Finalizer.java line 71: > >> 69: } >> 70: >> 71: > > extraneous whitespace? I think this version is outdated, and the extra whitespace was removed in later versions. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Tue, 31 Aug 2021 08:42:58 GMT, Erik Gahlin wrote:
>> Markus Grönlund has updated the pull request incrementally with three
>> additional commits since the last revision:
>>
>> - localize
>> - cleanup
>> - FinalizerStatistics
>
> test/jdk/jdk/jfr/event/runtime/TestFinalizerStatisticsEvent.java line 98:
>
>> 96: case TEST_CLASS_NAME: {
>> 97:
>> Asserts.assertTrue(event.getString("codeSource").startsWith("file://"));
>> 98: foundTestClassName = true;
>
> Could we (sanity) check "objects" and "totalFinalzersRun" fields as well?
It's risky to do because of the non-deterministic nature of when the Finalizer
thread runs (if at all). The best I could think of is to check if either field
is 0 or more, but that becomes so weak it's not much of a sanity check.
-
PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund 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
Marked as reviewed by egahlin (Reviewer).
src/java.base/share/classes/java/lang/ref/Finalizer.java line 71:
> 69: }
> 70:
> 71:
extraneous whitespace?
test/jdk/jdk/jfr/event/runtime/TestFinalizerStatisticsEvent.java line 98:
> 96: case TEST_CLASS_NAME: {
> 97:
> Asserts.assertTrue(event.getString("codeSource").startsWith("file://"));
> 98: foundTestClassName = true;
Could we (sanity) check "objects" and "totalFinalzersRun" fields as well?
-
PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Thu, 14 Oct 2021 22:02:43 GMT, Coleen Phillimore wrote: >> Ok, I see - grow the table. >> >> I'm not sure if you need to ask ik->is_loader_alive() or not, but I think >> you do. The InstanceKlass is removed from your table during class unloading >> but before that during concurrent class unloading, the class might not be >> alive while you look at it and concurrent class unloading hasn't gotten >> around to removing it yet. Since you save classes regardless of CLD, you >> have to check if it's alive. See classLoaderDataGraph.cpp >> ClassLoaderDataGraphIterator for example. The CLDG_lock only keeps the >> graph from not getting modified, but the classes in it might be dead. > > That said, I don't see where you return an InstanceKlass in the table, which > would need this check. Not in class_unloading_do because this follows the > _unloading list. So there is already support for concurrent class unloading today in JFR, and the protocol is built around the CLDG_lock. If JFR holds it, concurrent class unloading cannot run. If GC holds it, JFR cannot inspect classes. That's why the table inspection is guarded via the CLDG_lock, for mutual exclusion to avoid this problem. I.e. if concurrent class unloading is in progress, JFR will not inspect the table. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Thu, 14 Oct 2021 21:58:42 GMT, Coleen Phillimore wrote: >> "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? > > Ok, I see - grow the table. > > I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you > do. The InstanceKlass is removed from your table during class unloading but > before that during concurrent class unloading, the class might not be alive > while you look at it and concurrent class unloading hasn't gotten around to > removing it yet. Since you save classes regardless of CLD, you have to check > if it's alive. See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for > example. The CLDG_lock only keeps the graph from not getting modified, but > the classes in it might be dead. That said, I don't see where you return an InstanceKlass in the table, which would need this check. Not in class_unloading_do because this follows the _unloading list. - PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Mon, 13 Sep 2021 10:54:18 GMT, Markus Grönlund wrote:
>> 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.
ok.
>> src/hotspot/share/services/finalizerService.cpp line 331:
>>
>>> 329: }
>>> 330: Thread* const thread = Thread::current();
>>> 331: // We use current size
>>
>> 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?
Ok, I see - grow the table.
I'm not sure if you need to ask ik->is_loader_alive() or not, but I think you
do. The InstanceKlass is removed from your table during class unloading but
before that during concurrent class unloading, the class might not be alive
while you look at it and concurrent class unloading hasn't gotten around to
removing it yet. Since you save classes regardless of CLD, you have to check
if it's alive. See classLoaderDataGraph.cpp ClassLoaderDataGraphIterator for
example. The CLDG_lock only keeps the graph from not getting modified, but the
classes in it might be dead.
-
PR: https://git.openjdk.java.net/jdk/pull/4731
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Wed, 8 Sep 2021 17:37:20 GMT, Coleen Phillimore 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(&_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
Re: RFR: 8266936: Add a finalization JFR event [v10]
On Fri, 27 Aug 2021 15:23:35 GMT, Markus Grönlund 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(&_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
Re: RFR: 8266936: Add a finalization JFR event [v10]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/4731/files - new: https://git.openjdk.java.net/jdk/pull/4731/files/e6b786f1..fd86899f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=4731&range=08-09 Stats: 457 lines in 17 files changed: 190 ins; 222 del; 45 mod Patch: https://git.openjdk.java.net/jdk/pull/4731.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4731/head:pull/4731 PR: https://git.openjdk.java.net/jdk/pull/4731
