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

[David Holmes]

Hi Ioi,

But one further follow up in relation to biased-locking code. This 
assertion is invalid:

  104 unsigned HeapShared::oop_hash(oop const& p) {
  105   assert(!p->mark().has_bias_pattern(),
  106          "this object should never have been locked");  // so 
identity_hash won't safepoin
  107   unsigned hash = (unsigned)p->identity_hash();
  108   return hash;
  109 }

As I discovered having the "bias pattern" doesn't mean you have been 
locked, it can mean you're eligible to be biased-locked (ie you are 
anonymously biased). So if you enable BL for CDS dump (and fix the 
asserts I'm working on) you will still hit this assertion.

Cheers,
David

On 11/08/2020 7:31 pm, David Holmes wrote:
> 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 
>>>>
>>>>
>>>>
>>