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

[Jaikiran Pai]

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.

> The tests are run by many people every day, they are run continuously on all platforms in several CI systems. So I think you'll get much more variability that way rather than launching 50 VMs.

I have updated the PR to reduce the runs from 50 to just 2. Once with 
default CDS and once with CDS disabled.

> I also feel like the test could be re-written to test the module descriptors of all modules in the boot layer. That would avoid having a dependency on the java.sql module as it would be testing every module.

I think that's a good point and will make this test case cover more 
modules. I've updated the PR to now cover all these boot layer modules.

-Jaikiran