Skip to content

8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview#2236

Open
Arraying wants to merge 4 commits intoopenjdk:lworldfrom
Arraying:JDK-8380282
Open

8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview#2236
Arraying wants to merge 4 commits intoopenjdk:lworldfrom
Arraying:JDK-8380282

Conversation

@Arraying
Copy link
Copy Markdown
Member

@Arraying Arraying commented Mar 17, 2026

Hi,

The following tests fail with --enable-preview and have been bulk problemlisted:

resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java
serviceability/sa/CDSJMapClstats.java
serviceability/sa/ClhsdbDumpheap.java
serviceability/sa/ClhsdbJhisto.java
serviceability/sa/ClhsdbJstackWithConcurrentLock.java
serviceability/sa/TestJmapCore.java
serviceability/sa/TestJmapCoreMetaspace.java
sun/tools/jhsdb/HeapDumpTest.java
sun/tools/jhsdb/HeapDumpTestWithActiveProcess.java
sun/tools/jhsdb/JShellHeapDumpTest.java

Testing on Linux (x64, AArch64), macOS (x64, AArch64), Windows x64:

  • tier 1 sanity test
  • :hotspot_serviceability (most of the SA tests), with and without preview
  • :hotspot_resourcehogs (some SA tests), with and without preview
  • :svc_tools (some jhsdb tests), with and without preview

Progress

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

Issue

  • JDK-8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview (Bug - P4)

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 2236

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/valhalla/pull/2236.diff

Using Webrev

Link to Webrev Comment

@bridgekeeper
Copy link
Copy Markdown

bridgekeeper bot commented Mar 17, 2026

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

@openjdk
Copy link
Copy Markdown

openjdk bot commented Mar 17, 2026

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

8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview

Reviewed-by: coleenp, cjplummer

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 125 new commits pushed to the lworld 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 lworld branch, type /integrate in a new comment.

@openjdk openjdk bot changed the title 8380282 8380282: [lworld] Skip SA tests when running with --enable-preview Mar 17, 2026
@Arraying Arraying marked this pull request as ready for review March 17, 2026 16:25
@openjdk openjdk bot added the rfr Pull request is ready for review label Mar 17, 2026
@mlbridge
Copy link
Copy Markdown

mlbridge bot commented Mar 17, 2026

Webrevs

@plummercj
Copy link
Copy Markdown
Contributor

plummercj commented Mar 17, 2026

What is the motivation for disabling all the SA tests instead of problem listing just the failing tests? How many SA tests are failing with --enable-preview?

@coleenp
Copy link
Copy Markdown
Contributor

coleenp commented Mar 18, 2026

I would 100% much rather we programmatically disable them than problem list a list of tests. The ProblemList list is noise that I thought I cleaned out once but it keeps changing. This is a lot better. Also if this fixes this test to not run, it should be removed from the problem list.

ProblemList-enable-preview.txt:serviceability/sa/TestHeapDumpForInvokeDynamic.java                                              8377387 generic-all

@alexmenkov
Copy link
Copy Markdown

I agree with Chris.
Although currently SA does not support value objects, the support have to be implemented before value classes out of preview (I believe it happen earlier than we deprecate SA).
As most of the SA test pass, it would be better to keep them running to catch new issues. To me ProblemList-enable-preview is a good place to see what needs to be done.

@coleenp
Copy link
Copy Markdown
Contributor

coleenp commented Mar 18, 2026

I feel like this makes extra work in categorizing and triaging issues for something that is going to continue to break in different ways all the time. Maybe we could have one issue to reenable the tests and fix them, if we plan on fixing these in the future.

@plummercj
Copy link
Copy Markdown
Contributor

It is best to detect new failures as they happen, not months down the road when someone decides to disable this change and retest, and then has to figure out what changes caused the new failures.

It looks like only 8 tests out of about 90 tests are failing. Most of the failures I skimmed over already have bugs filed for them. We should get them all problem listed so we have clean results and can then readily detect new failures.

@Arraying Arraying changed the title 8380282: [lworld] Skip SA tests when running with --enable-preview 8380282 Mar 24, 2026
@openjdk openjdk bot changed the title 8380282 8380282: [lworld] Problem-list several SA/jhsdb tests when running with --enable-preview Mar 24, 2026
@Arraying
Copy link
Copy Markdown
Member Author

Thanks for the feedback, everyone. I've disabled the tests I found failing, and created JBS issues where necessary.

Copy link
Copy Markdown
Contributor

@coleenp coleenp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's only two bugs so not a big effort. Looks good.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Mar 24, 2026
@Arraying
Copy link
Copy Markdown
Member Author

@plummercj @alexmenkov could I get a quick svc review/look before I integrate? TIA.

Comment on lines +49 to +50
serviceability/sa/TestJmapCore.java 8380779 generic-all
serviceability/sa/TestJmapCoreMetaspace.java 8380779 generic-all
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following tests should use 8379925:

serviceability/sa/TestJmapCore.java
serviceability/sa/TestJmapCoreMetaspace.java
serviceability/sa/ClhsdbDumpheap.java

The other 4 are all separate issues and they don't require -XX:-UseCompressedOops to reproduce. I was actually getting ready to do a problem list update myself (already have the diffs in place) but was just holding off until I could get bugs filed. If you want to file them all under 8379925 for now that is ok. I might change them once I file new CRs, but you should update the CR to not require -XX:-UseCompressedOops.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually the 8380769 CR is correct for TestHeapDumpForLargeArray.java. I misread it as 8380779

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you meant 8379920 (8379925 is about /jdk/sun/tools/jhsdb/ tests)
I've closed 8379920 as a dup of 8380779

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either 8379925 or 8379920 is fine. They are dups, so whichever one is kept open.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed JDK-8380851 for serviceability/sa/ClhsdbJstackWithConcurrentLock.java

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed JDK-8380852 for serviceability/sa/CDSJMapClstats.java

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I filed JDK-8380853 for the serviceability/sa/ClhsdbJhisto.java failure

@plummercj
Copy link
Copy Markdown
Contributor

@Arraying I have a fix for the following for tests, so please don't problem list them. See #2271

resourcehogs/serviceability/sa/TestHeapDumpForLargeArray.java
serviceability/sa/CDSJMapClstats.java
serviceability/sa/ClhsdbJhisto.java
serviceability/sa/ClhsdbJstackWithConcurrentLock.java

@Arraying
Copy link
Copy Markdown
Member Author

@plummercj @alexmenkov thanks for the feedback. I've updated the tests to use 8379925 and removed the ones being addressed in the PR. I'll address the comments in the JBS issues momentarily.

@Arraying Arraying closed this Mar 27, 2026
@Arraying
Copy link
Copy Markdown
Member Author

Arraying commented Mar 27, 2026

I did not mean to close the PR, but if you think it's easier to do the problemlist changes in your respective feature PRs I'm okay to abandon this.

@Arraying Arraying reopened this Mar 27, 2026
Copy link
Copy Markdown
Contributor

@plummercj plummercj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready Pull request is ready to be integrated rfr Pull request is ready for review

Development

Successfully merging this pull request may close these issues.

4 participants