8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang #21670

hanklo6 · 2024-10-23T21:35:45Z

We generate each test randomly. The asmtest.out.h is currently around 100 KB. We have added the --full argument to allow users to create a complete test set.

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-8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang (Bug - P2)

Reviewers

Vladimir Kozlov (@vnkozlov - Reviewer)
Sandhya Viswanathan (@sviswa7 - Reviewer)
@abdelhak-zaaim (no known openjdk.org user name / role)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21670

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

Using diff file

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

Webrev

Link to Webrev Comment

bridgekeeper · 2024-10-23T21:36:29Z

👋 Welcome back hanklo6! 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 · 2024-10-23T21:36:37Z

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

8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang

Reviewed-by: kvn, sviswanathan

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

3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
f7a61fc: 8342931: ProblemList failing tests from JDK-8335912
25c2f48: 8338544: Dedicated Array class descriptor implementation
158b93d: 8335912: Add an operation mode to the jar command when extracting to not overwriting existing files
28d23ad: 8340177: Malformed system classes loaded by bootloader crash the JVM in product builds
... and 19 more: https://git.openjdk.org/jdk/compare/c61d2c5a3403ad5f48daad22bdfb031f42bc3dd5...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 (@vnkozlov, @sviswa7) 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 · 2024-10-23T21:37:17Z

@hanklo6 To determine the appropriate audience for reviewing this pull request, one or more labels corresponding to different subsystems will normally be applied automatically. However, no automatic labelling rule matches the changes in this pull request. In order to have an "RFR" email sent to the correct mailing list, you will need to add one or more applicable labels manually using the /label pull request command.

Applicable Labels

build
client
compiler
core-libs
graal
hotspot
hotspot-compiler
hotspot-gc
hotspot-jfr
hotspot-runtime
i18n
ide-support
javadoc
jdk
jmx
kulla
net
nio
security
serviceability
shenandoah

hanklo6 · 2024-10-23T21:43:53Z

/label hotspot-compiler

openjdk · 2024-10-23T21:44:10Z

@hanklo6
The hotspot-compiler label was successfully added.

mlbridge · 2024-10-23T21:47:34Z

Webrevs

01: Full - Incremental (e79a5a65)
00: Full (dae652eb)

vnkozlov · 2024-10-23T22:48:11Z

Please, change title of the bug in JBS in this PR to:
"Gtest added by 8339507 appears to be causing 8GB build machines to hang"

It seems "JDK-8339507" at the beginning of title confuses our tools/bots to where add PR link and may cause issues later too.

vnkozlov

Please, update instructions comment in test_assemblerx86.cpp.

What subset of instructions will be generated by default?
Did you compare compilation time between files with default and "full" sets of instructions.

vnkozlov · 2024-10-23T22:54:11Z

What if we split it into several files? Will combined compilation time be different? At least it will not consume a lot of memory and page swapping.

hanklo6 · 2024-10-23T23:40:01Z

Please, update instructions comment in test_assemblerx86.cpp.

What subset of instructions will be generated by default? Did you compare compilation time between files with default and "full" sets of instructions.

@vnkozlov The default test set includes the original map0/map1 instructions with random registers, address, and immediate. I tested on my machine (i9-13900k with 32cores), with the compilation time being 10s for the default test and 30s for the full test.

hanklo6 · 2024-10-23T23:49:47Z

What if we split it into several files? Will combined compilation time be different? At least it will not consume a lot of memory and page swapping.

But splitting into several files still results in large code size. We are trying to reduce the code size as well.

sviswa7

Looks good to me.

dholmes-ora · 2024-10-24T01:25:43Z

What if you split into separate test files (.cpp) as well so that rather than one gtest that checks everything you have several smaller ones. That will reduce compilation overhead as required and also allow full coverage, potentially running tests in parallel.

sviswa7 · 2024-10-24T02:41:54Z

What if you split into separate test files (.cpp) as well so that rather than one gtest that checks everything you have several smaller ones. That will reduce compilation overhead as required and also allow full coverage, potentially running tests in parallel.

Do we really want that? Just to put things in perspective the original file size was 45963 lines and the reduced one is now 1293 lines so we are talking about 35 such files. What Hank has done in this PR is to test each instruction with random register, address, and immediate instead of all the combinations that he had before so we still have a decent coverage. The test generator tool continues to have the ability to generate the full combination optionally.

TobiHartmann · 2024-10-24T06:22:17Z

What Hank has done in this PR is to test each instruction with random register, address, and immediate instead of all the combinations that he had before so we still have a decent coverage. The test generator tool continues to have the ability to generate the full combination optionally.

I think that is good enough and preferable.

vnkozlov

Good.

hanklo6 · 2024-10-24T15:58:30Z

/integrate

openjdk · 2024-10-24T15:59:17Z

@hanklo6
Your change (at version e79a5a6) is now ready to be sponsored by a Committer.

sviswa7 · 2024-10-24T16:01:51Z

/sponsor

openjdk · 2024-10-24T16:02:33Z

Going to push as commit 7d5eefa.
Since your change was applied there have been 30 commits pushed to the master branch:

d8c3b0f: 8342768: GTest AssemblerX86.validate_vm failed: assert(VM_Version::supports_bmi1()) failed: tzcnt instruction not supported
3c14c2b: 8341566: Add Reader.of(CharSequence)
b0ac633: 8342075: HttpClient: improve HTTP/2 flow control checks
85774b7: 8342882: RISC-V: Unify handling of jumps to runtime
2c31c8e: 8339730: Windows regression after removing ObjectMonitor Responsible
f0b130e: 8339296: Record deconstruction pattern in switch fails to compile
e96b4cf: 8342387: C2 SuperWord: refactor and improve compiler/loopopts/superword/TestDependencyOffsets.java
f7a61fc: 8342931: ProblemList failing tests from JDK-8335912
25c2f48: 8338544: Dedicated Array class descriptor implementation
158b93d: 8335912: Add an operation mode to the jar command when extracting to not overwriting existing files
... and 20 more: https://git.openjdk.org/jdk/compare/c61d2c5a3403ad5f48daad22bdfb031f42bc3dd5...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-10-24T16:02:43Z

@sviswa7 @hanklo6 Pushed as commit 7d5eefa.

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

Random generate test

dae652e

openjdk bot added the rfr Pull request is ready for review label Oct 23, 2024

openjdk bot added the hotspot-compiler hotspot-compiler-dev@openjdk.org label Oct 23, 2024

hanklo6 changed the title ~~8339507: appears to be causing 8GB build machines to hang~~ 8342862: JDK-8339507 appears to be causing 8GB build machines to hang Oct 23, 2024

vnkozlov reviewed Oct 23, 2024

View reviewed changes

add instruction

e79a5a6

hanklo6 changed the title ~~8342862: JDK-8339507 appears to be causing 8GB build machines to hang~~ 8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang Oct 23, 2024

sviswa7 approved these changes Oct 23, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 23, 2024

abdelhak-zaaim approved these changes Oct 24, 2024

View reviewed changes

vnkozlov approved these changes Oct 24, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 24, 2024

openjdk bot added the integrated Pull request has been integrated label Oct 24, 2024

openjdk bot closed this Oct 24, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review sponsor Pull request is ready to be sponsored labels Oct 24, 2024

8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang #21670

8342862: Gtest added by 8339507 appears to be causing 8GB build machines to hang #21670

Uh oh!

Conversation

hanklo6 commented Oct 23, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issue

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

hanklo6 commented Oct 23, 2024

Uh oh!

openjdk bot commented Oct 23, 2024

Uh oh!

mlbridge bot commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

vnkozlov commented Oct 23, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

vnkozlov commented Oct 23, 2024

Uh oh!

hanklo6 commented Oct 23, 2024

Uh oh!

hanklo6 commented Oct 23, 2024

Uh oh!

sviswa7 left a comment

Choose a reason for hiding this comment

Uh oh!

dholmes-ora commented Oct 24, 2024

Uh oh!

sviswa7 commented Oct 24, 2024

Uh oh!

TobiHartmann commented Oct 24, 2024

Uh oh!

vnkozlov left a comment

Choose a reason for hiding this comment

Uh oh!

hanklo6 commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

sviswa7 commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

openjdk bot commented Oct 24, 2024

Uh oh!

Uh oh!

hanklo6 commented Oct 23, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Oct 23, 2024 •

edited

Loading

mlbridge bot commented Oct 23, 2024 •

edited

Loading