Patch to inline os::SpinPause() for X86 on non-Windows OS

[Man Cao]

Thanks Karen for the response!
I don't have a JBS account currently. I could ask my colleagues with JBS
accounts to create an RFE for this issue, but probably I cannot directly
post comments or performance results on JBS.

I'm working on upstreaming more Google-local runtime and GC patches, so I
can become an Author, according to:
https://wiki.openjdk.java.net/display/general/JBS+Overview
So far I have just contributed one patch: JDK-8193386.

Can I just post performance results on the mailing list and someone could
copy the results when creating an RFE?

-Man


On Wed, Jul 25, 2018 at 2:56 PM Karen Kinnear <karen.kinnear at oracle.com>
wrote:

> Man,
>
> Thank you for your proposal. The runtime is the correct team.
>
> Could you please file an rfe under hotspot/runtime with the information
> below and the patch as well as any
> tests you have run and any performance results you have?
>
> That will help us track this information and find you a sponsor.
>
> thanks,
> Karen
>
> On Jul 18, 2018, at 9:53 PM, Man Cao <manc at google.com> wrote:
>
> Hello,
>
> The Java platform team at Google has maintained a local patch to inline
> os::SpinPause() since 2014. We would like to upstream this patch to
> OpenJDK. Could someone sponsor this patch?
>
> It is difficult to demonstrate performance improvement in Java benchmarks.
> It is more of a code refactoring to better utilize modern GCC. It partly
> addresses the comment about inlining SpinPause() above its declaration in
> os.hpp.
> I found an interesting discussion about PAUSE and a microbenchmark in:
>
> http://mail.openjdk.java.net/pipermail/hotspot-runtime-dev/2012-August/004352.html
> However, the microbenchmark has a large variance in our experiment, making
> it difficult to tell if there's any benefit from inlining PAUSE. Inlining
> PAUSE does seem to reduce the variance a bit.
>
> The patch is inlined and attached below:
>
> diff --git a/src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
> b/src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
> --- a/src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
> +++ b/src/hotspot/os_cpu/bsd_x86/bsd_x86_32.s
> @@ -63,15 +63,6 @@
>          popl     %eax
>          ret
>
> -        .globl  SYMBOL(SpinPause)
> -        ELF_TYPE(SpinPause, at function)
> -        .p2align 4,,15
> -SYMBOL(SpinPause):
> -        rep
> -        nop
> -        movl    $1, %eax
> -        ret
> -
>          # Support for void Copy::conjoint_bytes(void* from,
>          #                                       void* to,
>          #                                       size_t count)
> diff --git a/src/hotspot/os_cpu/bsd_x86/bsd_x86_64.s
> b/src/hotspot/os_cpu/bsd_x86/bsd_x86_64.s
> --- a/src/hotspot/os_cpu/bsd_x86/bsd_x86_64.s
> +++ b/src/hotspot/os_cpu/bsd_x86/bsd_x86_64.s
> @@ -46,15 +46,6 @@
>
>   .text
>
> -        .globl SYMBOL(SpinPause)
> -        .p2align 4,,15
> -        ELF_TYPE(SpinPause, at function)
> -SYMBOL(SpinPause):
> -        rep
> -        nop
> -        movq   $1, %rax
> -        ret
> -
>          # Support for void Copy::arrayof_conjoint_bytes(void* from,
>          #                                               void* to,
>          #                                               size_t count)
> diff --git a/src/hotspot/os_cpu/linux_x86/linux_x86_32.s
> b/src/hotspot/os_cpu/linux_x86/linux_x86_32.s
> --- a/src/hotspot/os_cpu/linux_x86/linux_x86_32.s
> +++ b/src/hotspot/os_cpu/linux_x86/linux_x86_32.s
> @@ -42,15 +42,6 @@
>
>   .text
>
> -        .globl  SpinPause
> - .type   SpinPause, at function
> -        .p2align 4,,15
> -SpinPause:
> -        rep
> -        nop
> -        movl    $1, %eax
> -        ret
> -
>          # Support for void Copy::conjoint_bytes(void* from,
>          #                                       void* to,
>          #                                       size_t count)
> diff --git a/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
> b/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
> --- a/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
> +++ b/src/hotspot/os_cpu/linux_x86/linux_x86_64.s
> @@ -38,15 +38,6 @@
>
>   .text
>
> -        .globl SpinPause
> -        .align 16
> -        .type  SpinPause, at function
> -SpinPause:
> -        rep
> -        nop
> -        movq   $1, %rax
> -        ret
> -
>          # Support for void Copy::arrayof_conjoint_bytes(void* from,
>          #                                               void* to,
>          #                                               size_t count)
> diff --git a/src/hotspot/os_cpu/solaris_x86/solaris_x86_64.s
> b/src/hotspot/os_cpu/solaris_x86/solaris_x86_64.s
> --- a/src/hotspot/os_cpu/solaris_x86/solaris_x86_64.s
> +++ b/src/hotspot/os_cpu/solaris_x86/solaris_x86_64.s
> @@ -51,15 +51,6 @@
>          movq %fs:0x0,%rax
>          ret
>
> -        .globl  SpinPause
> -        .align  16
> -SpinPause:
> -        rep
> -        nop
> -        movq    $1, %rax
> -        ret
> -
> -
>          / Support for void Copy::arrayof_conjoint_bytes(void* from,
>          /                                               void* to,
>          /                                               size_t count)
> diff --git a/src/hotspot/share/runtime/os.hpp
> b/src/hotspot/share/runtime/os.hpp
> --- a/src/hotspot/share/runtime/os.hpp
> +++ b/src/hotspot/share/runtime/os.hpp
> @@ -1031,6 +1031,13 @@
>  // of the global SpinPause() with C linkage.
>  // It'd also be eligible for inlining on many platforms.
>
> +#if defined(X86) && !defined(_WINDOWS)
> +extern "C" int inline SpinPause() {
> +  __asm__ __volatile__ ("pause");
> +  return 1;
> +}
> +#else
>  extern "C" int SpinPause();
> +#endif
>
>  #endif // SHARE_VM_RUNTIME_OS_HPP
>
> -Man
> <inline_spinpause.patch>
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20180725/02dfbb45/attachment.htm>