RFR(S) 8241071 Generation of classes.jsa is not deterministic

[David Holmes]

Hi Ioi,

On 5/05/2020 3:43 am, Ioi Lam wrote:
> 
> 
> On 4/27/20 7:05 PM, David Holmes wrote:
>> On 28/04/2020 10:01 am, David Holmes wrote:
>>> On 28/04/2020 9:37 am, Ioi Lam wrote:
>>>> Hi David & Jiangli,
>>>>
>>>> Thanks for your comments.
>>>>
>>>> I thought about using a system property, but the user can also 
>>>> specify it like
>>>>
>>>>      java -Djdk.xshare.dump.salt=0 MyProgram
>>>
>>> There's a way to pass properties from the VM that the user can't 
>>> override. I'll need to lookup the details.
>>
>> It should suffice to define SystemProperty that is non-writeable and 
>> internal (for good measure). But it seems we have introduced a bug 
>> since Java 9 whereby a non-writeable property is actually being 
>> overwritten! :(
>>
>> David
>>
> 
> Hi David,
> 
> I tried implementing the seed as a system property. However, 
> ImmutableCollections.<clinit>() is executed very early during JDK 
> bootstrap (about 500-th bytecode), much earlier than when System.props 
> is initialized (about 43689-th bytecode). So attempts to read the system 
> property would give a NullPointerException
> 
> Error occurred during initialization of VM
> java.lang.ExceptionInInitializerError
>      at 
> java.util.ImmutableCollections$Set12.<init>(java.base/ImmutableCollections.java:626) 
> 
>      at java.util.Set.of(java.base/Set.java:468)
>      at java.lang.Module.<clinit>(java.base/Module.java:251)
> Caused by: java.lang.NullPointerException
>      at java.lang.System.getProperty(java.base/System.java:834)
>      at 
> java.util.ImmutableCollections.<clinit>(java.base/ImmutableCollections.java:80) 
> 
>      at 
> java.util.ImmutableCollections$Set12.<init>(java.base/ImmutableCollections.java:626) 
> 
>      at java.util.Set.of(java.base/Set.java:468)
>      at java.lang.Module.<clinit>(java.base/Module.java:251)
> 
> Because of this, I have to keep using the JVM_GetRandomSeedForCDSDump() 
> native function.

Okay - thanks for trying the alternative. Initialization is always tricky.

David

> Thanks
> - Ioi
> 
>>> David
>>>
>>>> This would circumvent the calculation of 
>>>> ImmutableCollections.SALT32L for normal execution. I am not sure if 
>>>> this is desirable, or if there's a way to prevent the user from 
>>>> doing that. I'll see what the core lib folks suggest.
>>>>
>>>> Updated webrev:
>>>> http://cr.openjdk.java.net/~iklam/jdk15/8241071-deterministic-cds-archive.v03/ 
>>>>
>>>> http://cr.openjdk.java.net/~iklam/jdk15/8241071-deterministic-cds-archive.v03.delta/ 
>>>>
>>>>
>>>> More comments below
>>>>
>>>>
>>>> On 4/26/20 11:20 PM, David Holmes wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> On 27/04/2020 3:22 pm, Ioi Lam wrote:
>>>>>> https://bugs.openjdk.java.net/browse/JDK-8241071
>>>>>> http://cr.openjdk.java.net/~iklam/jdk15/8241071-deterministic-cds-archive.v02/ 
>>>>>>
>>>>>>
>>>>>> The goal is to for "java -Xshare:dump" to produce deterministic 
>>>>>> contents in
>>>>>> the CDS archive that depend only on the contents of 
>>>>>> $JAVA_HOME/lib/modules.
>>>>>>
>>>>>> Problems:
>>>>>> [1] Symbols in the CDS archive may have non-deterministic order 
>>>>>> because
>>>>>>      Arena allocation is non-deterministic.
>>>>>> [2] The contents of the CDS shared heap region may be randomized 
>>>>>> due to
>>>>>>      ImmutableCollections.SALT32L.
>>>>>>
>>>>>> Fixes:
>>>>>> [1] With -Xshare:dump, allocate Symbols from the class space 
>>>>>> (64-bit only).
>>>>>
>>>>> I'm not seeing what restricts this to 64-bit only ??
>>>>
>>>> The last version of the patch allocated using 
>>>> MetaspaceObj::ClassType. On 64-bit+compressed classes, this will 
>>>> allocate from the class space.
>>>>
>>>> But anyway, as Thomas commented, doing this is problematic.
>>>>
>>>>>
>>>>> Will changing the allocation of symbols cause any problems with the 
>>>>> amount of "class space" we need?
>>>>>
>>>>
>>>> Per Thomas's comment, I moved the allocation of Symbols in a 
>>>> separate virtual space. Please see my comments to Thomas's e-mail.
>>>>
>>>>>>      See changes in symbol.cpp for details.
>>>>>> [2] When running the VM with -Xshare:dump, 
>>>>>> ImmutableCollections.SALT32L is
>>>>>>      initialized with a deterministic seed. NOTE: this affects 
>>>>>> ONLY when the
>>>>>>      VM is running with the special flag -Xshare:dump to dump the 
>>>>>> CDS archive.
>>>>>>      It does NOT affect normal execution of Java programs.
>>>>>
>>>>> I can't say I like adding in the VM call for this. Another 
>>>>> alternative would be a VM defined system property. But I'll let 
>>>>> core-libs folk make the call here.
>>>>>
>>>>>> ---
>>>>>> I also cleaned up the -Xlog:cds output and print out the CRC of each
>>>>>> CDS region, to help diagnose why two CDS archives may have 
>>>>>> different contents.
>>>>>
>>>>> Generally seems okay. A couple of minor nits:
>>>>>
>>>>> src/hotspot/share/oops/symbol.cpp
>>>>>
>>>>> typo: adresses -> addresses
>>>>>
>>>>
>>>> Fixed
>>>>
>>>>> test/hotspot/jtreg/runtime/cds/DeterministicDump.java
>>>>>
>>>>> Should the test use @driver mode?
>>>>>
>>>>
>>>> Fixed
>>>>
>>>>>  39 // use of SharedArchiveFile argument.
>>>>>
>>>>> s/argument/arguments/
>>>>>
>>>>>  40 // methods to form command line to create/use shared archive.
>>>>>
>>>>> s/line/lines/
>>>>>
>>>>> s/shared/the shared/
>>>>>
>>>>
>>>> I copied these comments from another test. They are not needed here 
>>>> so I deleted them in the new webrev.
>>>>
>>>>>  71 if (n0 == 0) {
>>>>>
>>>>> Shouldn't this check come before the loop at line 63?
>>>>
>>>> Fixed.
>>>>
>>>>
>>>> Thanks
>>>> - Ioi
>>>>>
>>>>> Thanks,
>>>>> David
>>>>> -----
>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>
>