8354284: Add more compiler test folders to tier1 runs

[marc-chevalier]

Some folders in jtreg/compiler have been reported not to be run in any tier, while tier1 was probably intended, but the tier definition was mistakenly not updated. I've checked which folders are not referenced into TEST.groups.

The unmentioned ones:

• ccp
• ciReplay
• ciTypeFlow
• compilercontrol
• debug
• oracle
• predicates
• print
• relocations
• sharedstubs
• splitif
• tiered
• whitebox

And those, that are not test folders:

• lib
• patches
• testlibraries

I'm adding ccp, ciTypeFlow, predicates, sharedstubs and splitif to tier1.

The other folders seems to have been around for very long (since at least mid-2021). It's not clear how meaningful it'd be to add them/what the intent from them was. I've rather focused on the recently(-ish) added folders, that one forgot to put in a tier when adding it.

Feel free to tell if other folders should be included (and in which tier).

Thanks,
Marc

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8354284: Add more compiler test folders to tier1 runs (Enhancement - P4)

Reviewers

• Christian Hagedorn (@chhagedorn - Reviewer)
• Vladimir Kozlov (@vnkozlov - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24817/head:pull/24817
$ git checkout pull/24817

Update a local copy of the PR:
$ git checkout pull/24817
$ git pull https://git.openjdk.org/jdk.git pull/24817/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 24817

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24817.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back mchevalier! 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]

@marc-chevalier 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:

8354284: Add more compiler test folders to tier1 runs

Reviewed-by: chagedorn, kvn

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 215 new commits pushed to the master branch:

• c94a7ae: 8354276: Strict HTTP header validation
• a5f4366: 8353565: Javac throws "inconsistent stack types at join point" exception
• c8ce61c: 8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell
• ... and 212 more: https://git.openjdk.org/jdk/compare/9eeb86d972ac4cc38d923b2b868b426bbd27a4e8...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.

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 (@chhagedorn, @vnkozlov) 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).

[openjdk]

@marc-chevalier The following label will be automatically applied to this pull request:

• hotspot

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

[marc-chevalier]

/label add hotspot-compiler

[openjdk]

@marc-chevalier
The hotspot-compiler label was successfully added.

[mlbridge]

Webrevs

• 01: Full - Incremental (3232e5b8)
• 00: Full (7918a832)

[lmesnik]

The tier3_compiler includes all tests that are not in tier1 and tier2. The goal is to keep tier1 minimal. Tier1 shouldn't include long tests.
Can you please confirm that newly added test groups don't have tests with custom timeouts and overall time hasn't increased significantly.

[marc-chevalier]

Before:
duration: 1h 06m 20s; machine time: 22h 21m 08s
After:
duration: 59m 36s; machine time: 21h 27m 03s

It seems not to increase by more than the existing fluctuations.

Also, I think it's not quite as simple as this. It's rather a tradeoff between tier1 should be minimal in duration, while being maximal in coverage. A tier1 that doesn't cover some topics is not as useful. Some of these folders should have always been in tier1: they are just a more meaningful classification than stuffing everything in a generically named folder, but the group definition was mistakenly not updated back then.

This PR is adding only 39 files. There is one with custom timeout: predicates/TestCloningWithManyDiamondsInExpression.java with a timeout of 30. To compare with some tests in c1 or c2, not excluded from tier1, with timeout up to 600.

[chhagedorn]

Looks good to me and I agree that the proposed tests should always have been in tier1 for the laid out reasons above.

It seems not to increase by more than the existing fluctuations.

Thanks for double-checking!

predicates/TestCloningWithManyDiamondsInExpression.java with a timeout of 30

A lower than standard timeout is always fine. I originally added this test which was stuck in an endless loop before the fix. I put a lower than standard timeout there because I wanted to make sure it's now finishing quickly.

[marc-chevalier]

I couldn't find where the standard timeout is specified. How much is it? And maybe, where I could have found it?

[chhagedorn]

At some point I've run into the default timeout by specifying flags that are too slow. It is set to 120s. I quickly checked the jtreg FAQ. It can also be found there (link):

[...] The default time is 120 seconds (2 minutes) but can be changed using the /timeout option for the action. [...]

[vnkozlov]

I looked through testing logs on slowest machine macosx-x64 and next test took more then 10 sec to run. Consider to remove it from tier1:

TEST: compiler/ccp/TestAndConZeroCCP.java
   build: 0.36 seconds
   compile: 0.327 seconds
   main: 17.989 seconds
   build: 0.0 seconds
   main: 19.864 seconds
   build: 0.0 seconds
   main: 12.58 seconds
 TEST RESULT: Passed. Execution successful

[marc-chevalier]

Rather than simply removing the test from tier1, can we make it faster? (spoiler: yes). There are two big slow things:

• a RepeatCompilation=300:
  

  jdk/test/hotspot/jtreg/compiler/ccp/TestAndConZeroCCP.java

  Lines 29 to 30 in 526951d

  	* @run main/othervm -XX:+UnlockDiagnosticVMOptions -XX:RepeatCompilation=300 -XX:+StressIGVN -XX:+StressCCP -Xcomp
  	* -XX:CompileOnly=java.lang.Integer::parseInt compiler.ccp.TestAndConZeroCCP compileonly

  
  we can remove this flag entirely. It's useful for debugging, but tier1 is run often, we probably don't need this flag. It is good for debugging, but I think we usually don't include such flags in tests.
• a rather big loop:
  

  jdk/test/hotspot/jtreg/compiler/ccp/TestAndConZeroCCP.java

  Lines 45 to 47 in 526951d

  	for (int i = 0; i < 10000; ++i) {
  	run();
  	}

  
  calling another rather big loop:
  

  jdk/test/hotspot/jtreg/compiler/ccp/TestAndConZeroCCP.java

  Lines 51 to 55 in 526951d

  	for (int cp = 0; cp <= 1 << 16; cp++) {
  	Arrays.binarySearch(array, cp);
  	Character.getType(cp);
  	Character.isAlphabetic(cp);
  	}

  
  that is overall 2^16 * 10^5 ~ 6.6e9 iterations. That's where most of the time is spent. But actually, the outer loop (with the 10000 bound) is not needed: I've reproduced the corresponding issue JDK-8350563 replacing the loop by a mere run() and it still reproduces, and is working after the fix. The inner loop (with the 1 << 16 bound) cannot be shortened: the bound is necessary for having the correct range in some node types that triggers the bug. We also cannot make a much bigger step (cp += 2 works, but not more), I'm not very sure why.

With these improvements, this test is doing much better! Running this test alone in testing takes only 1h18 of machine time when it used to take 2h30. The longer duration of a single result passed from 40s to 8s. I think with these changes, the test is still meaningful and is short enough for tier1. If you agree, I can push this tiny fix.

[vnkozlov]

-XX:RepeatCompilation=300

Based on run's output times this add only 2 sec. I agree to reduce number of iterations (may be 100) but not complete remove it.

replacing the loop by a mere run()

I would not do that. First compilation of run() will be OSR and we will never run it fully compiled. You need several iterations in main() to trigger and use normal compilation. But 100 iterations should be fine. This should put execution time under 1 sec.

[marc-chevalier]

I would not do that. First compilation of run() will be OSR and we will never run it fully compiled. You need several iterations in main() to trigger and use normal compilation. But 100 iterations should be fine. This should put execution time under 1 sec.

I thought so too, but actually, run() is OSR compiled at first (which doesn't reproduce) and then fully compiled (where the crash happens). I don't understand why, but I can see it happening. Of course, I can also make a hundred iterations, that is cheap enough.

[marc-chevalier]

I've pushed the suggested change. The test still passes, longest result was 12s, from 40s without fix, 8s with my more radical fix: so it's still a big improvement.

[vnkozlov]

Good.

[marc-chevalier]

/integrate

Thanks @lmesnik @vnkozlov and @chhagedorn for comments and reviews!

[openjdk]

@marc-chevalier
Your change (at version 3232e5b) is now ready to be sponsored by a Committer.

[chhagedorn]

/sponsor

[openjdk]

Going to push as commit 69d0f7a.
Since your change was applied there have been 215 commits pushed to the master branch:

• c94a7ae: 8354276: Strict HTTP header validation
• a5f4366: 8353565: Javac throws "inconsistent stack types at join point" exception
• c8ce61c: 8355371: NegativeArraySizeException in print methods in IO or System.console() in JShell
• ... and 212 more: https://git.openjdk.org/jdk/compare/9eeb86d972ac4cc38d923b2b868b426bbd27a4e8...master

Your commit was automatically rebased without conflicts.

[openjdk]

@chhagedorn @marc-chevalier Pushed as commit 69d0f7a.

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