Hello Alan,
On 01/11/21 9:22 pm, Alan Bateman wrote:
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
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
