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

8273314: Add tier4 test groups #5357

Closed
wants to merge 5 commits into from
Closed

Conversation

shipilev
Copy link
Contributor

@shipilev shipilev commented Sep 3, 2021

During the review of JDK-8272914 that added hotspot:tier{2,3} groups, @iignatev suggested to create tier4 groups that capture all tests not in tiers{1,2,3}.

Caveats:

  • I excluded applications from hotspot:tier4, because they require test dependencies (e.g. jcstress).
  • jdk:tier4 only runs well with JTREG_KEYWORDS=\!headful or reduced concurrency with TEST_JOBS=1, because headful tests cannot run in parallel

Sample run with JTREG_KEYWORDS=\!headful:

==============================
Test summary
==============================
   TEST                                              TOTAL  PASS  FAIL ERROR   
>> jtreg:test/hotspot/jtreg:tier4                     3585  3584     0     1 <<
>> jtreg:test/jdk:tier4                               2893  2887     5     1 <<
   jtreg:test/langtools:tier4                            0     0     0     0   
   jtreg:test/jaxp:tier4                                 0     0     0     0   
==============================

real	699m39.462s
user	6626m8.448s
sys	1110m43.704s

There are interesting test failures on my machine, which I would address separately.


Progress

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

Issue

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5357/head:pull/5357
$ git checkout pull/5357

Update a local copy of the PR:
$ git checkout pull/5357
$ git pull https://git.openjdk.java.net/jdk pull/5357/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 5357

View PR using the GUI difftool:
$ git pr show -t 5357

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5357.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 3, 2021

👋 Welcome back shade! 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.

@openjdk openjdk bot added the rfr label Sep 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 3, 2021

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

  • compiler
  • core-libs
  • hotspot

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.

@openjdk openjdk bot added hotspot core-libs compiler labels Sep 3, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 3, 2021

Webrevs

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 3, 2021

<...> I have excluded vmTestbase and hotspot:tier4<...> I have also excluded applications from hotspot:tier4 <...>

assuming the goal of tier4 is to catch the rest of the tests, I don't think we should exclude vmTestbase, applications or any other tests from tier4. unless you also want to create tier5 for them.

-- Igor

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 3, 2021

<...> I have excluded vmTestbase and hotspot:tier4<...> I have also excluded applications from hotspot:tier4 <...>

assuming the goal of tier4 is to catch the rest of the tests, I don't think we should exclude vmTestbase, applications or any other tests from tier4. unless you also want to create tier5 for them.

Apart from practicality of using tier4, I think vmTestbase and applications are separate test suites in their own right. tier4 is catching all the assorted Hotspot tests that are not part of larger suites. Makes sense?

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 4, 2021

it looks like the results above do not include the headful tests did you filter them out?

jtreg:test/jdk:tier4 2891 2885 4 2 <<

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 4, 2021

<...> I have excluded vmTestbase and hotspot:tier4<...> I have also excluded applications from hotspot:tier4 <...>

assuming the goal of tier4 is to catch the rest of the tests, I don't think we should exclude vmTestbase, applications or any other tests from tier4. unless you also want to create tier5 for them.

Apart from practicality of using tier4, I think vmTestbase and applications are separate test suites in their own right. tier4 is catching all the assorted Hotspot tests that are not part of larger suites. Makes sense?

to some extent. I agree that applications tests can/should be seen as a separate test suite, yet I differ on vmTestbase as the end goal for vmTestbase tests is (and always was) to become just another test within hotspot jtreg test suite, hence I don't think we should treat them any different than other jtreg tests. there is also a plan (which I need to formalize and share w/ a broader audience) to rearrange vmTestbase tests so they will be placed within the corresponding component subdirectories, which would bring us closer to the end goal and at the same time make it slightly harder to select all vmTestbase tests.

-- Igor

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 5, 2021

Hi Aleksey,

This seems rather arbitrary and subjective to me. The tier 1-3 groupings were driven by existing tier 1-3 notions. But here the definition of tier 4 as "all the rest except ..." is not really a well-defined meaning for tier 4. I don't see that it adds any value. Perhaps there is a need for a group that is "everything except tier 1, tier 2, tier 3, applications, ..." but I wouldn't call that tier 4.

Cheers,
David

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 6, 2021

@iignatev: OK, I reinstated vmTestbase and left applications out of hotspot:tier4.
@dholmes-ora: Generally speaking, all tierX definitions are rather arbitrary, as there seem to be nothing intrinsic about the tests to be in a particular tier. In other words, what tierX consists of is a matter of agreement. I'd say hotspot:tier4 is "all assorted Hotspot tests that are not application-specific suites"
@mrserb: Yes, I ran this on a headless config, so headful tests were skipped, apparently. I'll try to arrange runs on my desktop.

Please see new commit. As hotspot:tier4 is now much longer, it would take me a while to verify everything works there.

@mrserb
Copy link
Member

@mrserb mrserb commented Sep 6, 2021

@mrserb: Yes, I ran this on a headless config, so headful tests were skipped, apparently. I'll try to arrange runs on my desktop.

Then you probably need to skip "printer" tests as well. BTW it will be really good somehow to execute headless tests in tier4 concurrently, and run the headful tests in tier5 sequentially.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 6, 2021

Mailing list message from David Holmes on hotspot-dev:

On 7/09/2021 1:17 am, Aleksey Shipilev wrote:

@dholmes-ora: Generally speaking, all `tierX` definitions are rather arbitrary, as there seem to be nothing intrinsic about the tests to be in a particular tier. In other words, what `tierX` consists of is a matter of agreement. I'd say `hotspot:tier4` is "all assorted Hotspot tests that are not application-specific suites"

The difference is that your previous work just consolidated the existing
subsystem tier 1-3 definitions, but here you are choosing to define "all
the rest" as tier 4. I don't think it is actually helpful/useful to
anyone - and it bears no resemblance whatsoever to what we call "tier
4", so that will just lead to unnecessary confusion IMO.

David

@iignatev
Copy link
Member

@iignatev iignatev commented Sep 7, 2021

Mailing list message from David Holmes on hotspot-dev:

On 7/09/2021 1:17 am, Aleksey Shipilev wrote:

@dholmes-ora: Generally speaking, all tierX definitions are rather arbitrary, as there seem to be nothing intrinsic about the tests to be in a particular tier. In other words, what tierX consists of is a matter of agreement. I'd say hotspot:tier4 is "all assorted Hotspot tests that are not application-specific suites"

The difference is that your previous work just consolidated the existing
subsystem tier 1-3 definitions, but here you are choosing to define "all
the rest" as tier 4. I don't think it is actually helpful/useful to
anyone - and it bears no resemblance whatsoever to what we call "tier
4", so that will just lead to unnecessary confusion IMO.

@dholmes-ora , although I fully agree that this might lead to some misunderstanding b/w Oracle and non-Oracle folks, I don't see how it's different from the previous patch, which introduced hotspot:tier2 and hotspot:tier3. even if we reduce tierN to just a set of tests, the test groups added by 8272914 bear as much resemblance to the test sets used in Oracle's tier2-3 as the suggested hotspot:tier4 groups in this patch to the actual tier4 definition used in Oracle's internal system, e.g. hotspot:tier2 group has 0 tests from test/hotspot/jtreg/compiler, but Oracle's tier2 does include a number of test/hotspot/jtreg/compiler tests (which aren't part of :tier1). I believe that this patch actually moves us closer to a convergence point, as the union of hotspot:tier1 -- hotspot:tier4 test groups is very close to the test sets used in hotspot parts of Oracle's tier1 -- tier4 definitions.

Thanks,
-- Igor

@mlbridge
Copy link

@mlbridge mlbridge bot commented Sep 7, 2021

Mailing list message from David Holmes on hotspot-dev:

On 7/09/2021 2:20 pm, Igor Ignatyev wrote:

On Mon, 6 Sep 2021 13:22:03 GMT, Aleksey Shipilev <shade at openjdk.org> wrote:

During the review of JDK-8272914 that added hotspot:tier{2,3} groups, @iignatev suggested to create tier4 groups that capture all tests not in tiers{1,2,3}. I have excluded `vmTestbase` and `hotspot:tier4,` because they take 10+ hours on my highly parallel machine. I have also excluded `applications` from `hotspot:tier4`, because they require test dependencies (e.g. jcstress).

Sample run:

==============================
Test summary
==============================
TEST TOTAL PASS FAIL ERROR

jtreg:test/hotspot/jtreg:tier4 426 425 1 0 <<
jtreg:test/jdk:tier4 2891 2885 4 2 <<
jtreg:test/langtools:tier4 0 0 0 0
jtreg:test/jaxp:tier4 0 0 0 0
==============================

real 64m13.994s
user 1462m1.213s
sys 39m38.032s

There are interesting test failures on my machine, which I would address separately.

Aleksey Shipilev has updated the pull request incrementally with one additional commit since the last revision:

Drop applications and fix the comment

_Mailing list message from [David Holmes](mailto:david.holmes at oracle.com) on [hotspot-dev](mailto:hotspot-dev at mail.openjdk.java.net):_

On 7/09/2021 1:17 am, Aleksey Shipilev wrote:

@dholmes-ora: Generally speaking, all `tierX` definitions are rather arbitrary, as there seem to be nothing intrinsic about the tests to be in a particular tier. In other words, what `tierX` consists of is a matter of agreement. I'd say `hotspot:tier4` is "all assorted Hotspot tests that are not application-specific suites"

The difference is that your previous work just consolidated the existing
subsystem tier 1-3 definitions, but here you are choosing to define "all
the rest" as tier 4. I don't think it is actually helpful/useful to
anyone - and it bears no resemblance whatsoever to what we call "tier
4", so that will just lead to unnecessary confusion IMO.

@dholmes-ora , although I fully agree that this might lead to some misunderstanding b/w Oracle and non-Oracle folks, I don't see how it's different from the previous patch, which introduced `hotspot:tier2` and `hotspot:tier3`.

Because hotspot:tier2 and hotspot:tier3 simply grouped existing
component definitions for tiers 2 and 3:

+tier2 = \
+ :hotspot_tier2_runtime \
+ :hotspot_tier2_runtime_platform_agnostic \
+ :hotspot_tier2_serviceability \
+ :tier2_gc_epsilon \
+ :tier2_gc_shenandoah
+
+tier3 = \
+ :hotspot_tier3_runtime \
+ :tier3_gc_shenandoah

but that is not the case for tier4.

even if we reduce `tierN` to just a set of tests, the test groups added by 8272914 bear as much resemblance to the test sets used in Oracle's tier2-3 as the suggested `hotspot:tier4` groups in this patch to the actual `tier4` definition used in Oracle's internal system, e.g. `hotspot:tier2` group has 0 tests from `test/hotspot/jtreg/compiler`, but Oracle's `tier2` does include a number of `test/hotspot/jtreg/compiler` tests (which aren't part of `:tier1`). I believe that this patch actually moves us closer to a convergence point, as the union of `hotspot:tier1` -- `hotspot:tier4` test groups is very close to the test sets used in hotspot parts of Oracle's `tier1` -- `tier4` definitions.

We can discuss Oracle's tier definitions privately - I don't see any
connection between those and this tier4 however.

Thanks,
David
-----

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 7, 2021

Once again, the disconnect between Oracle and OpenJDK test definitions seems to be the problem for Oracle's side. Rectifying that disconnect might fall under the scope of this PR, but I have to point out that it is a courtesy of upstream open-source project to care about proprietary downstream definitions.

More to the point: since tier4 as "catch-all-the-rest" was Igor's idea, I assumed that would be in agreement with Oracle's test definitions. Following this discussion, it seems I assumed wrong. So it puts me in a weird position to be between two Oracle engineers arguing about proprietary test definitions I cannot really know about, and have no decision power about. For all I care for OpenJDK, we might as well model tier4 after what Oracle does, as to minimize confusion for Oracle engineers. But then again, I have no idea what Oracle means by tier4. So as the alternative, I can postpone this PR until you folks have a coherent view on this, or I can just give up on this PR and re-assign the RFE to Igor, assuming he is willing to work this out.

Tell me what you want me to do here.

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 8, 2021

Hi Aleksey,

I've discussed this with Igor and while I don't agree with the rationale I won't "block it".

Cheers,
David

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 15, 2021

Progress:
a) hotspot:tier4 still runs cleanly, and a bit faster due to recent vmTestbase parallelism improvements.
b) jdk:tier4 has a lot of failures in headful mode, probably because the tests do not like to run in parallel, see for example #5533. It would take a while to resolve for all GUI tests. If we are in agreement that current tier4 definition is good, maybe it would be proper to integrate this PR, and then make tier4 clean for headful mode?

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 17, 2021

All right, I am convinced that current patch is as good as it gets. GUI tests still do not run well with default parallelism, but I see no reason to block this integration before that is resolved. Either run tier4 in headless mode, or limit the parallelism.

@iignatev, @mrserb, @dholmes-ora -- are you good with this?

mrserb
mrserb approved these changes Sep 17, 2021
Copy link
Member

@mrserb mrserb left a comment

It is fine to run headful and headless tests separately.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 17, 2021

@shipilev 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:

8273314: Add tier4 test groups

Reviewed-by: serb, iignatyev

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 15 new commits pushed to 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.

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

@openjdk openjdk bot added the ready label Sep 17, 2021
Copy link
Member

@iignatev iignatev left a comment

LGTM

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Sep 19, 2021

I abstain - you have your reviews.

Cheers,
David

@shipilev
Copy link
Contributor Author

@shipilev shipilev commented Sep 20, 2021

All right, there goes.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 20, 2021

Going to push as commit 1f8af52.
Since your change was applied there have been 27 commits pushed to the master branch:

  • 699865f: 8259039: Passing different version to --release flag than javac version output warning
  • f71df14: 8273187: jtools tests fail with missing markerName check
  • 6f3e40c: 8273825: TestIRMatching.java fails after JDK-8266550
  • a561eac: 8273895: compiler/ciReplay/TestVMNoCompLevel.java fails due to wrong data size with TieredStopAtLevel=2,3
  • d2388b7: 8273959: Some metaspace diagnostic switches should be develop
  • dc7f452: 8273815: move have_special_privileges to os_posix for POSIX platforms
  • 7c9868c: 8273454: C2: Transform (-a)(-b) into ab
  • bb9d142: 8273958: gtest/MetaspaceGtests executes unnecessary tests in debug builds
  • 2a2e919: 8273685: Remove jtreg tag manual=yesno for java/awt/Graphics/LCDTextAndGraphicsState.java & show test instruction
  • 8302061: 8273774: CDSPluginTest should only expect classes_nocoops.jsa exists on supported 64-bit platforms
  • ... and 17 more: https://git.openjdk.java.net/jdk/compare/241ac89f120f9bcfef65962aa05b51b9f847c4ce...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 20, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 20, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 20, 2021

@shipilev Pushed as commit 1f8af52.

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

@shipilev shipilev deleted the JDK-8273314-tier4 branch Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler core-libs hotspot integrated
4 participants