RFR(S) 8249276 CDS archived objects must have "neutral" markwords
Ioi Lam
ioi.lam at oracle.com
Tue Aug 11 16:32:01 UTC 2020
Hi Coleen,
Thanks for the review. I've changed the code to the following, and
removed the LockDuringDumpAgent.mf file
+ private static final String MANIFEST =
+ "Manifest-Version: 1.0\nPremain-Class: LockDuringDumpAgent\n";
+
public static void main(String[] args) throws Throwable {
String agentJar =
ClassFileInstaller.writeJar("LockDuringDumpAgent.jar",
- ClassFileInstaller.Manifest.fromSourceFile("LockDuringDumpAgent.mf"),
+ ClassFileInstaller.Manifest.fromString(MANIFEST),
agentClasses);
Thanks
- Ioi
On 8/11/20 9:09 AM, Coleen Phillimore wrote:
>
> http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v03/test/hotspot/jtreg/runtime/cds/appcds/javaldr/LockDuringDumpAgent.mf.html
>
>
> This doesn't have a copyright. There are tests like
> test/hotspot/jtreg/serviceability/jvmti/GetObjectSizeClass.java that
> create the manifest file inside the .java source code so you don't
> need this file, which I think you can do here:
>
> http://cr.openjdk.java.net/~iklam/jdk16/8249276-reset-archive-obj-headers.v03/test/hotspot/jtreg/runtime/cds/appcds/javaldr/LockDuringDump.java.html
>
>
> Coleen
>
> On 8/11/20 1:29 AM, 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
>>>>
>>>>
>>>>
>>
>
More information about the hotspot-runtime-dev
mailing list