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

[Ioi Lam]

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

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