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

[David Holmes]

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

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