Navigation Menu

Skip to content
This repository has been archived by the owner on Sep 2, 2022. It is now read-only.

8067223: [TESTBUG] Rename Whitebox API package #290

Closed
wants to merge 14 commits into from

Conversation

iignatev
Copy link
Member

@iignatev iignatev commented Jul 28, 2021

Hi all,

could you please review this big tedious and trivial(-ish) patch which moves sun.hotspot.WhiteBox and related classes to jdk.test.whitebox package?

the majority of the patch is the following substitutions:

  • s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g
  • s/sun\.hotspot\.parser/jdk.test.whitebox.parser/g
  • s/sun\.hotspot\.cpuinfo/jdk.test.whitebox.cpuinfo/g
  • s/sun\.hotspot\.code/jdk.test.whitebox.code/g
  • s/sun\.hotspot\.gc/jdk.test.whitebox.gc/g
  • s/sun\.hotspot\.WhiteBox/jdk.test.whitebox.WhiteBox/g

testing: tier1-4

Thanks,
-- Igor


Progress

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

Issue

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 290

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Jul 28, 2021

👋 Welcome back iignatyev! 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 openjdk bot changed the title 8067223 8067223: [TESTBUG] Rename Whitebox API package Jul 28, 2021
@openjdk
Copy link

openjdk bot commented Jul 28, 2021

@iignatev The following labels will be automatically applied to this pull request:

  • core-libs
  • hotspot
  • security
  • serviceability
  • shenandoah

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

@openjdk openjdk bot added serviceability serviceability-dev@openjdk.java.net security security-dev@openjdk.java.net hotspot hotspot-dev@openjdk.java.net core-libs core-libs-dev@openjdk.java.net shenandoah shenandoah-dev@openjdk.java.net labels Jul 28, 2021
@iignatev iignatev marked this pull request as ready for review July 29, 2021 00:46
@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 29, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 29, 2021

Webrevs

@vnkozlov
Copy link
Contributor

I know that tests fixes could be pushed during RDP2 without approval.
But these one is very big and it is not a fix - it is enhancement.

What is the reason you want to push it into JDK 17 just few days before first Release Candidate? Instead of pushing it into JDK 18.

And I can't even review it because GutHub UI hangs on these big changes.

@dholmes-ora
Copy link
Member

I agree with @vnkozlov this too big and disruptive for 17 at this stage of the release.

I also think a better approach to this cleanup would have been to copy the WhiteBox class to the new package structure, then update the tests in chunks, then remove the old WhiteBox classes when that is complete.

Thanks,
David

@iignatev
Copy link
Member Author

@vnkozlov, @dholmes-ora,

Thank you for looking at this!

I want this to be integrated into JDK 17 b/c some "external" libraries use (used to use) WhiteBox API, e.g. jcstress[2] used WhiteBox API to deoptimize compiled methods[3], and it would be easier for maintainers of such libraries to condition package name based on JDK major version. Also, given JDK 17 is an LTS, it would be beneficial for everyone not to have big differences in test bases b/w it and the mainline.

according to JEP 3, test RFEs are allowed until the very end and don't require late enhancement approval: "Enhancements to tests and documentation during RDP 1 and RDP 2 do not require approval, as long as the relevant issues are identified with a noreg-self or noreg-doc label, as appropriate"[1]. So, process-wise, I don't see any issues w/ integrating this RFE, yet, I fully agree that due to its size, this patch can be disruptive and can cause massive failures, which is something we obviously don't want at the current stage of JDK 17.

I like David's idea about phasing this clean-up, and, due to the reasons described above, I would like to integrate the first part, copying WhiteBox classes to the new package structure and associated changes w/o updating all the tests, into JDK 17 and update the tests on the mainline (w/ backporting into jdk17u).

WDYT?

Cheers,
-- Igor

@iignatev
Copy link
Member Author

Vladimir, David,

I've (forced) pushed a smaller version of the renaming. instead of removing sun.hotspot classes, it copies them to jdk.test.whitebox (w/ s.h.parser.DiagnosticCommand being removed as it's used in WhiteBox signature and it was easier to update a few tests that use it), updates hotspot code to register native methods for both sun.hotspot.WhiteBox and jdk.test.whitebox.WhiteBox classes. To make it easier and not to introduce extra dependency, I've made it impossible to use s.h.WB w/ a security manager enabled, otherwise there would be a dependency b/w s.h.WB and j.t.w.WB$WhiteBoxPermission or there would be 2 permissions. There are no open JDK tests that are impacted by this limitation.

With minor tweaks in closed source, the patch successfully passes Oracle's tier1-4.

-- Igor

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Choose a reason for hiding this comment

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

Hi Igor,

This set of changes seems much more manageable to me.

Not sure about the new deprecation warnings for the old WB classes .. might that not itself trigger some failures? If not then I don't see how the deprecation warnings help with transitioning to the new WB class?

Thanks,
David

@openjdk
Copy link

openjdk bot commented Aug 2, 2021

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

8067223: [TESTBUG] Rename Whitebox API package

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

  • e351de3: 8271272: C2: assert(!had_error) failed: bad dominance
  • 6180cf1: 8271512: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java due to 8270326
  • a1b5b81: 8271507: ProblemList SA tests that are failing with ZGC due to JDK-8248912
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • 20d2dc1: 8271403: mark hotspot runtime/memory tests which ignore external VM flags
  • e593e3d: 8271402: mark hotspot runtime/os tests which ignore external VM flags
  • 7bf72ce: 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java
  • 6878b05: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6"

Please see this link for an up-to-date comparison between the source branch of this pull request and the master 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 master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Aug 2, 2021
@DamonFool
Copy link
Member

Shall we also update the copyright year like test/lib/sun/hotspot/cpuinfo/CPUInfo.java?
Thanks.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Choose a reason for hiding this comment

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

I agree with these revised changes for JDK 17.

@iignatev
Copy link
Member Author

iignatev commented Aug 2, 2021

Hi David,

This set of changes seems much more manageable to me.

thank you for your review, David.

Not sure about the new deprecation warnings for the old WB classes .. might that not itself trigger some failures? If not then I don't see how the deprecation warnings help with transitioning to the new WB class?

the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests.

Thanks,
-- Igor

@iignatev
Copy link
Member Author

iignatev commented Aug 2, 2021

Hi Jie,

Shall we also update the copyright year like test/lib/sun/hotspot/cpuinfo/CPUInfo.java?

we most certainly shall, just pushed the commit that updates the copyright years in the touched files.

Cheers,
-- Igor

@iignatev
Copy link
Member Author

iignatev commented Aug 2, 2021

I agree with these revised changes for JDK 17.

Thanks for your review, Vladimir.

I'll rerun my testing before integrating (just for good luck).

-- Igor

@iignatev
Copy link
Member Author

iignatev commented Aug 2, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Aug 2, 2021

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

  • f8fb571: 8271150: Remove EA from JDK 17 version string starting with Initial RC promotion on Aug 5, 2021(B34)
  • e351de3: 8271272: C2: assert(!had_error) failed: bad dominance
  • 6180cf1: 8271512: ProblemList serviceability/sa/sadebugd/DebugdConnectTest.java due to 8270326
  • a1b5b81: 8271507: ProblemList SA tests that are failing with ZGC due to JDK-8248912
  • 286d313: 8271489: (doc) Clarify Filter Factory example
  • 20d2dc1: 8271403: mark hotspot runtime/memory tests which ignore external VM flags
  • e593e3d: 8271402: mark hotspot runtime/os tests which ignore external VM flags
  • 7bf72ce: 8271412: ProblemList javax/sound/midi/Sequencer/Looping.java
  • 6878b05: 8271251: JavaThread::java_suspend() fails with "fatal error: Illegal threadstate encountered: 6"

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Aug 2, 2021
@openjdk 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 Aug 2, 2021
@openjdk
Copy link

openjdk bot commented Aug 2, 2021

@iignatev Pushed as commit ada58d1.

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

@mlbridge
Copy link

mlbridge bot commented Aug 2, 2021

Mailing list message from David Holmes on shenandoah-dev:

On 3/08/2021 2:25 am, Igor Ignatyev wrote:

On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev <iignatyev at openjdk.org> wrote:

Hi all,

could you please review this big tedious and trivial(-ish) patch which moves `sun.hotspot.WhiteBox` and related classes to `jdk.test.whitebox` package?

the majority of the patch is the following substitutions:
- `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g`
- `s/sun.hotspot.parser/jdk.test.whitebox.parser/g`
- `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g`
- `s/sun.hotspot.code/jdk.test.whitebox.code/g`
- `s/sun.hotspot.gc/jdk.test.whitebox.gc/g`
- `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g`

testing: tier1-4

Thanks,
-- Igor

Igor Ignatyev has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains 12 new commits since the last revision:

- fixed ctw build
- updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's inner class
- updated requires.VMProps
- updated TEST.ROOT
- adjusted hotspot source
- added test
- moved and adjusted WhiteBox tests (test/lib-test/sun/hotspot/whitebox)
- updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class
- removed sun/hotspot/parser/DiagnosticCommand
- deprecated sun/hotspot classes
disallowed s.h.WhiteBox w/ security manager
- ... and 2 more: https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e886

Hi David,

This set of changes seems much more manageable to me.

thank you for your review, David.

Not sure about the new deprecation warnings for the old WB classes .. might that not itself trigger some failures? If not then I don't see how the deprecation warnings help with transitioning to the new WB class?

the deprecation warnings (hopefully) will help people not to forget that they should use the new WB class in new tests.

If the test passes it is unlikely people will actually notice these in
the jtr file - and even if they see them they may just ignore them
thinking they are similar to all the security manager warnings that we
ignore.

But as long as it does no harm.

Cheers,
David

@iignatev iignatev deleted the 8067223 branch August 3, 2021 05:45
@mlbridge
Copy link

mlbridge bot commented Aug 4, 2021

Mailing list message from David Holmes on shenandoah-dev:

Skara email test - please ignore.

David

On 3/08/2021 8:00 am, David Holmes wrote:

On 3/08/2021 2:25 am, Igor Ignatyev wrote:

On Sat, 31 Jul 2021 20:42:10 GMT, Igor Ignatyev
<iignatyev at openjdk.org> wrote:

Hi all,

could you please review this big tedious and trivial(-ish) patch
which moves `sun.hotspot.WhiteBox` and related classes to
`jdk.test.whitebox` package?

the majority of the patch is the following substitutions:
? - `s~sun/hotspot/WhiteBox~jdk/test/whitebox/WhiteBox~g`
? - `s/sun.hotspot.parser/jdk.test.whitebox.parser/g`
? - `s/sun.hotspot.cpuinfo/jdk.test.whitebox.cpuinfo/g`
? - `s/sun.hotspot.code/jdk.test.whitebox.code/g`
? - `s/sun.hotspot.gc/jdk.test.whitebox.gc/g`
? - `s/sun.hotspot.WhiteBox/jdk.test.whitebox.WhiteBox/g`

testing: tier1-4

Thanks,
-- Igor

Igor Ignatyev has refreshed the contents of this pull request, and
previous commits have been removed. The incremental views will show
differences compared to the previous content of the PR. The pull
request contains 12 new commits since the last revision:

? - fixed ctw build
? - updated runtime/cds/appcds/JarBuilder to copy j.t.w.WhiteBox's
inner class
? - updated requires.VMProps
? - updated TEST.ROOT
? - adjusted hotspot source
? - added test
? - moved and adjusted WhiteBox tests
(test/lib-test/sun/hotspot/whitebox)
? - updated ClassFileInstaller to copy j.t.w.WhiteBox's inner class
? - removed sun/hotspot/parser/DiagnosticCommand
? - deprecated sun/hotspot classes
??? disallowed s.h.WhiteBox w/ security manager
? - ... and 2 more:
https://git.openjdk.java.net/jdk17/compare/8f12f2cf...237e886

Hi David,

This set of changes seems much more manageable to me.

thank you for your review, David.

Not sure about the new deprecation warnings for the old WB classes ..
might that not itself trigger some failures? If not then I don't see
how the deprecation warnings help with transitioning to the new WB
class?

the deprecation warnings (hopefully) will help people not to forget
that they should use the new WB class in new tests.

If the test passes it is unlikely people will actually notice these in
the jtr file - and even if they see them they may just ignore them
thinking they are similar to all the security manager warnings that we
ignore.

But as long as it does no harm.

Cheers,
David

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
core-libs core-libs-dev@openjdk.java.net hotspot hotspot-dev@openjdk.java.net integrated Pull request has been integrated security security-dev@openjdk.java.net serviceability serviceability-dev@openjdk.java.net shenandoah shenandoah-dev@openjdk.java.net
4 participants