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

[Bertrand Delsart]

On 03/10/14 14:42, Bertrand Delsart wrote:
> 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.

Or, if you really do not want to loop, to add an assert verifying that 
your concurrency hypothesis is still correct by checking that the flag 
is indeed always set when exiting from the atomic_try_set_bits.

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