Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
On Thu, 4 Nov 2021 05:16:42 GMT, Jaikiran Pai 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: > > use Enum#name() for hashCode() generation instead of Enum#ordinal() FWIW, the GitHub Actions job failure for Windows is due to an unrelated reason: 2021-11-04T05:22:48.5273779Z /usr/bin/bash: line 1: 1174 Segmentation fault (core dumped) /usr/bin/make -s -r -R -I /cygdrive/d/a/jdk/jdk/jdk/make/common SPEC=/cygdrive/d/a/jdk/jdk/jdk/build/windows-x64/spec.gmk MAKE_LOG_FLAGS="-s" -f ModuleWrapper.gmk -I /cygdrive/d/a/jdk/jdk/jdk/make/common/modules -I/cygdrive/d/a/jdk/jdk/jdk/make/modules/java.base MODULE=java.base MAKEFILE_PREFIX=Copy 2021-11-04T05:22:48.5304517Z make[2]: *** [make/Main.gmk:157: java.base-copy] Error 139 2021-11-04T05:22:48.5305851Z make[2]: *** Waiting for unfinished jobs 2021-11-04T05:22:48.8158343Z Compiling 12 properties into resource bundles for jdk.jdeps 2021-11-04T05:22:49.2707959Z 2021-11-04T05:22:49.2712144Z ERROR: Build failed for target 'default (product-bundles test-bundles)' in configuration 'windows-x64' (exit code 2) 2021-11-04T05:22:49.8818435Z 2021-11-04T05:22:49.8823500Z No indication of failed target found. 2021-11-04T05:22:49.8845292Z Hint: Try searching the build log for '] Error'. 2021-11-04T05:22:49.9156326Z Hint: See doc/building.html#troubleshooting for assistance. 2021-11-04T05:22:49.9156822Z 2021-11-04T05:22:49.9206739Z make[1]: *** [/cygdrive/d/a/jdk/jdk/jdk/make/Init.gmk:315: main] Error 2 2021-11-04T05:22:49.9249924Z make: *** [/cygdrive/d/a/jdk/jdk/jdk/make/Init.gmk:186: default] Error 2 2021-11-04T05:22:50.0054100Z ##[error]Process completed with exit code 1. Going ahead with the merge of this PR. Thank you Alan and Magnus for the inputs and the reviews. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
On Thu, 4 Nov 2021 14:08:50 GMT, Alan Bateman wrote: > One final thought on this is whether we should remove the > tools/jlink/JLinkReproducibleXXX tests from the exclude list. Yes, I'm working on it in a separate PR. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
On Thu, 4 Nov 2021 05:16:42 GMT, Jaikiran Pai 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: > > use Enum#name() for hashCode() generation instead of Enum#ordinal() Marked as reviewed by ihse (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
On Thu, 4 Nov 2021 05:16:42 GMT, Jaikiran Pai 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: > > use Enum#name() for hashCode() generation instead of Enum#ordinal() One final thought on this is whether we should remove the tools/jlink/JLinkReproducibleXXX tests from the exclude list. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Thu, 4 Nov 2021 05:21:25 GMT, Jaikiran Pai wrote: > That's a good point and thank you for explaining that part. So to address > that, how about not leaving them out, but instead of using `Enum#ordinal()` > for the hashCode generation, we use `Enum#name()` which would translate to a > `String#hashCode()`. Moving to the hashCode of the names of the values is good. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Thu, 4 Nov 2021 03:18:40 GMT, Jaikiran Pai wrote:
> > So I decided to stick with using the ordinal modifiers in the hashCode()
> > computation. However, I'm not an expert on the performance aspects of the
> > Collections and how much using the modifiers for hashCode() will improve
> > the performance in this case. Perhaps that's what you meant by it not
> > giving a good (enough) spread? If so, do let me know and I'll go ahead and
> > update the PR to remove using the modifiers in the hashCode() computation
> > and also update the javadoc of each of those hashCode() methods which state
> > that the modifiers are used in the hashCode() computation.
>
> Okay, but we may have to re-visit this some time because summing the ordinals
> won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as
> TRANSITIVE. This is why I was saying it would be nearly the same to just
> leave them out.
That's a good point and thank you for explaining that part. So to address that,
how about not leaving them out, but instead of using `Enum#ordinal()` for the
hashCode generation, we use `Enum#name()` which would translate to a
`String#hashCode()`. Something like:
private static int modsHashCode(Iterable> enums) {
int h = 0;
for (Enum e : enums) {
h = h * 43 + Objects.hashCode(e.name());
}
return h;
}
That way the hashCode isn't dependent on the Object identity (which is what the
original issue was), is still reproducible across JVM runs and at the same tie
should provide a good spread for examples like the one you noted. Furthermore,
unlike the usage of `ordinal` where any reordering of the enum values would
have changed the hashCode value (which was OK), this usage of `name()` doesn't
exhibit that nature (which again should be OK).
I've updated the PR to use this construct. The test continues to pass. Let me
know if this looks fine or if you prefer doing it differently.
-
PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v12]
> 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: use Enum#name() for hashCode() generation instead of Enum#ordinal() - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/9dd2774c..a0c4f847 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=10-11 Stats: 7 lines in 1 file changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
Hello Alan, On 04/11/21 1:21 am, Alan Bateman wrote: On Wed, 3 Nov 2021 03:22:40 GMT, Jaikiran Pai wrote: So I decided to stick with using the ordinal modifiers in the hashCode() computation. However, I'm not an expert on the performance aspects of the Collections and how much using the modifiers for hashCode() will improve the performance in this case. Perhaps that's what you meant by it not giving a good (enough) spread? If so, do let me know and I'll go ahead and update the PR to remove using the modifiers in the hashCode() computation and also update the javadoc of each of those hashCode() methods which state that the modifiers are used in the hashCode() computation. Okay, but we may have to re-visit this some time because summing the ordinals won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as TRANSITIVE. This is why I was saying it would be nearly the same to just leave them out. So let's go with it for now. Thanks for the update to the test. The assertNotSame to check that they aren't the same instance looks a bit odd but at least you have a comment to explain it. I guess we should add an assertEquals to check that the 2 descriptors are equals too. Then I think we are done. I intentionally didn't add the assertEquals for the 2 module descriptors because that check will fail (with CDS enabled) until the other issue https://bugs.openjdk.java.net/browse/JDK-8275731 is fixed. -Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Wed, 3 Nov 2021 03:22:40 GMT, Jaikiran Pai wrote: > So I decided to stick with using the ordinal modifiers in the hashCode() > computation. However, I'm not an expert on the performance aspects of the > Collections and how much using the modifiers for hashCode() will improve the > performance in this case. Perhaps that's what you meant by it not giving a > good (enough) spread? If so, do let me know and I'll go ahead and update the > PR to remove using the modifiers in the hashCode() computation and also > update the javadoc of each of those hashCode() methods which state that the > modifiers are used in the hashCode() computation. Okay, but we may have to re-visit this some time because summing the ordinals won't give a good spread, e.g. STATIC+SYNTHETIC will give the same value as TRANSITIVE. This is why I was saying it would be nearly the same to just leave them out. So let's go with it for now. Thanks for the update to the test. The assertNotSame to check that they aren't the same instance looks a bit odd but at least you have a comment to explain it. I guess we should add an assertEquals to check that the 2 descriptors are equals too. Then I think we are done. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On 03/11/21 8:50 am, Jaikiran Pai wrote:
Hello Alan,
On 02/11/21 5:30 pm, Alan Bateman wrote:
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
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.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave
build reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple
and just test that the hashCode of the descriptors for modules in the
boot layer matches the hashCode computed from re-parsing the
module-info files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(),
descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
Sounds reasonable. I've updated the PR to simplify the test and run it
once with default CDS and once with -Xshare:off. Given this
simplification, it now allows me to use testng for these comparisons,
so I've moved it from regular "main" to testng test case. I hope
that's OK.
Test continues to pass with these changes and fails (as expected) when
the fix in the source code of ModuleDescriptor class is rolled back.
-Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
Hello Alan,
On 02/11/21 5:30 pm, Alan Bateman wrote:
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
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.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave build
reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple and
just test that the hashCode of the descriptors for modules in the boot
layer matches the hashCode computed from re-parsing the module-info
files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(), descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
Sounds reasonable. I've updated the PR to simplify the test and run it
once with default CDS and once with -Xshare:off. Given this
simplification, it now allows me to use testng for these comparisons, so
I've moved it from regular "main" to testng test case. I hope that's OK.
One other thought on the change to ModuleDescriptor is that we could
drop the modifiers from the computation of the hashCode of
ModuleDescriptor, Requires, Exports, Opens. Summing the ordinals is
okay but it doesn't give a good spread if you really have modules that
have everything the same except for the modifiers then it doesn't
really help.
I gave it a bit of thought. However I let the current PR to continue to
use ordinals for the hash code computation. The reason for that is this
statement on the Object.hashCode() method which says:
"the programmer should be aware that producing distinct integer results
for unequal objects may improve the performance of hash tables."
So if two module descriptors have all components equals() except for
their modifiers and if we don't use the modifiers in our hashCode()
computation, then they end up producing the same hashCode() which won't
violate the spec but will not satisfy the above statement about performance.
So I decided to stick with using the ordinal modifiers in the hashCode()
computation. However, I'm not an expert on the performance aspects of
the Collections and how much using the modifiers for hashCode() will
improve the performance in this case. Perhaps that's what you meant by
it not giving a good (enough) spread? If so, do let me know and I'll go
ahead and update the PR to remove using the modifiers in the hashCode()
computation and also update the javadoc of each of those hashCode()
methods which state that the modifiers are used in the hashCode()
computation.
-Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v11]
> 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: simplify the test based on review inputs - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/3552edf0..9dd2774c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=09-10 Stats: 91 lines in 1 file changed: 5 ins; 67 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On 02/11/2021 06:38, Jaikiran Pai wrote:
:
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.
I'm not sure that we really need this. There are several jlink tests
that check reproducibility, we think one of them is failing
intermittently with this issue already. So maybe we should leave build
reproducibility to the jlink tests and expand them as needed.
For ModuleDescriptor::hashCode then I was hoping we keep it simple and
just test that the hashCode of the descriptors for modules in the boot
layer matches the hashCode computed from re-parsing the module-info
files, e.g. something simple like this:
for (Module module : ModuleLayer.boot().modules()) {
System.out.println(module);
ModuleDescriptor descriptor1 = module.getDescriptor();
ModuleDescriptor descriptor2;
try (InputStream in =
module.getResourceAsStream("module-info.class")) {
descriptor2 = ModuleDescriptor.read(in);
}
assertEquals(descriptor1, descriptor2);
assertEquals(descriptor1.hashCode(), descriptor2.hashCode());
assertEquals(descriptor1.compareTo(descriptor2), 0);
}
It run can twice, with with the default, the other with -Xshare:off.
One other thought on the change to ModuleDescriptor is that we could
drop the modifiers from the computation of the hashCode of
ModuleDescriptor, Requires, Exports, Opens. Summing the ordinals is okay
but it doesn't give a good spread if you really have modules that have
everything the same except for the modifiers then it doesn't really help.
-Alan
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
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 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
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
Hello Alan, On 01/11/21 9:22 pm, Alan Bateman wrote: On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai 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
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v10]
> 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: Test case changes: - Test all boot layer modules instead of just the java.sql module - Reduce the number of processes launched for reproducibility testing from 50 to 2 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/dee4197f..3552edf0 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=08-09 Stats: 74 lines in 1 file changed: 16 ins; 35 del; 23 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
On Mon, 1 Nov 2021 03:10:45 GMT, Jaikiran Pai 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
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v9]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/e8f19d82..dee4197f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=07-08 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v8]
> 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: remove unintentional change in test case - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/065c3118..e8f19d82 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=06-07 Stats: 7 lines in 1 file changed: 0 ins; 6 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v7]
> 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: restructure the test case to move out the actual testing logic into the top level class - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/8cfcef13..065c3118 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=05-06 Stats: 48 lines in 1 file changed: 11 ins; 2 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
On 01/11/21 7:26 am, Jaikiran Pai wrote: Hello Alan, On 01/11/21 1:03 am, Alan Bateman wrote: On Fri, 29 Oct 2021 04:19:21 GMT, Jaikiran Pai wrote: Do you mean the jtreg `driver` style this test is using? I used it because it was necessary to compare output (the hashCode value) between multiple JVM runs. I use the `driver` along with the `ProcessBuilder` to capture the output between multiple JVM runs and compare those. No, I mean the test needs cleanup so it can easily be maintained. For starters I think the main method can take a parameter to indicate its the child process. HashCodeChecker.main becomes a method in ModuleDescriptorHashCodeTest. You'll see several examples of this in the test suite. I looked around some existing tests and I could find some which have a "realMain" and a "main" methods. But in all such test cases, the "realMain" is just called as a method within the same process. So I guess that's not the one you meant. I'll look around some more to try and find the expected style for such child processes. I think I might have understood what you meant. I have now updated the PR to restructure the test case accordingly. Please let me know if this looks fine or if this isn't what you meant. -Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v6]
> 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 four additional commits since the last revision: - review suggestion - rename assertTestPrerequisite to something more clearer - review suggestion - remove use of final in test case - review suggestion - rename the fromBootLayer method to something that signifies that it uses java.sql module. Plus add @modules java.sql to the test definition - review suggestion - change requirements to requires - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/60deab10..8cfcef13 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=04-05 Stats: 26 lines in 1 file changed: 1 ins; 0 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
Hello Alan, On 01/11/21 1:03 am, Alan Bateman wrote: On Fri, 29 Oct 2021 04:19:21 GMT, Jaikiran Pai wrote: Do you mean the jtreg `driver` style this test is using? I used it because it was necessary to compare output (the hashCode value) between multiple JVM runs. I use the `driver` along with the `ProcessBuilder` to capture the output between multiple JVM runs and compare those. No, I mean the test needs cleanup so it can easily be maintained. For starters I think the main method can take a parameter to indicate its the child process. HashCodeChecker.main becomes a method in ModuleDescriptorHashCodeTest. You'll see several examples of this in the test suite. I looked around some existing tests and I could find some which have a "realMain" and a "main" methods. But in all such test cases, the "realMain" is just called as a method within the same process. So I guess that's not the one you meant. I'll look around some more to try and find the expected style for such child processes. assertTestPrerequisite tests if the modules requires has modifiers so let's rename the method to make this clearer. Also "requirements" should be "requires". Fixed in the updated version of the PR. fromBootLayer returns the "java.sql" module so should be renamed to something clearer, or changed to take a parameter with the module name. The dependency Renamed fromBootLayer to javaSQLModuleFromBootLayer in the updated version of the PR. on the java.sql module means the test needs "@modules java.sql" Done. Style wise, the over-use of the "final" modifier is annoying, most/all of them aren't needed. I removed all the "final" modifiers in the updated version of the PR. Test continues to pass with these changes. -Jaikiran
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
On Fri, 29 Oct 2021 04:19:21 GMT, Jaikiran Pai wrote: > Do you mean the jtreg `driver` style this test is using? I used it because it > was necessary to compare output (the hashCode value) between multiple JVM > runs. I use the `driver` along with the `ProcessBuilder` to capture the > output between multiple JVM runs and compare those. No, I mean the test needs cleanup so it can easily be maintained. For starters I think the main method can take a parameter to indicate its the child process. HashCodeChecker.main becomes a method in ModuleDescriptorHashCodeTest. You'll see several examples of this in the test suite. assertTestPrerequisite tests if the modules requires has modifiers so let's rename the method to make this clearer. Also "requirements" should be "requires". fromBootLayer returns the "java.sql" module so should be renamed to something clearer, or changed to take a parameter with the module name. The dependency on the java.sql module means the test needs "@modules java.sql" Style wise, the over-use of the "final" modifier is annoying, most/all of them aren't needed. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
On Mon, 25 Oct 2021 09:21:28 GMT, Jaikiran Pai 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: > > add newline at end of test class Hello Alan, > I'd prefer not have commented out "TODO" code if possible. Done. I've updated the PR to remove that TODO. > I think the test will need a round or two of clean-up to get it more > consistent with the existing naming and style of tests in this area.? Do you mean the jtreg `driver` style this test is using? I used it because it was necessary to compare output (the hashCode value) between multiple JVM runs. I use the `driver` along with the `ProcessBuilder` to capture the output between multiple JVM runs and compare those. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v5]
> 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: remove commented out TODO in test - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/006255d9..60deab10 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=03-04 Stats: 5 lines in 1 file changed: 0 ins; 5 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
On 28/10/2021 05:43, Jaikiran Pai wrote: : Hello Alan, Looking at the CDS issue that's being tracked at https://bugs.openjdk.java.net/browse/JDK-8275731, it's looking like a much bigger change and might take a while. In the meantime do you think this test case (and the fix to the hashCode() part) looks OK? I am open to deleting the commented out equality check in this test case since although that equality testing should be done, it doesn't have to be done as part of this hashCode() fix/test PR. Let me know what you and others prefer. I think it's okay to move to using the ordinal of the hash code. I think the test will need a round or two of clean-up to get it more consistent with the existing naming and style of tests in this area. I'd prefer not have commented out "TODO" code if possible. -Alan.
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
On Fri, 22 Oct 2021 10:33:42 GMT, Jaikiran Pai wrote: >> src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 2559: >> >>> 2557: continue; >>> 2558: } >>> 2559: h += e.ordinal(); >> >> This e == null check suggests there is an issue elsewhere, can you explain >> the scenario where you see this? Also can you drop the spurious use of >> "final" here, it's inconsistent with the existing code. >> >> Do you want us to look at the test? It looks like it needs clean-up and I'm >> not sure if you want to wait for the CDS issue or not. > > Hello Alan, > >> This e == null check suggests there is an issue elsewhere, can you explain >> the scenario where you see this? > > That was just a pre-emptive defensive check I had added. There isn't a > scenario in that flow where any of the elements are currently null. I have > updated this PR to remove that check. Test continues to pass with this > change. Also removed the "final". > >> Do you want us to look at the test? It looks like it needs clean-up and I'm >> not sure if you want to wait for the CDS issue or not. > > Yes please. The test is ready for review. The only place where there's a > `TODO` is where that part of commented code is affected by the bug mentioned > in that comment. I would like to use that check (the commented out one) but I > don't want to wait for that other bug to be fixed for this PR, since I am > unsure how big or how long it might take for the other bug to be fixed. I > plan to uncomment that part in a separate PR once that other bug is fixed, if > that's OK. Hello Alan, Looking at the CDS issue that's being tracked at https://bugs.openjdk.java.net/browse/JDK-8275731, it's looking like a much bigger change and might take a while. In the meantime do you think this test case (and the fix to the hashCode() part) looks OK? I am open to deleting the commented out equality check in this test case since although that equality testing should be done, it doesn't have to be done as part of this hashCode() fix/test PR. Let me know what you and others prefer. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v3]
On Mon, 25 Oct 2021 08:04:43 GMT, Magnus Ihse Bursie wrote: >> Jaikiran Pai has updated the pull request incrementally with one additional >> commit since the last revision: >> >> drop another final to make it consistent with rest of the code > > test/jdk/java/lang/module/ModuleDescriptorHashCodeTest.java line 159: > >> 157: } >> 158: } >> 159: } > > It seems you're missing a newline at the EOF. Indeed. Updated the PR to fix it. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v4]
> 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: add newline at end of test class - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/89669ea4..006255d9 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v3]
On Fri, 22 Oct 2021 10:44:31 GMT, Jaikiran Pai 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: > > drop another final to make it consistent with rest of the code test/jdk/java/lang/module/ModuleDescriptorHashCodeTest.java line 159: > 157: } > 158: } > 159: } It seems you're missing a newline at the EOF. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v3]
> 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: drop another final to make it consistent with rest of the code - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/c4ad8067..89669ea4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v2]
On Fri, 22 Oct 2021 10:17:13 GMT, Alan Bateman wrote: >> Jaikiran Pai has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - drop final >> - no need to check for null values > > src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 2559: > >> 2557: continue; >> 2558: } >> 2559: h += e.ordinal(); > > This e == null check suggests there is an issue elsewhere, can you explain > the scenario where you see this? Also can you drop the spurious use of > "final" here, it's inconsistent with the existing code. > > Do you want us to look at the test? It looks like it needs clean-up and I'm > not sure if you want to wait for the CDS issue or not. Hello Alan, > This e == null check suggests there is an issue elsewhere, can you explain > the scenario where you see this? That was just a pre-emptive defensive check I had added. There isn't a scenario in that flow where any of the elements are currently null. I have updated this PR to remove that check. Test continues to pass with this change. Also removed the "final". > Do you want us to look at the test? It looks like it needs clean-up and I'm > not sure if you want to wait for the CDS issue or not. Yes please. The test is ready for review. The only place where there's a `TODO` is where that part of commented code is affected by the bug mentioned in that comment. I would like to use that check (the commented out one) but I don't want to wait for that other bug to be fixed for this PR, since I am unsure how big or how long it might take for the other bug to be fixed. I plan to uncomment that part in a separate PR once that other bug is fixed, if that's OK. - PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds [v2]
> 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 two additional commits since the last revision: - drop final - no need to check for null values - Changes: - all: https://git.openjdk.java.net/jdk/pull/6078/files - new: https://git.openjdk.java.net/jdk/pull/6078/files/f583dce3..c4ad8067 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6078=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6078=00-01 Stats: 4 lines in 1 file changed: 0 ins; 3 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/6078.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6078/head:pull/6078 PR: https://git.openjdk.java.net/jdk/pull/6078
Re: RFR: 8275509: ModuleDescriptor.hashCode isn't reproducible across builds
On Fri, 22 Oct 2021 09:33:35 GMT, Jaikiran Pai 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. src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 2559: > 2557: continue; > 2558: } > 2559: h += e.ordinal(); This e == null check suggests there is an issue elsewhere, can you explain the scenario where you see this? Also can you drop the spurious use of "final" here, it's inconsistent with the existing code. Do you want us to look at the test? It looks like it needs clean-up and I'm not sure if you want to wait for the CDS issue or not. - PR: https://git.openjdk.java.net/jdk/pull/6078
