RFR(S) 8249276 CDS archived objects must have "neutral" markwords
Coleen Phillimore
coleen.phillimore at oracle.com
Tue Aug 11 16:37:39 UTC 2020
On 8/11/20 12:32 PM, Ioi Lam wrote:
> 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);
>
>
Great! Thank you. Looks good.
Coleen
> 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