RFR(S) 8249276 CDS archived objects must have "neutral" markwords
David Holmes
david.holmes at oracle.com
Fri Aug 7 00:55:40 UTC 2020
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
>
>
>
More information about the hotspot-runtime-dev
mailing list