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

8287373: remove unnecessary paddings in generated code #8453

Closed
wants to merge 8 commits into from

Conversation

bulasevich
Copy link
Contributor

@bulasevich bulasevich commented Apr 28, 2022

The goal is to remove unnecessary paddings in generated code. The alignment of the [Stub Code] section is determined by the same value as the alignment of the [Entry Point] section: the CodeEntryAlignment parameter with default values 64B on AARCH, and 32B on AMD.

Large entry alignment values are questionable for entry section. For example, Arm Neoverse N1 Software Optimization Guide recommends to align subroutines to 32B, while static compilers uses an even smaller value of 16B. However, with this change, I suggest to apply different (and smaller) values for [Constants] and [Stub Code] section alignments. This makes overall code 2% smaller on AARCH.

The correctness of the changes is checked by jtreg. Performance tested by Renaissance and SpecJBB benchmarkds on AARCH and AMD.

Example. Dummy method disassembly on AARCH, before vs after:

[Verified Entry Point]                          |  [Verified Entry Point]
  78c63b80:   nop                               |    7437e480:   nop
  78c63b84:   sub     x9, sp, #0x20, lsl #12    |    7437e484:   sub     x9, sp, #0x20, lsl #12
  78c63b88:   str     xzr, [x9]                 |    7437e488:   str     xzr, [x9]
  78c63b8c:   sub     sp, sp, #0x20             |    7437e48c:   sub     sp, sp, #0x20
  78c63b90:   stp     x29, x30, [sp, #16]       |    7437e490:   stp     x29, x30, [sp, #16]
  78c63b94:   orr     w1, wzr, #0x10            |    7437e494:   orr     w1, wzr, #0x10
  78c63b98:   bl      78343e00                  |    7437e498:   bl      73a61980
  78c63b9c:   .inst   0x00000000 ; undefined    |    7437e49c:   .inst   0x00000000 ; undefined
  78c63ba0:   .inst   0x00000000 ; undefined    |
  78c63ba4:   .inst   0x00000000 ; undefined    |
  78c63ba8:   .inst   0x00000000 ; undefined    |
  78c63bac:   .inst   0x00000000 ; undefined    |
  78c63bb0:   .inst   0x00000000 ; undefined    |
  78c63bb4:   .inst   0x00000000 ; undefined    |
  78c63bb8:   .inst   0x00000000 ; undefined    |
  78c63bbc:   .inst   0x00000000 ; undefined    |
[Stub Code]                                     |  [Stub Code]
  78c63bc0:   ldr     x8, 78c63bc8              |    7437e4a0:   ldr     x8, 7437e4a8
  78c63bc4:   br      x8                        |    7437e4a4:   br      x8
  78c63bc8:   .inst   0x78343e00 ; undefined    |    7437e4a8:   .inst   0x73a61980 ; undefined
  78c63bcc:   .inst    ; undefined              |    7437e4ac:   .inst    ; undefined
[Exception Handler]                             |  [Exception Handler]
  78c63bd0:   b       783ee080                  |    7437e4b0:   b       73b0c100
[Deopt Handler Code]                            |  [Deopt Handler Code]
  78c63bd4:   adr     x30, 78c63bd4             |    7437e4b4:   adr     x30, 7437e4b4
  78c63bd8:   b       78343ac0                  |    7437e4b8:   b       73a61620
  78c63bdc:   .inst   0x00000000 ; undefined    |    7437e4bc:   .inst   0x00000000 ; undefined

Progress

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

Issue

  • JDK-8287373: remove unnecessary paddings in generated code

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 8453

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/8453.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Apr 28, 2022

👋 Welcome back bulasevich! 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 Apr 28, 2022

@bulasevich The following label will be automatically applied to this pull request:

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Apr 28, 2022
@bulasevich bulasevich force-pushed the InteriorEntryAlignment branch 3 times, most recently from c802b1e to a631841 Compare Apr 30, 2022
@@ -37,7 +37,7 @@ define_pd_global(bool, TrapBasedNullChecks, false);
define_pd_global(bool, UncommonNullCast, true); // Uncommon-trap NULLs past to check cast

define_pd_global(uintx, CodeCacheSegmentSize, 64 COMPILER1_AND_COMPILER2_PRESENT(+64)); // Tiered compilation has large code-entry alignment.
define_pd_global(intx, CodeEntryAlignment, 64);
Copy link
Contributor

@eastig eastig May 5, 2022

Choose a reason for hiding this comment

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

This change looks reasonable to me. I found the following in the N1 Opt Guide:

Consider aligning subroutine entry points and branch targets to 32B boundaries, within the bounds of the code-density requirements of the program. This will ensure that the subsequent fetch can maximize bandwidth following the taken branch by bringing in all useful instructions

Copy link
Contributor Author

@bulasevich bulasevich Jun 9, 2022

Choose a reason for hiding this comment

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

I have rolled back CodeEntryAlignment 64->32 change as it shows different results on ARM platforms. I think it should be done in a separate PR. It can be tuned in vm_version after CPU architecture check.

src/hotspot/share/asm/codeBuffer.hpp Show resolved Hide resolved
@bulasevich bulasevich force-pushed the InteriorEntryAlignment branch 2 times, most recently from ea4483c to 7237201 Compare May 25, 2022
@bulasevich bulasevich changed the title remove unnecessary paddings within the generated code 8287373: remove unnecessary paddings within the generated code May 26, 2022
@bulasevich bulasevich changed the title 8287373: remove unnecessary paddings within the generated code 8287373: remove unnecessary paddings in generated code Jun 1, 2022
…nerate_handler_blob asks a 1KB buffer,

actually CodeBuffer::initialize reserves the given size with some extra bytes;
with the new formula (code_size + align + slop * SECT_LIMIT) the extra bytes number is reduced, causing the handler_blob generator buffer to overflow.
Solution: double the code_size estimate for handler_blob generator buffer (the temporary buffer size is not really important)
@bulasevich bulasevich marked this pull request as ready for review Jun 7, 2022
@openjdk openjdk bot added the rfr label Jun 7, 2022
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jun 7, 2022

Webrevs

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Why you changed tested alignment to 8 for JVMCI tests?

@@ -2632,7 +2632,7 @@ SafepointBlob* SharedRuntime::generate_handler_blob(address call_ptr, int poll_t

// allocate space for the code
// setup code generation tools
CodeBuffer buffer("handler_blob", 1024, 512);
CodeBuffer buffer("handler_blob", 2048, 1024);
Copy link
Contributor

@vnkozlov vnkozlov Jun 7, 2022

Choose a reason for hiding this comment

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

Why you need to double the size?

Copy link
Contributor Author

@bulasevich bulasevich Jun 7, 2022

Choose a reason for hiding this comment

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

I did it to fix the CodeBuffer overflow assert on x86 build. SharedRuntime::generate_handler_blob asks a 1KB buffer -
actually CodeBuffer::initialize reserves the given size with some extra bytes; with the new formula (code_size + align + slop * SECT_LIMIT) the extra bytes number is reduced, causing the handler_blob generator buffer to overflow. My solution is to double the code_size estimate for handler_blob generator buffer. The same 2048/1024 numbers we can see on other platforms - aarch, pcc, amd, riscv.

Copy link
Contributor

@vnkozlov vnkozlov Jun 7, 2022

Choose a reason for hiding this comment

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

okay

int total_size = align_up(_constants_size, CodeSection::alignment(CodeBuffer::SECT_INSTS)) +
align_up(_code_size, CodeSection::alignment(CodeBuffer::SECT_STUBS)) +
stubs_size;
Copy link
Contributor

@vnkozlov vnkozlov Jun 7, 2022

Choose a reason for hiding this comment

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

At first look it is messed up. I understand that you are trying to take into account space between sections. It assumed the order of sections.
Add comment.

Copy link
Contributor Author

@bulasevich bulasevich Jun 7, 2022

Choose a reason for hiding this comment

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

Ok. Thank you

src/hotspot/share/asm/codeBuffer.cpp Outdated Show resolved Hide resolved
@bulasevich
Copy link
Contributor Author

@bulasevich bulasevich commented Jun 7, 2022

Why you changed tested alignment to 8 for JVMCI tests?

HotSpotCompiledCode.dataSectionAlignment value was changed to 8 to correspond the new CodeBuffer.constants code section alignment.

The jtreg tests issue was repoduced as "invalid data section alignment" JVMCI error in CodeInstaller::initialize_fields

@@ -85,9 +85,11 @@ public boolean equals(Object obj) {
}
}

static final int validDataSectionAlignment = 8;
Copy link
Contributor

@vnkozlov vnkozlov Jun 7, 2022

Choose a reason for hiding this comment

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

Add comment to explain where 8 comes from.

Copy link
Contributor Author

@bulasevich bulasevich Jun 9, 2022

Choose a reason for hiding this comment

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

fixed. thanks!

@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented Jun 9, 2022

@bulasevich Please do not rebase or force-push to an active PR as it invalidates existing review comments. All changes will be squashed into a single commit automatically when integrating. See OpenJDK Developers’ Guide for more information.

eastig
eastig approved these changes Jun 9, 2022
Copy link
Contributor

@eastig eastig left a comment

lgtm

@@ -85,9 +85,12 @@ public boolean equals(Object obj) {
}
}

// DataSectionAlignment value matches the alignment of the CodeBuffer::SECT_CONSTS code section
Copy link
Contributor

@vnkozlov vnkozlov Jun 9, 2022

Choose a reason for hiding this comment

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

the alignment sizeof(jdouble) of the

Copy link
Contributor

@vnkozlov vnkozlov left a comment

Looks good. I will test it before approval.

Copy link
Contributor

@vnkozlov vnkozlov left a comment

My tier1-4 testing passed.

@openjdk
Copy link

@openjdk openjdk bot commented Jun 10, 2022

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

8287373: remove unnecessary paddings in generated code

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

  • 900d967: 8287924: Avoid redundant HashMap.containsKey call in EnvHelp.mapToHashtable
  • d482d7f: 8286160: (fs) Files.exists returns unexpected results with C:\pagefile.sys because it's not readable
  • edff51e: 8284858: Start of release updates for JDK 20
  • 2671443: 8286171: HttpClient/2 : Expect:100-Continue blocks indefinitely when response is not 100
  • 59b0de6: 8288048: Build failure with GCC 6 after JDK-8286562
  • db4405d: 8288078: linux-aarch64-optimized build fails in Tier5 after JDK-8287567
  • 7e948f7: 8287903: Reduce runtime of java.math microbenchmarks
  • 3fa9984: 8287854: Dangling reference in ClassVerifier::verify_class
  • 4285e3d: 8288023: AArch64: disable PAC-RET when preview is enabled
  • 560e292: 8287738: [PPC64] jdk/incubator/vector/*VectorTests failing
  • ... and 156 more: https://git.openjdk.org/jdk/compare/784697138182f104c30f8365b01f67be4d28083c...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 Jun 10, 2022
@bulasevich
Copy link
Contributor Author

@bulasevich bulasevich commented Jun 10, 2022

My tier1-4 testing passed.

Thank you!

@bulasevich
Copy link
Contributor Author

@bulasevich bulasevich commented Jun 15, 2022

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2022

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

  • 2471f8f: 8287647: VM debug support: find node by pattern in name or dump
  • 33f34d5: 8288207: Enhance MalformedURLException in Uri.parseCompat
  • 444a0d9: 8284977: MetricsTesterCgroupV2.getLongValueEntryFromFile fails when named value doesn't exist
  • 08400f1: 8287349: AArch64: Merge LDR instructions to improve C1 OSR performance
  • fe80721: 8287917: System.loadLibrary does not work on Big Sur if JDK is built with macOS SDK 10.15 and earlier
  • bbaeacb: 8265586: [windows] last button is not shown in AWT Frame with BorderLayout and MenuBar set.
  • 0f58097: 8288134: Super class names don't have envelopes
  • fb29770: 8287186: JDK modules participating in preview
  • 0530f4e: 8288094: cleanup old _MSC_VER handling
  • 1a65332: 8287906: Rewrite of GitHub Actions (GHA) sanity tests
  • ... and 212 more: https://git.openjdk.org/jdk/compare/784697138182f104c30f8365b01f67be4d28083c...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated label Jun 15, 2022
@openjdk openjdk bot closed this Jun 15, 2022
@openjdk openjdk bot removed ready rfr labels Jun 15, 2022
@openjdk
Copy link

@openjdk openjdk bot commented Jun 15, 2022

@bulasevich Pushed as commit 68b2057.

💡 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
Labels
hotspot integrated
3 participants