On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai <j...@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 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 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. ------------- PR: https://git.openjdk.java.net/jdk/pull/6078
