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

[David Holmes]

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.

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
>