jtreg
jtreg copied to clipboard
openjdk
7902847: Class directory of a test case should be always used to compile a library
The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag.
For first case Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.
For 2nd case The library classes are compiled using @build tag and library classes are placed into some shared location.
These 2 cases might be mixed in the single test and can have part of library classes compiled into shared directory and part compiled into test classes directory.
jtreg uses classfiles to check if source code should be compiled. Let we have 2 classes LibA and Lib that extends LibA. So if class LibA might be compiled into test class directory while LibB is compiled into library directory. Later jtreg can try to use LibB class without and compilation because LibB already exists. Thus the CNFE will be thrown.
The another possible problem, is that the library code might include test code for libraries like "/" and "/vmTestbase" so any test classes with same packages would be compiled into the same location because they are treated as library classes. So for tree like this: test1/Test.java test2/Test.java with both tests having
\@library "/"
\@build Test
we are going just to have one Test class in library directory.
The only reliable fix would be don't use shared class directory at all and compile all libraries for each test. Although it looks like huge performance overhead, the impact is low. The libraries are not often compiled using build tag.
Times for tier1 execution with fix: test-results/jtreg_test_lib_test_tier1/text/timeStats.txt:Total elapsed time 0m 28s test-results/jtreg_test_hotspot_jtreg_tier1/text/timeStats.txt:Total elapsed time 18m 14s test-results/jtreg_test_jdk_tier1/text/timeStats.txt:Total elapsed time 15m 8s test-results/jtreg_test_langtools_tier1/text/timeStats.txt:Total elapsed time 7m 59s
and before fix test-results/jtreg_test_lib_test_tier1/text/timeStats.txt:Total elapsed time 0m 32s test-results/jtreg_test_hotspot_jtreg_tier1/text/timeStats.txt:Total elapsed time 17m 51s test-results/jtreg_test_jdk_tier1/text/timeStats.txt:Total elapsed time 14m 49s test-results/jtreg_test_langtools_tier1/text/timeStats.txt:Total elapsed time 7m 56s
The full fix might require more testing and adding testcase. Please note that there are plans to work on the https://bugs.openjdk.org/browse/CODETOOLS-7903882 and https://bugs.openjdk.org/browse/JDK-8346058 So the libraries like '/test/lib' might be precompiled during build without tests and be used as classpath during test execution.
Progress
- [ ] Change must be properly reviewed (1 review required, with at least 1 Reviewer)
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
Issue
- CODETOOLS-7902847: Class directory of a test case should be always used to compile a library (Bug - P3)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jtreg.git pull/256/head:pull/256
$ git checkout pull/256
Update a local copy of the PR:
$ git checkout pull/256
$ git pull https://git.openjdk.org/jtreg.git pull/256/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 256
View PR using the GUI difftool:
$ git pr show -t 256
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jtreg/pull/256.diff
Using Webrev
:wave: Welcome back lmesnik! 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.
![bridgekeeper[bot] avatar](https://avatars.githubusercontent.com/u/41768318?v=4 "Published by a pub.dev verified publisher"){kind=small}
@lmesnik 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 details.
After integration, the commit message for the final commit will be:
7902847: Class directory of a test case should be always used to compile a library
Reviewed-by: cstein
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 3 new commits pushed to the master branch:
- a02f8b1087097917186ca0d952385324371eb7f1: 7904048: Update jtreg to use Ant 1.10.15
- caccd061414aee41168be66d1e67836377fe7cb6: 7904045: Remove the macOS version check from jtreg
- 9c22a6c31866299612199aa992e05af5d2580e3e: 7904046: Update jtreg to bundle JUnit 5.13.2
Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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.
As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@sormuras) but any other Committer may sponsor as well.
➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).
Webrevs
- 12: Full (cd30631a)
- 11: Full - Incremental (34372797)
- 10: Full - Incremental (271d58b3)
- 09: Full - Incremental (9dfaf822)
- 08: Full - Incremental (997c061e)
- 07: Full - Incremental (d0b28bff)
- 06: Full - Incremental (4d275a77)
- 05: Full - Incremental (cfe6aecb)
- 04: Full - Incremental (42b188e6)
- 03: Full - Incremental (532981df)
- 02: Full - Incremental (07a65a2d)
- 01: Full - Incremental (d5aca4dc)
- 00: Full (6079e8f2)
Hello Leonid,
Not a code review, but a general review about the proposal.
The only reliable fix would be don't use shared class directory at all and compile all libraries for each test.
By all libraries, do you mean all classes that reside under the directory tree of each of the @library that been declared on a specific test definition?
Although it looks like huge performance overhead, the impact is low.
The before/after numbers that you posted for the JDK repo look reasonable. JDK isn't the sole user of jtreg, but yes I think it's the largest user, so we might have to consider if this change needs to be configurable in some way to allow select projects (like the JDK) to opt-in to this new behaviour.
The libraries are not often compiled using build tag.
I have a contradicting understanding of that. In fact, the jtreg documentation here https://openjdk.org/jtreg/tag-spec.html for @library tag strongly recommends using @build for library classes used in the test:
In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes. A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled. It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile, and may lead to incomplete recompilation when a test or library code has been modified.
Hello Leonid,
Not a code review, but a general review about the proposal.
The only reliable fix would be don't use shared class directory at all and compile all libraries for each test.
By all libraries, do you mean all classes that reside under the directory tree of each of the
@librarythat been declared on a specific test definition?
Yes,
Although it looks like huge performance overhead, the impact is low.
The before/after numbers that you posted for the JDK repo look reasonable. JDK isn't the sole user of
jtreg, but yes I think it's the largest user, so we might have to consider if this change needs to be configurable in some way to allow select projects (like the JDK) to opt-in to this new behaviour.
Hmm, just curious, what are the other users.
The libraries are not often compiled using build tag.
I have a contradicting understanding of that. In fact, the jtreg documentation here https://openjdk.org/jtreg/tag-spec.html for
@librarytag strongly recommends using@buildfor library classes used in the test:In general, classes in library directories are not automatically compiled as part of a compilation command explicitly naming the source files containing those classes. A test that relies upon library classes should contain appropriate @build directives to ensure that the classes will be compiled. It is strongly recommended that tests do not rely on the use of implicit compilation by the Java compiler. Such an approach is generally fragile, and may lead to incomplete recompilation when a test or library code has been modified.
This part of documentation needs to be updated. The main issues is that requirement to use @build directive for each package or class used from library by test is overkill. Engineers prefer to rely on implicit compilation. So we have thousands of implicit usage of libraries in our tests. The explicit build is used usually only if test source doesn't depend on the library classes during compilation. Usually, if they are used by classes running with ProcessTools.
Might be all would work if jtreg disable implicit compilation library dependency and require to cover all classes by build but it overcomplicate tests.
Yes, it is a known and existing problem that "implicit" dependency doesn't call recompilation if library changes only. However, nothing can be done with it right now.
I also like to see an opt-out switch for this change.
Having at least a system property to enable the old and well-known behaviour, using absBaseClsDir, would be a good fall-back. Such a boolean switch could later be enhanced by either an option on the @library tag, and/or defined in a TEST.ROOT or TEST.properties configuration files.
Mailing list message from Jonathan Gibbons on jtreg-dev:
On 3/25/25 10:52 AM, Leonid Mesnik wrote:
The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag.
For first case Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.
Implicit compilation of the library classes into the test class directory is deplorable and should not be encourages.
For 2nd case The library classes are compiled using @build tag and library classes are placed into some shared location.
Generally, library classes should be independent of any test classes and test class directory. As such, @build is the recommended way to build libraries.
Class directory of a test case should be always used to compile a library
That would be an anti-pattern to be discouraged. Libraries are supposed to be shared between tests and as such, should not be beholden to any individual test class directory.
-- Jon
Mailing list message from Jonathan Gibbons on jtreg-dev:
It would be a worthwhile audit to examine test class directories after a test run to see whether any library clases have leaked into those directories.
A different approach would be to compile test classes with `-implicit:none` to ensure library classes do not leak into test class directories. See https://docs.oracle.com/en/java/javase/17/docs/specs/man/javac.html#option-implicit
-- Jon
On 4/1/25 12:04 PM, Jonathan Gibbons wrote:
Class directory of a test case should be always used to compile a library That would be an anti-pattern to be discouraged. Libraries are supposed to be shared between tests and as such, should not be beholden to any individual test class directory.
-- Jon
-------------- next part -------------- An HTML attachment was scrubbed... URL: <https://mail.openjdk.org/pipermail/jtreg-dev/attachments/20250401/0c9fea5d/attachment.htm>
Thank you for your feedback. See my comments inline.
Mailing list message from Jonathan Gibbons on jtreg-dev:
On 3/25/25 10:52 AM, Leonid Mesnik wrote:
The classes from test libraries can be compiled implicitly as a test dependency or explicitly with @build tag. For first case Test library used as a source path during test compilation and library classes as well as test classes are compiled into class directory.
Implicit compilation of the library classes into the test class directory is deplorable and should not be encourages.
Most of tests are doing this already. The people are not going to add build tags if test works without them. I don't think there is a way to encourage people to do this except by disabling implicit compilation. Even I do this once, it will be always the problematic to expect from jtreg test developers to add the build tags in new and update tests.
For 2nd case The library classes are compiled using @build tag and library classes are placed into some shared location.
Generally, library classes should be independent of any test classes and test class directory. As such, @build is the recommended way to build libraries.
Class directory of a test case should be always used to compile a library
That would be an anti-pattern to be discouraged. Libraries are supposed to be shared between tests and as such, should not be beholden to any individual test class directory.
That's also that is broken for a lot of cases and since it was not restricted, the library tag might refer to mix of library and test code.
So currently we have 2 type of libraries.
- Shared, test-independent libraries like /test/lib that could be shared as expected There is an RFE to support such type of libraries separately. It might be decrease performance of single test execution for single test first time. However, it allow to better control libraries, support projects like Valhalla and also have fully incremented build. https://bugs.openjdk.org/browse/CODETOOLS-7903882
2)The test-mixed libraries, like "/" or "vmTesbase/" that mix test and library code, and might even have same classes into different directories.
Ideally, we'll get rid of them and switch to using only N known libraries, but so far need to support them without any test failures. So this fix is intended to do. Having feature that allow user to break the execution is not the good idea, better to have performance/disk penalty. Requirement to always add @build for library classes is going to waste more developers time that I also prefer to avoid.
-- Jon
⚠️ @lmesnik This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).
@lmesnik This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a /touch or /keepalive command to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!
@sormuras The pull request is being re-evaluated and the inactivity timeout has been reset.
Please update this branch to the latest of openjdk/jtreg to include bug fixes and features introduced by https://github.com/lmesnik/jtreg/compare/7902847...openjdk%3Ajtreg%3Amaster
@lmesnik Your change (at version cd30631a5a10fee6769ce2da81bbf9b4ec6ea678) is now ready to be sponsored by a Committer.
Going to push as commit 0a8cbb5cfa6f0d82db4bae5843a066529c39dbf4.
Since your change was applied there have been 3 commits pushed to the master branch:
- a02f8b1087097917186ca0d952385324371eb7f1: 7904048: Update jtreg to use Ant 1.10.15
- caccd061414aee41168be66d1e67836377fe7cb6: 7904045: Remove the macOS version check from jtreg
- 9c22a6c31866299612199aa992e05af5d2580e3e: 7904046: Update jtreg to bundle JUnit 5.13.2
Your commit was automatically rebased without conflicts.
@sormuras @lmesnik Pushed as commit 0a8cbb5cfa6f0d82db4bae5843a066529c39dbf4.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.