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

[Ioi Lam]

On 8/6/20 5:55 PM, David Holmes wrote:
> 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),
>
>

Hi David,

(The Helper class was actually archived, because it was loaded by 
LockDuringDumpAgent during dump time, so it was not necessary to 
explicitly include it in the class list. ....)

Anyway, it wasn't clear to me whether any code (now or in the future) in 
the JVM might synchronize on the java mirrors. So I've updated the test 
case to synchronize on a literal string instead. This way I know that 
only the test, and no one else, will synchronize on it.

I also added comments to explain how the test works.

http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v03/

Thanks
- Ioi


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