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

8264543: Cross modify fence optimization for x86 #3334

Closed
wants to merge 742 commits into from

Conversation

xbzhang99
Copy link
Contributor

@xbzhang99 xbzhang99 commented Apr 2, 2021

Intel introduced a new instruction “serialize” which ensures that all modifications to flags, registers, and memory by previous instructions are completed and all buffered writes are drained to memory before the next instruction is fetched and executed. It is a serializing instruction and can be used to implement cross modify fence (OrderAccess::cross_modify_fence_impl) more efficiently than using “cpuid” on supported 32-bit and 64-bit x86 platforms.

The availability of the SERIALIZE instruction is indicated by the presence of the CPUID feature flag SERIALIZE, bit 14 of the EDX register in sub-leaf CPUID:7H.0H.

https://software.intel.com/content/www/us/en/develop/download/intel-architecture-instruction-set-extensions-programming-reference.html


Progress

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

Error

 ⚠️ Executable files are not allowed (file: src/hotspot/os_cpu/windows_x86/orderAccess_windows_x86.hpp)

Issue

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 3334

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 2, 2021

👋 Welcome back xbzhang99! 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 added the rfr Pull request is ready for review label Apr 2, 2021
@openjdk
Copy link

openjdk bot commented Apr 2, 2021

@xbzhang99 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 hotspot-dev@openjdk.org label Apr 2, 2021
@mlbridge
Copy link

mlbridge bot commented Apr 2, 2021

Webrevs

@xbzhang99 xbzhang99 changed the title 8264543: Using Intel serialize instruction to replace cpuid in Cross modify fence, on supported platforms 8264543: Cross modify fence optimization for x86 Apr 5, 2021
@gerard-ziemski
Copy link

If this is an optimization, do you have any numbers that show an improvement with this change?

@xbzhang99
Copy link
Contributor Author

Hi Gerard,
Although we don't have benchmark data yet, serialize is the recommended alternative vs. using CPUID. Also, in the virtual machine environment, serialize provides full/fast serialization without causing VMEXIT

@xbzhang99
Copy link
Contributor Author

@gerard-ziemski Hi, could you review the patch? thanks.

@xbzhang99
Copy link
Contributor Author

Could I please get someone to review this PR?

@gerard-ziemski
Copy link

@gerard-ziemski Hi, could you review the patch? thanks.

Sorry xbzhang99, I do not know enough x86 assembly to actually review it.

@kimbarrett
Copy link

Just a drive-by comment, not a review.

This change is (necessarily) kind of messy because of the needed low-level
conditionalization. So it really ought to be supported by benchmark data to
argue that it's worth the effort, both now and in future maintenance.

If the performance impact of this change makes it worth doing, then it
probably ought not be limited to Linux, but should be done for all OS's that
run on x86.

@dholmes-ora
Copy link
Member

Although we don't have benchmark data yet, serialize is the recommended alternative vs. using CPUID.

I don't see any rush to make this change so there is time to gather the missing benchmark data. If Intel recommend making this change then we should definitely consider it, but we need to know that it won't adversely affect anyone first. And as Kim said this should be done for all x86 in that case.

Also, in the virtual machine environment, serialize provides full/fast serialization without causing VMEXIT

Sorry what do you mean by this?

Thanks,
David

@xbzhang99
Copy link
Contributor Author

@dholmes-ora VM exit is transitioning from a running VM to the hypervisor. The processor has to save the state of the VM. It is a costly operation. The cpuid instruction requires VM exit (https://www.intel.com/content/dam/www/public/us/en/documents/manuals/64-ia-32-architectures-software-developer-vol-3c-part-3-manual.pdf 25.1.2)

@bridgekeeper
Copy link

bridgekeeper bot commented Jun 25, 2021

@xbzhang99 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 add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

RealCLanger and others added 14 commits June 29, 2021 21:02
…fails in Windows debug build

Reviewed-by: serb, psadhukhan, pbansal
…est.java failed with OutOfMemoryError

Reviewed-by: stuefe, iignatyev
…mtiExport::post_compiled_method_load

Reviewed-by: sspitsyn, dholmes, coleenp
…s wrong mechanism to cause evacuation failure

Reviewed-by: kbarrett, iwalulya, ayang
…in the loop during execution

Reviewed-by: vromero
Co-authored-by: Wang Huang <whuang@openjdk.org>
Co-authored-by: Ai Jiaming <aijiaming1@huawei.com>
Reviewed-by: njian, jbhateja, sviswanathan, dlong, adinn
…s take 1ms minimum

Reviewed-by: kbarrett, ayang, lkorinth
…ting handle outside HandleMark

Reviewed-by: coleenp, jvernee
zhengyu123 and others added 24 commits July 15, 2021 16:40
…rtcuts for additional launchers

Reviewed-by: asemenyuk, almatvee
…eate failed (EAGAIN)

Reviewed-by: stuefe, iklam
…ackage/HeapMonitor* tests

Reviewed-by: tschatzl, jiefu
…_start* to asserts

Reviewed-by: ayang, tschatzl
…certification/LetsEncryptCA

Reviewed-by: mbaesken
…parameter to size_t type

Reviewed-by: iklam, minqi
…st.java on Linux

Reviewed-by: mikael, pbansal
Reviewed-by: dholmes, xliu, stuefe, minqi
… does not support it

Reviewed-by: tschatzl, lkorinth
…modify fence, on supported platforms

rebase with master
@openjdk
Copy link

openjdk bot commented Jul 19, 2021

⚠️ @xbzhang99 This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@openjdk openjdk bot removed the rfr Pull request is ready for review label Jul 20, 2021
@xbzhang99 xbzhang99 closed this Jul 20, 2021
@xbzhang99
Copy link
Contributor Author

reopened new one as #4848
The current one has some unwanted commits mingled

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot hotspot-dev@openjdk.org
Development

Successfully merging this pull request may close these issues.