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

[David Holmes]

Hi Ioi,

Okay nothing further from me on the test.

Thanks,
David

On 11/08/2020 3:29 pm, Ioi Lam wrote:
> 
> 
> 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 
>>>
>>>
>>>
>