RFR: 8056240: Investigate increased GC remark time after class unloading changes in CRM Fuse

[Bertrand Delsart]

Hi Stefan,

I'll do a more in depth review next week but here are a few quick 
comments to keep us busy in between :-)

-1- The fact that set_on_stack() is now implemented by just a 
atomic_try_set_bits seems dangerous.

We could end up not setting the JVM_ACC_ON_STACK flag.

In your case, you may be sure that the only concurrent operation that 
might happen is a setting of the same flag but this looks error prone 
with respect to future possible usages of that method (or new concurrent 
operations).

I would prefer to be sure the flag has really been set when exiting from 
set_on_stack(), as was the case before.

More generally, the semantic of the new atomic_try_set_bits is IMHO 
dangerous. It returns false both if the bits were already set or if 
another flag was changed concurrently (in which case the bits may not be 
set). This should at least be clearly documented in the .hpp file.

An alternative would be for atomic_try_set_bits to return the current 
value when the cas failed or was not performed. It would be up to the 
caller to decide whether to loop or not (by checking whether the bits 
are set in the returned value).

-2- There is one part of the change that does not seem to be described 
in the RFR or the CR. You added something to keep alive some metadata in 
case of klass redefinition (the mark_on_stack stuff in nmethod.cpp, 
which for instance now marks the content of the CompiledICHolder).

Did you just change when that marking was performed or is that a marking 
that we were not doing previously ? (sorry, cannot review it all 
currently and see whether that marking was indeed removed elsewhere)

Regards,

Bertrand

On 02/10/14 12:53, Stefan Karlsson wrote:
> Hi all,
>
> (The following patch changes HotSpot code in areas concerning GC, RT,
> and Compiler. So, it would be good to get reviews from all three teams.)
>
> Please, review this patch to optimize and parallelize the CodeCache part
> of MetadaOnStackMark.
>
> G1 performance measurements showed longer than expected remark times on
> an application using a lot of nmethods and Metadata. The cause for this
> performance regression was the call to
> CodeCache::alive_nmethods_do(nmethod::mark_on_stack); in
> MetadataOnStackMark. This code path is only taken when class
> redefinition is used. Class redefinition is typically used in monitoring
> and diagnostic frameworks.
>
> With this patch we now:
> 1) Filter out duplicate Metadata* entries instead of storing a Metadata*
> per visited metadata reference.
> 2) Collect the set of Metadata* entries in parallel. The code
> piggy-backs on the parallel CodeCache cleaning task.
>
> http://cr.openjdk.java.net/~stefank/8056240/webrev.00/
> https://bugs.openjdk.java.net/browse/JDK-8056240
>
> Functional testing:
> JPRT, Kitchensink, parallel_class_unloading, unit tests
>
> Performance testing:
> CRM Fuse - where the regression was found
>
> The patch changes HotSpot code in areas concerning GC, RT, Compiler, and
> Serviceability. It would be good to get some reviews from the other
> teams, and not only from the GC team.
>
> thanks,
> StefanK


-- 
Bertrand Delsart,                     Grenoble Engineering Center
Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
38334 Saint Ismier,                                        FRANCE
bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
NOTICE: This email message is for the sole use of the intended
recipient(s) and may contain confidential and privileged
information. Any unauthorized review, use, disclosure or
distribution is prohibited. If you are not the intended recipient,
please contact the sender by reply email and destroy all copies of
the original message.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~