RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]

[Jaikiran Pai]

On 02/11/21 10:29 am, Jaikiran Pai wrote:
> Hello Alan,
>
> On 01/11/21 9:22 pm, Alan Bateman wrote:
>> On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:
>>
>>>> Can I please get a review for this change which fixes the issue 
>>>> reported in https://bugs.openjdk.java.net/browse/JDK-8275509?
>>>>
>>>> The `ModuleDescriptor.hashCode()` method uses the hash code of its 
>>>> various components to compute the final hash code. While doing so 
>>>> it ends up calling hashCode() on (collection of) various `enum` 
>>>> types. Since the hashCode() of enum types is generated from a call 
>>>> to `java.lang.Object.hashCode()`, their value isn't guaranteed to 
>>>> stay the same across multiple JVM runs.
>>>>
>>>> The commit here proposes to use the ordinal of the enum as the hash 
>>>> code. As Alan notes in the mailing list discussion, any changes to 
>>>> the ordinal of the enum (either due to addition of new value, 
>>>> removal of a value or just reordering existing values, all of which 
>>>> I think will be rare in these specific enum types) isn't expected 
>>>> to produce the same hash code across those changed runtimes and 
>>>> that should be okay.
>>>>
>>>> The rest of the ModuleDescriptor.hashCode() has been reviewed too 
>>>> and apart from calls to the enum types hashCode(), rest of the 
>>>> calls in that implementation, either directly or indirectly end up 
>>>> as calls on `java.lang.String.hashCode()` and as such aren't 
>>>> expected to cause non-deterministic values.
>>>>
>>>> A new jtreg test has been added to reproduce this issue and verify 
>>>> the fix.
>>> Jaikiran Pai has updated the pull request incrementally with one 
>>> additional commit since the last revision:
>>>
>>>    better method name in test class
>> Thanks for the update to the test. Instead of launching 50 VMs, what 
>> would you think about tossing the use of ProcessTools and just 
>> changing the test description so that the test runs twice, once with 
>> the default, the second with CDS disabled, as, in:
>>
>> @run ModuleDescriptorHashCodeTest
>> @run main/othervm -Xshare:off ModuleDescriptorHashCodeTest
>
> When I started off with this test, I had thought of a similar 
> construct. However, in order to compare the hash code value generated 
> across JVM runs (i.e. the one generated in @run 1 against the one 
> generated in @run 2), I would need to somehow hold on to that dynamic 
> value/result for comparison. Using the @run construct wouldn't allow 
> me to do that. So I decided to use the ProcessTools library to have 
> more control over it. If I missed some way to still use @run for such 
> a test, do let me know, I'll change it accordingly.

Perhaps run 1 writing the hash code of each of the boot modules' 
descriptor into a file and then run 2 reading that same file to compare 
the hash codes would be one way to do it. But I think that would just 
make this test more complex, which I think is avoidable.

-Jaikiran