jdk icon indicating copy to clipboard operation
jdk copied to clipboard

8359359: AArch64: share trampolines between static calls to the same method

Open mikabl-arm opened this issue 4 months ago • 8 comments
trafficstars

Modify the C2 compiler to share trampoline stubs between static calls that resolve to the same callee method. Since the relocation target for all static calls is initially set to the static call resolver stub, the call's target alone cannot be used to distinguish between different static method calls. Instead, trampoline stubs should be shared based on the actual callee.

The SharedTrampolineTest.java was designed to verify the sharing of trampolines among static calls. However, due to imprecise log analysis, the test currently passes even when trampolines are not shared. Additionally, comments within the test suggest ambiguity regarding whether it was intended to assess trampoline sharing for static calls or runtime calls. To address these issues and eliminate ambiguity, this patch renames and updates the existing test. Furthermore, a new test is introduced, using the existing one as a foundation, to accurately evaluate trampoline sharing for both static and runtime calls.

This has passed tier1-3 and jcstress testing on AArch64.


Progress

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

Issue

  • JDK-8359359: AArch64: share trampolines between static calls to the same method (Enhancement - P4)

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 25954

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

Using diff file

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

Using Webrev

Link to Webrev Comment

mikabl-arm avatar Jun 24 '25 15:06 mikabl-arm

:wave: Welcome back mablakatov! 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.

bridgekeeper[bot] avatar Jun 24 '25 15:06 bridgekeeper[bot]

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

openjdk[bot] avatar Jun 24 '25 15:06 openjdk[bot]

@mikabl-arm 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[bot] avatar Jun 24 '25 15:06 openjdk[bot]

Webrevs

mlbridge[bot] avatar Jun 24 '25 15:06 mlbridge[bot]

One thing that looks a little odd: why do you maintain two separate lists and two sets of trampoline stub handlers?

theRealAph avatar Jun 25 '25 09:06 theRealAph

Hi @theRealAph , thank you for taking a look.

JIC, these are hash tables with (target address; list of requests (call site offsets)) as (k;v) pairs. I've considered unifying the two hash tables. The problem here is the keys. We need a way to distinguish between runtime call keys and static call keys as we iterate through the table. We could use CodeCache::is_non_nmethod(address addr) for that but this would only work when segmented code cache is enabled as far as I understand.

We could use something like Pair<callKind, address> for keys instead and use the first to distinguish between runtime and static calls. I'd need to extend template<...> class Pair so it properly supports hashing and comparison (at least equality) for this to work.

mikabl-arm avatar Jun 25 '25 12:06 mikabl-arm

@theRealAph , I've tried the above and it seems to work as intended. Please let me know if you find that solution more suitable and I'll prepare a patch for submission.

mikabl-arm avatar Jun 25 '25 14:06 mikabl-arm

We could use something like Pair<callKind, address> for keys instead and use the first to distinguish between runtime and static calls. I'd need to extend template<...> class Pair so it properly supports hashing and comparison (at least equality) for this to work.

Sounds good.

theRealAph avatar Jun 26 '25 08:06 theRealAph

We could use something like Pair<callKind, address> for keys instead and use the first to distinguish between runtime and static calls. I'd need to extend template<...> class Pair so it properly supports hashing and comparison (at least equality) for this to work.

Sounds good.

Done. @theRealAph , could you give it another look when you've got some time?

mikabl-arm avatar Jul 02 '25 16:07 mikabl-arm

@mikabl-arm this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout 8359359
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

openjdk[bot] avatar Aug 14 '25 13:08 openjdk[bot]

@mikabl-arm 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!

bridgekeeper[bot] avatar Sep 12 '25 20:09 bridgekeeper[bot]

/touch

eastig avatar Sep 12 '25 21:09 eastig

@eastig The pull request is being re-evaluated and the inactivity timeout has been reset.

openjdk[bot] avatar Sep 12 '25 21:09 openjdk[bot]