RFR(S) 8249276 CDS archived objects must have "neutral" markwords

[Coleen Phillimore]

This looks good.
Coleen

On 8/5/20 6:26 PM, Ioi Lam wrote:
> Hi David & Coleen,
>
> Thanks for the review. I have created a new version:
>
> http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v02/ 
>
>
>
> I created a new test case (LockDuringDump), and incorporated David's 
> fix for JDK-8250606 [1] during my testing:
>
> + Without my patch, LockDuringDumpApp would crash at runtime, because
>   we have an invalid markword for the Helper.class object.
>
> + Without the JDK-8250606 patch, we would assert during CDS dump time.
>
> I also removed the assert in HeapShared::oop_hash() since it's 
> irrelevant after JDK-8250606.
>
> I intend to push my patch after JDK-8250606 is integrated.
>
>
> -------
>
> More comments below
>
> On 8/3/20 6:13 PM, David Holmes wrote:
>> Hi Ioi,
>>
>> On 27/07/2020 3:20 pm, Ioi Lam wrote:
>>> https://bugs.openjdk.java.net/browse/JDK-8249276
>>> http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v01/ 
>>>
>>>
>>> Please review this change (initial patch provided by David Holmes; I 
>>> rearranged it a little and added more comments).
>>>
>>> We reinitialize the markWord of all archived objects to remove any 
>>> side effect of GC/locking/etc during "java -Xshare:dump".
>>
>> Changes look good to me (naturally :) ). A couple of nits:
>>
>>    int hash_archived = archived_oop->identity_hash();
>>    assert(hash_original == hash_archived, "Different hash codes: 
>> original %x, archived %x", hash_original, hash_archived);
>>
>> the has_archived setting should be in ifdef DEBUG.
>>
>
> Fixed
>
>
>> 42 Object.class.wait();
>>   43                 }
>>   44                 System.out.println("Huh?? notified??");
>>
>> That could be a spurious wakeup. You could just use Thread.sleep with 
>> a suitably long sleep time. that said I'm not sure how the test is 
>> intended to operate. If premain returns after starting the thread 
>> there is no guarantee the thread will start executing and lock the 
>> object before the dump actually commences. Even the sighting of 
>> "Let's wait ....." is not sufficient to guarantee that unless you 
>> move it inside the sync block.
>>
>
> I have fixed the test case so that the "Let's hold the lock" message 
> is printed after the lock is held, and the lock is never released 
> afterwards.
>
> I also added synchronization to make sure the premain() method doesn't 
> return until the child thread has held the lock. The log file shows 
> that theses messages are printed before CDS tries to load classes from 
> the classlist:
>
> [0.005s][info][cds] Allocated shared space: 3221225472 bytes at 
> 0x0000000800000000
> inside LockDuringDumpAgent: 
> jdk.internal.loader.ClassLoaders$AppClassLoader at 707f7052
> Helper class is initialized
> Class = class LockDuringDumpApp$Helper
> Let's hold the lock on Helper.class forever .....
> Thread has started
> [0.145s][info][cds] Loading classes to share ...
>
>>> (David mentioned in the bug comments about assertions inside the 
>>> identity_hash() call, but maybe this should be fixed in a different 
>>> bug?)
>>
>> Yes those assertions are being removed under:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8250606
>>
>
> Thanks
> - Ioi
>
> -------
>
> [1] 
> https://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2020-August/040985.html
>
>