7903671: jcstress: Update buffer tests for JDK-8318966

[judovana]

part 1: Detecting jdk of target VM and exuding illegible tests

This is prequel to full fixing of CODETOOLS-7903671. This PR detects target JDK version and then disables illegible tests.
part 2 will be done in templates of affected tests and logic around generating the classes from them. In meantime this should server.

The jdk detection will be most likely used anyway. WDYT? If we agree on similar approach, I will create a new bug for this PR only.

---

Progress

• Change must not contain extraneous whitespace
• Change must be properly reviewed (1 review required, with at least 1 Committer)

Issue

• CODETOOLS-7903671: jcstress: Update buffer tests for JDK-8318966 (Bug - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 159

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jcstress/pull/159.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

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

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full (13537f78)
• 00: Full (00e60dee)

[judovana]

I made a much more deeper look, and consulted with Aph and Thomas, and it seems that for jdk 23 and up, the correct replacement are direct buffers - which - feel free to correct me - are well covered in jcstress,

Thus the buffer tests should be conditionally excluded, as here, or removed.

[bridgekeeper]

@judovana 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[judovana]

I still have faith!

[bridgekeeper]

@judovana 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[judovana]

loosing faith:(

[judovana]

../live..

[shipilev]

Finally have time to look at this.

No, we are not going to hard-code test names in generic harness code. The common way to "skip" the test that are incompatible with some JDKs is to record them as "soft errors", which make them skipped. This is what CODETOOLS-7903695 did, and it is supposed to be enough.

I think status quo with soft-failing alignment 8 tests, while inconvenient, is what we want. JDK 21 should be testing the heap buffers with strong alignment constraints. If anything, we should be looking whether to update the heap tests to also include alignment 1.

[judovana]

hi! Thanx a lot for eyball!

If this should be the way, that I would rather run it with alignment 1 then stay with error. The error seem redundant here. JCStress is doing a lot of checks, and the final runs are based on this. Checking the java version or ByteBuffer impl seems more reasonable.. However as far as I can tell, the alignment 1 is not triggering the parts it was with 8 and thus not testing what it should.

Sorry to say that, but I have doubts that the ByteBuffer tests are handled as soft errors. If that would be the case I think I would not bother with PR. "api mismatch" would be good enough. But as you know much better, I will try and elaborate. If they are not returning api mismatch, then I will elaborate in this PR so they start to return api mismatch.

[shipilev]

To emphasize, my point is that alignment(8) is a behavior that someone likely depends on in earlier JDKs. We need to continue testing that. So alignment(8) should stay until the last JDK that supports it, dies. Yes, it would make sense to make sure that alignment(8) return API mismatch when run with incompatible JDK. I thought it does. If it does not, that would definitely need improvement.

[judovana]

Ack. Wdyt about keeping the java version check, and on newer jdks set it to 1 then?

[shipilev]

Ack. Wdyt about keeping the java version check, and on newer jdks set it to 1 then?

Maybe, but likely not. I could see how JDK-specific test behavior would be confusing: you run seemingly the same test on different JDKs, but you actually run different tests? Bisecting for regressions would get extra fun.

So, I would like all JDK versions attempt to run all the tests, and gracefully fail out some of them. So older JDKs would test both alignment(1) and alignment(8). Newer JDKs only run alignment(8) tests.

[judovana]

You mean " So older JDKs would test both alignment(1) and alignment(8). Newer JDKs only run alignment(1) tests." right? Well that make sense to me.

[shipilev]

Yes. We strive to test everything that users would likely end up using.

[openjdk]

@judovana Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. See OpenJDK Developers’ Guide for more information.

[judovana]

Finally have time to look at this.

No, we are not going to hard-code test names in generic harness code. The common way to "skip" the test that are incompatible with some JDKs is to record them as "soft errors", which make them skipped. This is what CODETOOLS-7903695 did, and it is supposed to be enough.

Ok. you are right. They are just api-mishmash, so soft errors, so ignored. Not sure when I run it last so it was so intrusive it led me to create and elaborate on CODETOOLS-7903671. That sounds like this PR is indeed no go.

I'm tempted to add the alignment(1) variants so jdk24+ do not suffer the lack of coverage. What is your advice here?

[judovana]

faith restored

[bridgekeeper]

@judovana 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!

[judovana]

Is there hope?

[bridgekeeper]

@judovana 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!

[judovana]

Loosing the faith...

[bridgekeeper]

@judovana 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!

[judovana]

Loosing the faith...

[bridgekeeper]

@judovana 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!

[judovana]

keep_alive

[judovana]

Finally have time to look at this.
No, we are not going to hard-code test names in generic harness code. The common way to "skip" the test that are incompatible with some JDKs is to record them as "soft errors", which make them skipped. This is what CODETOOLS-7903695 did, and it is supposed to be enough.

Ok. you are right. They are just api-mishmash, so soft errors, so ignored. Not sure when I run it last so it was so intrusive it led me to create and elaborate on CODETOOLS-7903671. That sounds like this PR is indeed no go.

They ar eno longer apimishmases:

  o.o.j.t.accessAtomic.varHandles.byteArray.big.acqrel.CharTest 		ERROR 			Error while running the test
  o.o.j.t.accessAtomic.varHandles.byteArray.big.acqrel.DoubleTest 		ERROR 			Error while running the test
....
split
    actor1: Interpreter
    actor2: Interpreter
    actor1: package group 0, core group 0
    actor2: package group 1, core group 1
[-Djcstress.console.printIntervalMs=3600000}]
	ERROR
Check test failed
java.lang.ExceptionInInitializerError
	at org.openjdk.jcstress.tests.accessAtomic.varHandles.byteArray.big.acqrel.CharTest_jcstress.jcstress_sanityCheck_API(CharTest_jcstress.java:80)
	at org.openjdk.jcstress.tests.accessAtomic.varHandles.byteArray.big.acqrel.CharTest_jcstress.sanityCheck(CharTest_jcstress.java:29)
	at org.openjdk.jcstress.infra.runners.Runner.run(Runner.java:64)
	at org.openjdk.jcstress.ForkedMain.main(ForkedMain.java:79)
Caused by: java.lang.UnsupportedOperationException: Unit size unsupported for non-direct buffers: 8
	at java.base/java.nio.ByteBuffer.alignmentOffset(ByteBuffer.java:2295)
	at org.openjdk.jcstress.tests.accessAtomic.varHandles.byteArray.big.acqrel.CharTest.(CharTest.java:47)
	... 4 more

Not sure whats wrong here, and why I had ever seen api-mishmash.... Can not see it with jdk25

I'm tempted to add the alignment(1) variants so jdk24+ do not suffer the lack of coverage. What is your advice here?

Will go by this way now.