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 [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 [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 [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
