[14] RFR(S): 8225670: compiler/types/correctness/* tests fail with "assert(recv == __null || recv->is_klass()) failed: wrong type"

[Tobias Hartmann]

Hi Patrick,

thanks, we've already noticed that regression and backed the fix out:
https://bugs.openjdk.java.net/browse/JDK-8230020

Best regards,
Tobias

On 22.08.19 13:20, Patrick Zhang OS wrote:
> Hi Christian,
> 
> My test system shows this fix probably introduced a significant performance loss with a couple of apps/benchmarks on both aarch64 (Ampere eMAG) and x86 systems, I filed this ticket to track it. https://bugs.openjdk.java.net/browse/JDK-8230036
> 
> Regards
> Patrick
> 
> -----Original Message-----
> From: hotspot-compiler-dev <hotspot-compiler-dev-bounces at openjdk.java.net> On Behalf Of Christian Hagedorn
> Sent: Friday, August 9, 2019 7:56 PM
> To: Erik Österlund <erik.osterlund at oracle.com>; hotspot-compiler-dev at openjdk.java.net; Tobias Hartmann <tobias.hartmann at oracle.com>
> Subject: Re: [14] RFR(S): 8225670: compiler/types/correctness/* tests fail with "assert(recv == __null || recv->is_klass()) failed: wrong type"
> 
> Hi Erik, hi Tobias
> 
> Thank you for your reviews!
> 
> On 09.08.19 11:21, Erik Österlund wrote:
>> Hi Christian,
>>
>> Looks good - well spotted.
>>
>> To answer your question - yes the GC (ZGC in particular, and probably 
>> soon Shenandoah when they hook in to the concurrent class unloading
>> framework) cleans the extra data section of MDOs concurrently, under 
>> the extra data lock.
>>
>> However, they clear whole rows from the extra data section, under the 
>> extra data lock of the MDOs; they never write that the Klass is NULL. 
>> So I believe this bug only relates to the use of the WhiteBox API.
> 
> That was my guess, too. Thanks for clearing that up and answering the question.
> 
>> The row clearing of concurrent GCs synchronizes with a metadata 
>> preparation phase for unpacking MDOs to ciMDOs. The preparation phase 
>> will in a fixed-point iteration try to create ci handles for all 
>> encountered metadata under the extradata lock. Every time it 
>> encounters an uncached metadata instance, it has to release the lock 
>> due to ranking issues, and may also run into safepoints then. Such 
>> situations are detected, triggering a restart of the fixed-pont iteration.
>>
>> Once the fixed-point iteration has finished, we know that we under the 
>> lock walked all metadata in the extra data section without ever 
>> releasing the lock, have ci handles keeping all metadata alive, and 
>> can't have gotten any safepoints due to being in VM state. After that, 
>> the rows are copied and translated, and now we are guaranteed that the 
>> translation will always already have the ci handles cached.
> 
> Thanks for the detailed explanation!
> 
>> There is some random original copy of the raw MDO extra data that is 
>> performed before preparing the metadata. I don't think it is really 
>> used or needed. Might be interesting to remove in a future RFE. It 
>> gets overwritten by the subsequent row-by-row processing after 
>> metadata preparation.
> 
> I created a new RFE [1] and referenced this conversation.
> 
> Best regards,
> Christian
> 
> 
> [1] https://bugs.openjdk.java.net/browse/JDK-8229353
>