Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8253750: use build-stable default seed for Utils.RANDOM_GENERATOR #391

Closed
wants to merge 3 commits into from

Conversation

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 29, 2020

Hi all,

could you please review the patch which updates jdk.test.lib.Utils to use md5 hash-sum of java.vm.version property as default seed for Utils.RANDOM_GENERATOR?

from JBS:

using the same seed for all runs of a build will make it possible (easier) to compare results from different test runs (e.g. on different platforms, w/ different flags) and consequently will make test results analysis easier. the proposed solution is to use the seed based on Runtime.version() / "java.vm.version", which are different from build to build, if there is no seed specified by "jdk.test.lib.random.seed" property.

the patch also updates RandomGeneratorTest test, so it expects now that the same values are generated if no seed is provided.

testing:
tier1
test/lib-test/jdk/test/lib/ against personal build on linux,windows,macos-x64
test/lib-test/jdk/test/lib/ against CI build on linux,windows,macos-x64


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8253750: use build-stable default seed for Utils.RANDOM_GENERATOR

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/391/head:pull/391
$ git checkout pull/391

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 29, 2020

👋 Welcome back iignatyev! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@iignatev The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot-runtime

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Sep 29, 2020

/cc hotspot
/label remove hotspot-runtime

Loading

@openjdk openjdk bot added the hotspot label Sep 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@iignatev
The hotspot label was successfully added.

Loading

@openjdk openjdk bot removed the hotspot-runtime label Sep 29, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Sep 29, 2020

@iignatev
The hotspot-runtime label was successfully removed.

Loading

@iignatev iignatev marked this pull request as ready for review Sep 29, 2020
@openjdk openjdk bot added the rfr label Sep 29, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 29, 2020

Webrevs

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 2, 2020

ping?

Loading

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Oct 2, 2020

Is this really a good idea? The purpose of using random numbers is to get broader coverage on multiple runs.
If the seed only changes once per version (6 months), that reduces test coverage.
At least for dev submitted runs, I would like to be different for every build (unless overridden).

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 2, 2020

Mailing list message from Joe Darcy on core-libs-dev:

I agree with Roger that this change should *not* go forward since it
would have the effect of reducing test coverage.

Regression tests that use randomness should using the "randomness" jtreg
label and should output the seed value used so a failing result can be
replicated.

Thanks,

-Joe

On 10/2/2020 10:11 AM, Roger Riggs wrote:

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 2, 2020

Hi Roger,

it's exactly that you want it to be. It is different for every build (not every release, as we are using java.vm.version not java.version) unless overridden, each dev submitted run and each CI builds have different java.vm.version (e.g. the last two mach5 CI builds have 16-ea+19-938 and 16-ea+19-939 as their java.vm.version, one of my ad-hoc mach5 runs -- 16-internal+0-2020-10-01-2150482.igor.ignatyev.jdk, my local build -- 16-internal+0-2020-10-01-2252075.iignatye..., ) and hence would get different seeds. all the test tasks for these builds, on the other hand, would use the same seed, so one could be more confident that if test T passed on all platforms but platform P, it's platform P specific problem, there before this fix, one would need to rerun test T on each platform with the faling seed and on platform P with at least one passing seed, this problem becomes even more acute when you start considering all build flavor, vm flags, host configurations we cover in our testing.

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 2, 2020

Hi Joe,

this change indeed reduces test coverage, but not drastically (one can even argue that it doesn't reduce coverage as generally speaking you can merge coverage from runs with different configurations, e.g. run on different platforms), due to that and the reasons I wrote in my response to Roger, I don't think the reducing coverage is an issue here, nor do I think that possible impact on coverage overweights the benefits we get from comparable runs.

Loading

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

ok, if I read the code closely and know how the promoted build process works then I see your rationale.
Please update the bug report and edit the PR description to describe the conditions under which the seed
for random is computed from the build number. Its might be clearer to refer to the Runtime.Version build information
in the description. Also make it clear that unless the system property is set, it will use a 'random' seed.

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 2, 2020

@RogerRiggs , I have updated the code to use Runtime::version and updated the docs to better reflect how seed value is being set, as well as added some explanation to both the JBS issue and PR. please let me know if it's still not clear enough.

Loading

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

The code and description are reversed from what I suggested.

The seed should only be computed from the version information if it is a promoted or weekly build.
All other builds should use a random seed.

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 5, 2020

Hi @RogerRiggs

I guess I have misinterpreted your sentence about system property being set (as java.vm.version is always set and the only other property which mattered in this context is jdk.test.lib.random.seed). in any case, I agree that for personal builds, it's more desirable to have different seeds on each execution, especially given the fact that the version string is set at configure-time and not at build-time, so one might end up with the same version string for a very long time.

I have reworked the code a bit, so now version-based seed is used only for promotable builds (i.e. ones that have build number and it's greater than 0); local and remote/mach5 ad-hoc builds (by default) don't specify a build number, so a random seed value will be used for them. and as before, if jdk.test.lib.random.seed is set, its value will be used as seed oblivious to build type.

Loading

Copy link
Contributor

@RogerRiggs RogerRiggs left a comment

Can you take a look at the existing jdk.test.lib.RandomFactory and see if we can avoid a separate random number generator and property?

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 5, 2020

Can you take a look at the existing jdk.test.lib.RandomFactory and see if we can avoid a separate random number generator and property?

I am not adding a new separate random number generator or a new property, jdk.test.lib.Utils.getRandomInstance has been around for a long time and is used by lots of tests (most of them are in /test/hotspot/jtreg). merging Utils.getRandomInstance and RandomFactory (or rather replacing one w/ another) is tracked by JDK-8212077.

Loading

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Oct 5, 2020

I was thinking that the code added to Utils.SEED initialization could just have easily been added to RandomFactory.getSystemSeed() and not have to be changed later.

Loading

@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 5, 2020

well, RandomFactory::getSystemSeed reads seed property, while Utils.SEED reads jdk.test.lib.random.seed, and there are tests which use these properties to specify seeds, I'd prefer to leave changing these tests to 8212077. o/c we can add a new method, e.g. RandomFactory::getBuildStableSeed, or refactor RandomFactory::getSystemSeed to take property name as an argument, but I don't think it's really worth it as 8212077 is starting to slowly bubble up in my working queue.

Loading

@RogerRiggs
Copy link
Contributor

@RogerRiggs RogerRiggs commented Oct 6, 2020

If the factories duplication will get combined/resolved with 8212077, I'll leave that up to you.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

@iignatev This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for more details.

After integration, the commit message for the final commit will be:

8253750: use build-stable default seed for Utils.RANDOM_GENERATOR

Reviewed-by: rriggs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 104 new commits pushed to the master branch:

  • a34f48b: 8253832: CharsetDecoder : decode() mentioning CoderMalfunctionError behavior not as per spec
  • f397b60: 8251123: doclint warnings about missing javadoc tags and comments
  • c9d1dcc: 8253902: G1: Starting a new marking cycle before the conc mark thread fully completed causes assertion failure
  • 9199783: 8253565: PPC64: Fix duplicate if condition in vm_version_ppc.cpp
  • 1728547: 8254010: GrowableArrayView::print fails to compile
  • 6e61861: 8254046: Remove double semicolon introduced by JDK-8235521
  • 5d84e95: 8204256: improve jlink error message to report unsupported class file format
  • 4fe68f5: 8253426: jpackage is unable to generate working EXE for add-launcher configurations
  • c9d0407: 8253794: TestAbortVMOnSafepointTimeout never timeouts
  • f2f77f7: 8253761: Wrong URI syntax printed by jar --describe-module
  • ... and 94 more: https://git.openjdk.java.net/jdk/compare/70b0fccf79ac7193b36c49aff0286fdc09bb370c...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Loading

@openjdk openjdk bot added the ready label Oct 6, 2020
@iignatev
Copy link
Member Author

@iignatev iignatev commented Oct 6, 2020

Thanks, Roger.

/integrate

Loading

@openjdk openjdk bot closed this Oct 6, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 6, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Oct 6, 2020

@iignatev Since your change was applied there have been 112 commits pushed to the master branch:

  • 6712f8c: 8254084: Remove TemplateTable::pd_initialize
  • 82fe023: 8254061: Missing space in flag description
  • bd50ccd: 8253735: Cleanup SearchIndexItem API
  • 54b340b: 8254081: java/security/cert/PolicyNode/GetPolicyQualifiers.java fails due to an expired certificate
  • 51fdb4c: 8254075: Shenandoah: Remove ShenandoahCodeRootsStyle diagnostic flag and related test
  • 77921b9: 8254080: fix for JDK-8204256 causes jlink test failures
  • 57493c1: 8253694: Remove Thread::muxAcquire() from ThreadCrashProtection()
  • d2b1dc6: 8254054: Pre-submit testing using GitHub Actions should not use the deprecated set-env command
  • a34f48b: 8253832: CharsetDecoder : decode() mentioning CoderMalfunctionError behavior not as per spec
  • f397b60: 8251123: doclint warnings about missing javadoc tags and comments
  • ... and 102 more: https://git.openjdk.java.net/jdk/compare/70b0fccf79ac7193b36c49aff0286fdc09bb370c...master

Your commit was automatically rebased without conflicts.

Pushed as commit ac772cd.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Loading

@iignatev iignatev deleted the 8253750 branch Oct 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
2 participants