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

[Ioi Lam]

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.

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
>>>