8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup #4882

dholmes-ora · 2021-07-22T22:42:20Z

Please review what is primarily a code style cleanup with some old native code:

add spaces around operators and after commas
fixed indent and line breaks
added braces to blocks
replaced raw malloc with c_malloc (check malloc)
fixed CE macro using and exception processing in general
- added different CHECK macro for monitor enter/exit
- removed some unreachable explicit exception checking code

I staged this as multiple commits to try and make the changes more obvious at each step (and if you hide whitespace changes it simplifies quite a bit), but it may be easier to just look at the end results and see if anything jumps out as still needing changing.

Note: there was no attempt to actually change the logic of any test or rewrite them in a better way - that is out of scope for the cleanup. The only logic changes relates to the CE macro and exception/error checking.

Testing:

local running of the tests themselves
tier5 testing in our CI system (in progress).

This may be of interest to @iignatev and @lmesnik .

Thanks,
David

Progress

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

Issue

JDK-8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup

Reviewers

Thomas Stuefe (@tstuefe - Reviewer)
Igor Ignatyev (@iignatev - Reviewer)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4882

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

Using diff file

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

- step 1: spaces around operators (automated, likely incomplete)

- check return codes for monitorEnter and exit (new CHECK macro) - add missing CE - remove CE from non-JNI calls - remove unreachable explicit exception processing code - need to double-check libjnistress005 as its exception logic appeared very strange

bridgekeeper · 2021-07-22T22:43:43Z

👋 Welcome back dholmes! 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 · 2021-07-22T22:45:41Z

@dholmes-ora The following label will be automatically applied to this pull request:

hotspot-runtime

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.

mlbridge · 2021-07-22T22:53:18Z

Webrevs

tstuefe

Looks good, small remarks inside.

Cheers, Thomas

test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress001.cpp

test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress003.cpp

dholmes-ora · 2021-07-23T05:50:27Z

Thanks for looking at this @tstuefe !

tstuefe

Looks good!

openjdk · 2021-07-23T06:06:30Z

@dholmes-ora 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:

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup

Reviewed-by: stuefe, 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 83 new commits pushed to the master branch:

efa63dc: 8171382: java.time.Duration missing isPositive method
ee55361: 8265474: Dubious 'null' assignment in CompactByteArray.expand
515113d: 8269924: Shenandoah: Introduce weak/strong marking asserts
f2c359a: 8271215: Fix data races in G1PeriodicGCTask
36aefa3: 8270447: [IR Framework] Add missing compilation level restriction when using FlipC1C2 stress option
e4295cc: 8270519: Move several vector helper methods to shared header file
e627cae: 8270317: Large Allocation in CipherSuite
0dcfc42: Merge
8adf008: 8269984: [macos] JTabbedPane title looks like disabled
e90ed6c: 8271173: serviceability/jvmti/GetObjectSizeClass.java doesn't check exit code
... and 73 more: https://git.openjdk.java.net/jdk/compare/1eeb1791abc438a65fa3491d1cd14ad69ef1ff98...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.

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

dholmes-ora · 2021-07-26T05:52:25Z

Looking for a second review please.

iignatev

LGTM

dholmes-ora · 2021-07-27T02:48:05Z

Thanks for the review @iignatev !

/integrate

openjdk · 2021-07-27T02:49:45Z

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

bb508e1: 8269753: Misplaced caret in PatternSyntaxException's detail message
c3d8e92: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
eb6da88: Merge
b76a838: 8269150: UnicodeReader not translating \u005c\u005d to \]
7ddabbf: 8271175: runtime/jni/FindClassUtf8/FindClassUtf8.java doesn't have to be run in othervm
3c27f91: 8271222: two runtime/Monitor tests don't check exit code
049b2ad: 8015886: java/awt/Focus/DeiconifiedFrameLoosesFocus/DeiconifiedFrameLoosesFocus.java sometimes failed on ubuntu
fcc7d59: 8269342: CICrashAt=1 does not always catch first Java method
8785737: 8269616: serviceability/dcmd/framework/VMVersionTest.java fails with Address already in use error
3aadae2: 8271140: Fix native frame handling in vframeStream::asJavaVFrame()
... and 85 more: https://git.openjdk.java.net/jdk/compare/1eeb1791abc438a65fa3491d1cd14ad69ef1ff98...master

Your commit was automatically rebased without conflicts.

openjdk · 2021-07-27T02:50:03Z

@dholmes-ora Pushed as commit fde1831.

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

dholmes-ora added 12 commits July 21, 2021 01:19

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup

1ef8662

- step 1: spaces around operators (automated, likely incomplete)

- step 2: fixup indent and line breaks

46be50f

- step 3: use CE macros from jnihelper.h

33b4373

- step 4: replace malloc with c_malloc

b25dcde

- step 5: add missing braces

11e3be5

- fixed incorrectly added spaces

efc30f4

- step 7: add spaces after commas

c2cb1e8

- fixed incorrectly added spaces

d5c9e02

- really fixed incorrectly added spaces

4814f6d

- Fixed the exception logic to restore original

977fe58

- final misc cleanups: more missing spaces around operators

64ae394

openjdk bot added the hotspot-runtime hotspot-runtime-dev@openjdk.org label Jul 22, 2021

dholmes-ora marked this pull request as ready for review July 22, 2021 22:48

openjdk bot added the rfr Pull request is ready for review label Jul 22, 2021

Restored 'dead code' that led to compilation failure on Windows

aa3c5c8

tstuefe reviewed Jul 23, 2021

View reviewed changes

test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress001.cpp Show resolved Hide resolved

test/hotspot/jtreg/vmTestbase/nsk/stress/jni/libjnistress003.cpp Outdated Show resolved Hide resolved

@tstuefe feedback

95d28b6

tstuefe approved these changes Jul 23, 2021

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Jul 23, 2021

iignatev approved these changes Jul 26, 2021

View reviewed changes

openjdk bot closed this Jul 27, 2021

openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jul 27, 2021

dholmes-ora deleted the 8212961 branch July 30, 2021 03:48

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup #4882

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup #4882

dholmes-ora commented Jul 22, 2021 •

edited by openjdk bot

bridgekeeper bot commented Jul 22, 2021

openjdk bot commented Jul 22, 2021

mlbridge bot commented Jul 22, 2021 •

edited

tstuefe left a comment

dholmes-ora commented Jul 23, 2021

tstuefe left a comment

openjdk bot commented Jul 23, 2021 •

edited

dholmes-ora commented Jul 26, 2021

iignatev left a comment

dholmes-ora commented Jul 27, 2021

openjdk bot commented Jul 27, 2021

openjdk bot commented Jul 27, 2021

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup #4882

8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup #4882

Conversation

dholmes-ora commented Jul 22, 2021 • edited by openjdk bot

Progress

Issue

Reviewers

Reviewing

bridgekeeper bot commented Jul 22, 2021

openjdk bot commented Jul 22, 2021

mlbridge bot commented Jul 22, 2021 • edited

Webrevs

tstuefe left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jul 23, 2021

tstuefe left a comment

Choose a reason for hiding this comment

openjdk bot commented Jul 23, 2021 • edited

dholmes-ora commented Jul 26, 2021

iignatev left a comment

Choose a reason for hiding this comment

dholmes-ora commented Jul 27, 2021

openjdk bot commented Jul 27, 2021

openjdk bot commented Jul 27, 2021

dholmes-ora commented Jul 22, 2021 •

edited by openjdk bot

mlbridge bot commented Jul 22, 2021 •

edited

openjdk bot commented Jul 23, 2021 •

edited