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

[Ioi Lam]

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