Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8244847: Linux/PPC: runtime/CompressedOops/CompressedClassPointers: smallHeapTest fails #1512

Closed
wants to merge 3 commits into from
Closed

8244847: Linux/PPC: runtime/CompressedOops/CompressedClassPointers: smallHeapTest fails #1512

wants to merge 3 commits into from

Conversation

@reinrich
Copy link
Contributor

@reinrich reinrich commented Nov 30, 2020

This is an XS sized patch to get a zerobased compressed class space on
Linux/PPC64 if a heap up to 2g size is configured and CDS is disabled.

On Linux 4.1.42 and higher we fail to get a zerobased CCS because just one
attempt is made to place the CCS right after the heap which will be at 4g
(ELF_ET_DYN_BASE) but there the java launcher is already mapped.

This change reuses the search already implemented for AARCH64.

                  Master without Fix                               Master with Fix

-Xmx   Narrow klass base     Compressed Class Space    Narrow klass base     Compressed Class Space
----------------------------------------------------------------------------------------------------
512m   0x00007fff00000000 !  0x00007fff00000000        0x0000000000000000    0x0000000200000000
  1g   0x00007fff14000000 !  0x00007fff14000000        0x0000000000000000    0x0000000200000000
  2g   0x00007fff30000000 !  0x00007fff30000000        0x0000000000000000    0x0000000200000000
  3g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
  4g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
  8g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
 12g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
 16g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
 20g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
 24g   0x0000000000000000    0x00000007c0000000        0x0000000000000000    0x00000007c0000000
 28g   0x0000001702000000    0x0000001702000000        0x0000001702000000    0x0000001702000000
 32g   0x0000000000000000    0x0000000080000000        0x0000000000000000    0x0000000080000000
 40g   0x0000000000000000    0x0000000080000000        0x0000000000000000    0x0000000080000000
 48g   0x0000000000000000    0x0000000080000000        0x0000000000000000    0x0000000080000000

Progress

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

Issue

  • JDK-8244847: Linux/PPC: runtime/CompressedOops/CompressedClassPointers: smallHeapTest fails

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/1512/head:pull/1512
$ git checkout pull/1512

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Nov 30, 2020

👋 Welcome back rrich! 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
Copy link

@openjdk openjdk bot commented Nov 30, 2020

@reinrich 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.

@reinrich reinrich marked this pull request as ready for review Nov 30, 2020
@openjdk openjdk bot added the rfr label Nov 30, 2020
@mlbridge
Copy link

@mlbridge mlbridge bot commented Nov 30, 2020

Webrevs

Copy link
Member

@tstuefe tstuefe left a comment

LGTM. Thanks for fixing this.

@openjdk
Copy link

@openjdk openjdk bot commented Nov 30, 2020

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

8244847: Linux/PPC: runtime/CompressedOops/CompressedClassPointers: smallHeapTest fails

Reviewed-by: stuefe, mdoerr

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

  • 1433baf: 8253751: Dependencies of automatic modules are not propagated through module layers
  • e3d0f27: 8257231: assert(!is_mcall || (call_returns[block->_pre_order] <= (uint) current_offset))
  • eaf4db6: 8257502: Builds fail with new warnings after JDK-8256254
  • 2966d0d: 8257223: C2: Optimize RegMask::is_bound
  • 3a11009: 8256830: misc tests failed with "assert(env->is_enabled(JVMTI_EVENT_OBJECT_FREE)) failed: checking"
  • c859fb0: 8249836: java/io/IOException/LastErrorString.java should have bug-id as 1st word in @ignore
  • e0de28c: 8257424: RecordingStream does not specify the recording name
  • 60f2ba9: 8257487: Include configuration name in summary
  • 021dced: 8257415: ZGC: Fix barrier_data types
  • aa2d36f: 8256807: C2: Not marking stores correctly as mismatched in string opts
  • ... and 26 more: https://git.openjdk.java.net/jdk/compare/e77aed624ec2e230cde71cd21622397cf115f1f6...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.

@openjdk openjdk bot added the ready label Nov 30, 2020
@reinrich
Copy link
Contributor Author

@reinrich reinrich commented Nov 30, 2020

LGTM. Thanks for fixing this.

Thank you, Thomas.

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for fixing it! Looks good to me, too. Was it tested on AIX?

@reinrich
Copy link
Contributor Author

@reinrich reinrich commented Dec 1, 2020

Thanks for fixing it! Looks good to me, too. Was it tested on AIX?

Yes, it was tested on AIX. I've overlooked a failure though, sorry...

The test CompressedClassPointers.largeHeapTest() fails on AIX because it manages to get a zerobased CCS which is unexpected. Actually a zerobased CCS is good but we get it accidentally because the first attempt to place it behind the heap which is located above 32g fails. On Linux/PPC this test succeeds because this first attempt to allocate a high CCS succeeds which is actually not optimal.

For now I excluded the narrow klass base test on AIX (as it was before).

As an enhancement the first attempt to reserve address space for CCS should be below 32g if the coops heap was placed above 32g. I will open a JBS item for this.

Thanks for the review. I will report back on the test results.

tstuefe
tstuefe approved these changes Dec 1, 2020
Copy link
Member

@tstuefe tstuefe left a comment

small nits; leave it up to you if you follow up on them.
LGTM otherwise.
Also, (T)rivial :)

if (testNarrowKlassBase()) {
if (testNarrowKlassBase() && !Platform.isAix()) {
// AIX: the heap cannot be placed below 32g. The first attempt to
// place the CCS after the heap fails (luckily). Finally CCS is
Copy link
Member

@tstuefe tstuefe Dec 1, 2020

Choose a reason for hiding this comment

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

small nits:
s/after/behind
s/Finally/Eventually or Subsequently

Copy link
Contributor Author

@reinrich reinrich Dec 1, 2020

Choose a reason for hiding this comment

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

Done.

// place the CCS after the heap fails (luckily). Finally CCS is
// successfully placed below 32g. So we get 0x0 as narrow klass
// base. As an enhancement the first attempt mapping the CCS should
// be made below 32g if oops are compressed but the heap is above 32g.
Copy link
Member

@tstuefe tstuefe Dec 1, 2020

Choose a reason for hiding this comment

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

I'd remove the enhancement remark since it is slightly confusing and irrelevant to the test in its current form. If you want to mention it, could you please create a JBS issue and mention it here ("see also JDK-xxxx").

Copy link
Contributor Author

@reinrich reinrich Dec 1, 2020

Choose a reason for hiding this comment

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

I removed it. I'll create the JBS item after some initial tests.

Copy link
Contributor

@TheRealMDoerr TheRealMDoerr left a comment

Thanks for fixing it!

@reinrich
Copy link
Contributor Author

@reinrich reinrich commented Dec 3, 2020

Testing succeeded. Thanks again for the reviews @tstuefe and @TheRealMDoerr.

/integrate

@openjdk openjdk bot closed this Dec 3, 2020
@openjdk openjdk bot added integrated and removed ready rfr labels Dec 3, 2020
@openjdk
Copy link

@openjdk openjdk bot commented Dec 3, 2020

@reinrich Since your change was applied there have been 76 commits pushed to the master branch:

Your commit was automatically rebased without conflicts.

Pushed as commit 4a267f1.

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

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