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

[David Holmes]

Hi Ioi,

On 6/08/2020 8:26 am, 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/ 
> 

Functional changes are good.

For the test, as per our offline chat, the Helper class is not being 
archived. This line:

  66                 TestCommon.testDump(appJar, 
TestCommon.list(LockDuringDumpApp.class.getName()),

should be

  66                 TestCommon.testDump(appJar, 
TestCommon.list(appClasses),

Thanks,
David

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