RFR(S) 8249276 CDS archived objects must have "neutral" markwords
Coleen Phillimore
coleen.phillimore at oracle.com
Fri Aug 7 00:50:42 UTC 2020
This looks good.
Coleen
On 8/5/20 6:26 PM, 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/
>
>
>
> 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