Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v3]

2021-07-15 Thread David Holmes
On Thu, 15 Jul 2021 15:54:50 GMT, Jorn Vernee  wrote:

>> This patch rewrites the prologue and epilogue of panama upcalls, in order to 
>> fix the test failure from the title.
>> 
>> Previously, we did a call to potentially attach the current thread to the 
>> VM, and then afterwards did the same suspend and stack reguard checks that 
>> we do on the back-edge of a native downcall. Then, on the back edge of the 
>> upcall we did another conditional call to detach the thread.
>> 
>> The suspend and reguard checks on the front-edge are incorrect, so I've 
>> changed the 2 calls to mimic what is done by JavaCallWrapper instead (with 
>> attach and detach included), and removed the old suspend and stack reguard 
>> checks.
>> 
>> FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code. 
>> This is now written in C++. Also, MacroAssembler code was added to 
>> save/restore the result of the upcall around the call on the back-edge, 
>> which was previously missing. Since the new code allocates a handle block as 
>> well, I've added handling for those oops to frame & OptimizedUpcallBlob.
>> 
>> Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address David's review comments

Thanks for the updates.

Minor comments below.

David

src/hotspot/share/prims/universalUpcallHandler.cpp line 62:

> 60: guarantee(result == JNI_OK, "Could not attach thread for upcall. JNI 
> error code: %d", result);
> 61: *should_detach = true;
> 62: thread = Thread::current();

You could use JavaThread::current() here and avoid the later conversions.

src/hotspot/share/prims/universalUpcallHandler.cpp line 138:

> 136:   debug_only(thread->dec_java_call_counter());
> 137: 
> 138:   // Old thread-local info. has been restored. We are not back in native 
> code.

Pre-existing: I think "not" should be "now".

-

Marked as reviewed by dholmes (Reviewer).

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread David Holmes
On Thu, 15 Jul 2021 10:47:17 GMT, Jorn Vernee  wrote:

>> src/hotspot/share/prims/universalUpcallHandler.cpp line 121:
>> 
>>> 119:   }
>>> 120: 
>>> 121:   MACOS_AARCH64_ONLY(thread->enable_wx(WXExec));
>> 
>> Not clear why this is needed? JavaCallWrapper doesn't use it.
>
> I took this from JavaCallWrapper. Seems to have been added in 17 for the mach 
> aarch64 port: 
> https://github.com/openjdk/jdk17/blame/a32d2eefea12771522b942b32985df0fe50119e8/src/hotspot/share/runtime/javaCalls.cpp#L112

Sorry not sure what I was looking at when I didn't see this.

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]

2021-07-15 Thread Sandhya Viswanathan
On Thu, 15 Jul 2021 08:34:42 GMT, Jatin Bhateja  wrote:

>> Current VectorAPI Java side implementation expresses rotateLeft and 
>> rotateRight operation using following operations:-
>> 
>> vec1 = lanewise(VectorOperators.LSHL, n)
>> vec2 = lanewise(VectorOperators.LSHR, n)
>> res = lanewise(VectorOperations.OR, vec1 , vec2)
>> 
>> This patch moves above handling from Java side to C2 compiler which 
>> facilitates dismantling the rotate operation if target ISA does not support 
>> a direct rotate instruction.
>> 
>> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over 
>> long and integer type vectors. For other cases (i.e. sub-word type vectors 
>> or for targets which do not support direct rotate operations )   instruction 
>> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted.
>> 
>> Please find below the performance data for included JMH benchmark.
>> Machine:  Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz)
>> 
>> 
>> Benchmark | (TESTSIZE) | Shift | Baseline AVX3 (ops/ms) | Withopt  AVX3 
>> (ops/ms) | Gain % | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain %
>> -- | -- | -- | -- | -- | -- | -- | -- | --
>>   |   |   |   |   |   |   |   |  
>> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 17223.35 | 17094.69 | 
>> -0.75 | 17008.32 | 17488.06 | 2.82
>> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 8944.98 | 8811.34 | -1.49 
>> | 8878.17 | 9218.68 | 3.84
>> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 17195.75 | 17137.32 | 
>> -0.34 | 16789.01 | 17780.34 | 5.90
>> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 9052.67 | 8838.60 | -2.36 
>> | 8814.62 | 9206.01 | 4.44
>> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 17100.19 | 16950.64 | 
>> -0.87 | 16827.73 | 17720.37 | 5.30
>> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 9079.95 | 8471.26 | -6.70 
>> | .44 | 9167.68 | 3.14
>> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 21231.33 | 21513.08 | 1.33 
>> | 21824.51 | 21479.48 | -1.58
>> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 11103.62 | 11180.16 | 0.69 
>> | 11173.67 | 11529.22 | 3.18
>> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 21119.14 | 21552.04 | 
>> 2.05 | 21693.05 | 21915.37 | 1.02
>> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 11048.68 | 11094.20 | 
>> 0.41 | 11049.90 | 11439.07 | 3.52
>> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 21506.31 | 21391.41 | 
>> -0.53 | 21263.18 | 21986.29 | 3.40
>> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 11056.12 | 11232.78 | 
>> 1.60 | 10941.59 | 11397.09 | 4.16
>> RotateBenchmark.testRotateLeftB | 512.00 | 7.00 | 17976.56 | 18180.85 | 1.14 
>> | 1212.26 | 2533.34 | 108.98
>> RotateBenchmark.testRotateLeftB | 512.00 | 15.00 | 17553.70 | 18219.07 | 
>> 3.79 | 1256.73 | 2537.41 | 101.91
>> RotateBenchmark.testRotateLeftB | 512.00 | 31.00 | 17618.03 | 17738.15 | 
>> 0.68 | 1214.69 | 2533.83 | 108.60
>> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 7258.87 | 7468.88 | 2.89 | 
>> 7115.12 | 7117.26 | 0.03
>> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 3586.65 | 3950.85 | 10.15 
>> | 3532.17 | 3595.80 | 1.80
>> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 1835.07 | 1999.68 | 8.97 | 
>> 1789.90 | 1819.93 | 1.68
>> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 7273.36 | 7410.91 | 1.89 
>> | 7198.60 | 6994.79 | -2.83
>> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 3674.98 | 3926.27 | 6.84 
>> | 3549.90 | 3755.09 | 5.78
>> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 1840.94 | 1882.25 | 2.24 
>> | 1801.56 | 1872.89 | 3.96
>> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 7457.11 | 7361.48 | -1.28 
>> | 6975.33 | 7385.94 | 5.89
>> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 3570.74 | 3929.30 | 10.04 
>> | 3635.37 | 3736.67 | 2.79
>> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 1902.32 | 1960.46 | 3.06 
>> | 1812.32 | 1813.88 | 0.09
>> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 11174.24 | 12044.52 | 7.79 
>> | 11509.87 | 11273.44 | -2.05
>> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 5981.47 | 6073.70 | 1.54 | 
>> 5593.66 | 5661.93 | 1.22
>> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 2932.49 | 3069.54 | 4.67 | 
>> 2950.86 | 2892.42 | -1.98
>> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 11764.11 | 12098.63 | 
>> 2.84 | 11069.52 | 11476.93 | 3.68
>> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 5855.20 | 6080.40 | 3.85 
>> | 5919.11 | 5607.04 | -5.27
>> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 2989.05 | 3048.56 | 1.99 
>> | 2902.63 | 2821.83 | -2.78
>> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 11652.84 | 11965.40 | 
>> 2.68 | 11525.62 | 11459.83 | -0.57
>> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 5851.82 | 6164.94 | 5.35 
>> | 5882.60 | 5842.30 | -0.69
>> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 3015.99 | 3043.79 | 0.92 
>> | 2963.71 | 2947.97 | -0.53
>> RotateBenchmark.testRotateLeftI | 512.00 | 7.00 | 

Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Vladimir Kozlov
On Thu, 15 Jul 2021 12:44:23 GMT, Jorn Vernee  wrote:

>> Thanks.
>> 
>> I've been careful here to return a `Thread*` since the result is stored in 
>> `r15_thread` and I thought converting between sub and super types could 
>> potentially result in different pointers due to the way super types are laid 
>> out within a subtype. I thought it worked like this:
>> 
>> 
>> Subclass
>> +---
>> | {Subclass vtable pointer}
>> | +--- (base class Super)
>> | | {Super vtable pointer}
>> | +---
>> +---
>> 
>> 
>> So, I thought plainly using a `JavaThread*` in generated machine code where 
>> a `Thread*` was expected could cause trouble, since the pointer needs to be 
>> offset for the type conversion.
>> 
>> But now that I'm looking at some cases with compiler explorer, the pointer 
>> offset only seems to be needed when using multiple inheritance, for instance:
>> 
>> 
>> class SuperA {
>> public:
>> virtual void foo();
>> };
>> 
>> class SuperB {
>> public:
>> virtual void bar();
>> };
>> 
>> class Sub : public SuperA, public SuperB {
>> public:
>> virtual void baz();
>> };
>> 
>> 
>> Results in:
>> 
>> 
>> class Subsize(16):
>>  +---
>>  0   | +--- (base class SuperA)
>>  0   | | {vfptr}
>>  | +---
>>  8   | +--- (base class SuperB)
>>  8   | | {vfptr}
>>  | +---
>>  +---
>> 
>> Sub::$vftable@SuperA@:
>>  | _meta
>>  |  0
>>  0   | ::foo 
>>  1   | ::baz 
>> 
>> Sub::$vftable@SuperB@:
>>  | -8
>>  0   | ::bar 
>> 
>> Sub::baz this adjustor: 0
>> 
>> 
>> (https://godbolt.org/z/1665fWzff)
>> 
>> It seems that the sub type just reuses the vtable pointer of the first super 
>> type (probably to avoid having to do this pointer offsetting). Though, 
>> converting between `SuperB*` and `Sub*` would require offsetting the 
>> pointer. I'm still not sure this is guaranteed to work like this with all 
>> compilers though (the example is with MSVC, which has options to dump class 
>> layouts).
>> 
>> The result of `on_entry` is stored in `r15_thread`, so I guess I'm wondering 
>> if it's safe to store a `JavaThread*` instead of a `Thread*` in `r15`, and 
>> other code, which may expect `r15` to hold a `Thread*`, is guaranteed to 
>> keep working? (FWIW, after changing the return type to `JavaThread*` the 
>> tests that exercise this code still pass on Windows with MSVC, and on WSL 
>> Linux with GCC).
>
> Sorry, I sent the wrong godbolt link: https://godbolt.org/z/1665fWzff

@JornVernee I have small correct to your comment. We use simple inheritance for 
Thread subclasses. Their instances have **one** vtbl pointer at the same offset 
as in base class. But this pointer will point to separate vtable for each 
subclass (and base class). The layout (sequence) of methods pointers in vtable 
is the same in base class and subclasses. But subclass specific methods 
pointers will be different.

The only issue is that you have to make sure to cast passed object pointer to 
correct subclass (or base class). Otherwise you will get incorrect vtable and 
incorrect virtual methods pointers.

R15 is used by our JIT compiled code and Interpreter code which are executed 
only in JavaThread so the pinter it contains is JavaThread*

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v3]

2021-07-15 Thread David Holmes
On Thu, 15 Jul 2021 11:02:21 GMT, Jorn Vernee  wrote:

>> How does native code reach a safepoint polling point?
>
> The stack trace looks like this:
> 
> 
> Current thread (0x02a2489f0b50):  JavaThread "Thread-4551" 
> [_thread_in_Java, id=24920, stack(0x00d9e050,0x00d9e060)]
> 
> Stack: [0x00d9e050,0x00d9e060]
> Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native 
> code)
> V  [jvm.dll+0xae6651]  os::platform_print_native_stack+0xf1  
> (os_windows_x86.cpp:235)
> V  [jvm.dll+0xda3f25]  VMError::report+0x1005  (vmError.cpp:739)
> V  [jvm.dll+0xda58ae]  VMError::report_and_die+0x7fe  (vmError.cpp:1549)
> V  [jvm.dll+0xda5fe4]  VMError::report_and_die+0x64  (vmError.cpp:1330)
> V  [jvm.dll+0x4ceca7]  report_vm_error+0xb7  (debug.cpp:282)
> V  [jvm.dll+0x6511be]  HandleArea::allocate_handle+0x3e  (handles.cpp:35)
> V  [jvm.dll+0xb8e334]  
> ThreadSafepointState::handle_polling_page_exception+0x314  (safepoint.cpp:939)
> C  0x02a035d8caa7
> 
> Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
> v  ~SafepointBlob
> J 639 c1 
> java.lang.invoke.LambdaForm$MH+0x00080101b800.invoke(Ljava/lang/Object;)V 
> java.base@18-internal (87 bytes) @ 0x02a03635f7b4 
> [0x02a03635f4a0+0x0314]
> J 620 c1 
> java.lang.invoke.LambdaForm$MH+0x000801018c00.invoke(Ljava/lang/Object;)V 
> java.base@18-internal (37 bytes) @ 0x02a036353e0c 
> [0x02a036353720+0x06ec]
> v  ~BufferBlob::mэYЫGў
> 
> 
> So I think the 'native code' is something being called by the safepoint blob, 
> but I'm not sure why it's marked with a `C` instead of `V` in the stack trace 
> (maybe just a stack unwind failure?).
> 
> FWIW, this part has already been fixed as part of: 
> https://github.com/openjdk/jdk17/pull/173 (not sure why it still shows up in 
> the diff)

Okay so we're not actually _thread_in_native it is just a chunk of VM generated 
code. Something still seems off to me about the need for the HandleMark but 
that isn't your problem.

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: RFR: 8260265: UTF-8 by Default [v4]

2021-07-15 Thread Naoto Sato
> This is an implementation for the `JEP 400: UTF-8 by Default`. The gist of 
> the changes is `Charset.defaultCharset()` returning `UTF-8` and 
> `file.encoding` system property being added in the spec, but another notable 
> modification is in `java.io.PrintStream` where it continues to use the 
> `Console` encoding as the default charset instead of `UTF-8`. Other changes 
> are mostly clarification of the term "default charset" and their links. 
> Corresponding CSR has also been drafted.
> 
> JEP 400: https://bugs.openjdk.java.net/browse/JDK-8187041
> CSR: https://bugs.openjdk.java.net/browse/JDK-8260266

Naoto Sato has updated the pull request incrementally with one additional 
commit since the last revision:

  Removed leftover `console` references in `PrintStream`

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4733/files
  - new: https://git.openjdk.java.net/jdk/pull/4733/files/182dcb6d..38b8d988

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4733=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4733=02-03

  Stats: 14 lines in 1 file changed: 0 ins; 4 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4733.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4733/head:pull/4733

PR: https://git.openjdk.java.net/jdk/pull/4733


Integrated: JDK-8269387: jpackage --add-launcher should have option to not create shortcuts for additional launchers

2021-07-15 Thread Andy Herrick
On Thu, 8 Jul 2021 19:25:33 GMT, Andy Herrick  wrote:

> JDK-8269387: jpackage --add-launcher should have option to not create 
> shortcuts for additional launchers

This pull request has now been integrated.

Changeset: 057992f2
Author:Andy Herrick 
URL:   
https://git.openjdk.java.net/jdk/commit/057992f206d48d0f6152f6fdece229e2ff56e375
Stats: 341 lines in 11 files changed: 258 ins; 13 del; 70 mod

8269387: jpackage --add-launcher should have option to not create shortcuts for 
additional launchers

Reviewed-by: asemenyuk, almatvee

-

PR: https://git.openjdk.java.net/jdk/pull/4730


Re: RFR: 8075806: divideExact is missing in java.lang.Math [v4]

2021-07-15 Thread Brian Burkhalter
> Please consider this proposal to add `divideExact()` methods for integral 
> data types to `java.lang.Math` thereby rounding out "exact" support to all 
> four basic arithmetic operations.

Brian Burkhalter has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains one new commit 
since the last revision:

  8075806: Add StrictMath analogs

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4770/files
  - new: https://git.openjdk.java.net/jdk/pull/4770/files/60adb9e5..328e4188

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4770=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4770=02-03

  Stats: 7 lines in 1 file changed: 0 ins; 5 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4770.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4770/head:pull/4770

PR: https://git.openjdk.java.net/jdk/pull/4770


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v3]

2021-07-15 Thread Jorn Vernee
On Thu, 15 Jul 2021 15:54:50 GMT, Jorn Vernee  wrote:

>> This patch rewrites the prologue and epilogue of panama upcalls, in order to 
>> fix the test failure from the title.
>> 
>> Previously, we did a call to potentially attach the current thread to the 
>> VM, and then afterwards did the same suspend and stack reguard checks that 
>> we do on the back-edge of a native downcall. Then, on the back edge of the 
>> upcall we did another conditional call to detach the thread.
>> 
>> The suspend and reguard checks on the front-edge are incorrect, so I've 
>> changed the 2 calls to mimic what is done by JavaCallWrapper instead (with 
>> attach and detach included), and removed the old suspend and stack reguard 
>> checks.
>> 
>> FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code. 
>> This is now written in C++. Also, MacroAssembler code was added to 
>> save/restore the result of the upcall around the call on the back-edge, 
>> which was previously missing. Since the new code allocates a handle block as 
>> well, I've added handling for those oops to frame & OptimizedUpcallBlob.
>> 
>> Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3
>
> Jorn Vernee has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Address David's review comments

David, I've addressed your review comments.

For now I went with changing the return type of `on_entry` to `JavaThread*`, 
assuming the potential pointer conversion is not an issue. 

I tried for a while to implement a static assert to check that `Thread*` is 
trivial convertible to `JavaThread*` as well, but couldn't find a way to do 
that at compile time, so left it for now. It looks like C++ 20 has a type trait 
to check this: 
https://en.cppreference.com/w/cpp/types/is_pointer_interconvertible_base_of (I 
think this does what we want). I thought maybe we could just mimic the 
reference implementation, but looking at for instance the MSVC STL 
implementation, this type trait is implemented as a compiler intrinsic, so I 
think we'll have to wait until we are on C++ 20 before adding such a static 
assert.

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v3]

2021-07-15 Thread Jorn Vernee
> This patch rewrites the prologue and epilogue of panama upcalls, in order to 
> fix the test failure from the title.
> 
> Previously, we did a call to potentially attach the current thread to the VM, 
> and then afterwards did the same suspend and stack reguard checks that we do 
> on the back-edge of a native downcall. Then, on the back edge of the upcall 
> we did another conditional call to detach the thread.
> 
> The suspend and reguard checks on the front-edge are incorrect, so I've 
> changed the 2 calls to mimic what is done by JavaCallWrapper instead (with 
> attach and detach included), and removed the old suspend and stack reguard 
> checks.
> 
> FWIW, this removes the JavaFrameAnchor save/restore MacroAssembler code. This 
> is now written in C++. Also, MacroAssembler code was added to save/restore 
> the result of the upcall around the call on the back-edge, which was 
> previously missing. Since the new code allocates a handle block as well, I've 
> added handling for those oops to frame & OptimizedUpcallBlob.
> 
> Testing: local running of `jdk_foreign` on Windows and Linux (WSL). Tier 1-3

Jorn Vernee has updated the pull request incrementally with one additional 
commit since the last revision:

  Address David's review comments

-

Changes:
  - all: https://git.openjdk.java.net/jdk17/pull/149/files
  - new: https://git.openjdk.java.net/jdk17/pull/149/files/211bf316..128f48db

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk17=149=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk17=149=01-02

  Stats: 20 lines in 3 files changed: 0 ins; 8 del; 12 mod
  Patch: https://git.openjdk.java.net/jdk17/pull/149.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk17 pull/149/head:pull/149

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: RFR: 8075806: divideExact is missing in java.lang.Math [v3]

2021-07-15 Thread Brian Burkhalter
> Please consider this proposal to add `divideExact()` methods for integral 
> data types to `java.lang.Math` thereby rounding out "exact" support to all 
> four basic arithmetic operations.

Brian Burkhalter has updated the pull request incrementally with one additional 
commit since the last revision:

  8075806: Add StrictMath analogs

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4770/files
  - new: https://git.openjdk.java.net/jdk/pull/4770/files/6843e666..60adb9e5

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4770=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4770=01-02

  Stats: 46 lines in 1 file changed: 46 ins; 0 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4770.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4770/head:pull/4770

PR: https://git.openjdk.java.net/jdk/pull/4770


Re: RFR: 8270547: java.util.Random contains unnecessary @SuppressWarnings("exports")

2021-07-15 Thread Brian Burkhalter
On Thu, 15 Jul 2021 12:07:42 GMT, Jan Lahoda  wrote:

> Removing unnecessary `@SuppressWarnings("exports")`. The reason for the 
> suppress warnings was, as far as I can tell, removed by 
> https://bugs.openjdk.java.net/browse/JDK-8265137.

+1

-

Marked as reviewed by bpb (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4793


Re: RFR: 8270547: java.util.Random contains unnecessary @SuppressWarnings("exports")

2021-07-15 Thread Joe Darcy
On Thu, 15 Jul 2021 12:07:42 GMT, Jan Lahoda  wrote:

> Removing unnecessary `@SuppressWarnings("exports")`. The reason for the 
> suppress warnings was, as far as I can tell, removed by 
> https://bugs.openjdk.java.net/browse/JDK-8265137.

Looks fine.

-

Marked as reviewed by darcy (Reviewer).

PR: https://git.openjdk.java.net/jdk/pull/4793


Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v4]

2021-07-15 Thread Severin Gehwolf
On Thu, 15 Jul 2021 13:09:54 GMT, Matthias Baesken  wrote:

>> Hello, please review this PR; it extend the OSContainer API in order to also 
>> support the pids controller of cgroups.
>> 
>> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct", 
>> "memory"  on some older Linux distros (SLES 12.1, RHEL 7.1) the pids 
>> controller might not be there (or not fully supported) so it was added as 
>> optional  , see the coding
>> 
>> 
>>   if (!cg_infos[PIDS_IDX]._data_complete) {
>> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
>> // keep the other controller info, pids is optional
>>   }
>
> Matthias Baesken has updated the pull request incrementally with one 
> additional commit since the last revision:
> 
>   Add hotspot tests

Thanks, I'll test it and review once again. Meanwhile, please merge the master 
branch into your `JDK-8266490` branch and push so that we get a better ruling 
of pre-integration tests.

-

PR: https://git.openjdk.java.net/jdk/pull/4518


Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v2]

2021-07-15 Thread Matthias Baesken
On Fri, 9 Jul 2021 13:53:27 GMT, Matthias Baesken  wrote:

> > OK. Please also add a test on the hotspot side. You may want to add 
> > relevant parts to `TestMisc.java`.
> 
> Thanks for the suggestion, I will look into TestMisc.java .

I added some HS testing code in the latest commit.

Best regards, Matthias

-

PR: https://git.openjdk.java.net/jdk/pull/4518


Re: RFR: JDK-8266490: Extend the OSContainer API to support the pids controller of cgroups [v4]

2021-07-15 Thread Matthias Baesken
> Hello, please review this PR; it extend the OSContainer API in order to also 
> support the pids controller of cgroups.
> 
> I noticed that unlike the other controllers "cpu", "cpuset", "cpuacct", 
> "memory"  on some older Linux distros (SLES 12.1, RHEL 7.1) the pids 
> controller might not be there (or not fully supported) so it was added as 
> optional  , see the coding
> 
> 
>   if (!cg_infos[PIDS_IDX]._data_complete) {
> log_debug(os, container)("Optional cgroup v1 pids subsystem not found");
> // keep the other controller info, pids is optional
>   }

Matthias Baesken has updated the pull request incrementally with one additional 
commit since the last revision:

  Add hotspot tests

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4518/files
  - new: https://git.openjdk.java.net/jdk/pull/4518/files/f5527143..3fe73c3c

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4518=03
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4518=02-03

  Stats: 117 lines in 3 files changed: 116 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4518.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4518/head:pull/4518

PR: https://git.openjdk.java.net/jdk/pull/4518


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread David Holmes

On 15/07/2021 10:29 pm, Jorn Vernee wrote:

On Wed, 14 Jul 2021 00:47:47 GMT, David Holmes  wrote:


Jorn Vernee has updated the pull request incrementally with one additional 
commit since the last revision:

   Assert frame is correct type in frame_data_for_frame


src/hotspot/share/prims/universalUpcallHandler.cpp line 76:


74:
75: // modelled after JavaCallWrapper::JavaCallWrapper
76: Thread* ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData* 
context) {


This should return JavaThread not Thread.


Thanks.

I've been careful here to return a `Thread*` since the result is stored in 
`r15_thread` and I thought converting between sub and super types could 
potentially result in different pointers due to the way super types are laid 
out within a subtype. I thought it worked like this:


Wow! Okay I've never seen anyone query this before. AFAIK whatever we 
store in r15_thread is required to be a correct pointer to the current 
thread object whatever its exact subtype may be. The returned pointer 
has to work correctly for virtual functions and can't be a "sliced" 
Thread instead of the real type. So as far as I know this "just works" 
and I think we'd be in big trouble if it didn't work.


But I don't deal with the under-the-covers parts of the C++ compiler.

David
-



Subclass
+---
| {Subclass vtable pointer}
| +--- (base class Super)
| | {Super vtable pointer}
| +---
+---


So, I thought plainly using a `JavaThread*` in generated machine code where a 
`Thread*` was expected could cause trouble, since the pointer needs to be 
offset for the type conversion.

But now that I'm looking at some cases with compiler explorer, the pointer 
offset only seems to be needed when using multiple inheritance, for instance:


class SuperA {
public:
 virtual void foo();
};

class SuperB {
public:
 virtual void bar();
};

class Sub : public SuperA, public SuperB {
public:
 virtual void baz();
};


Results in:


class Sub   size(16):
+---
  0 | +--- (base class SuperA)
  0 | | {vfptr}
| +---
  8 | +--- (base class SuperB)
  8 | | {vfptr}
| +---
+---

Sub::$vftable@SuperA@:
| _meta
|  0
  0 | ::foo
  1 | ::baz

Sub::$vftable@SuperB@:
| -8
  0 | ::bar

Sub::baz this adjustor: 0


(https://godbolt.org/z/rq9bT8d9d)

It seems that the sub type just reuses the vtable pointer of the first super 
type (probably to avoid having to do this pointer offsetting). Though, 
converting between `SuperB*` and `Sub*` would require offsetting the pointer. 
I'm still not sure this is guaranteed to work like this with all compilers 
though (the example is with MSVC, which has options to dump class layouts).

The result of `on_entry` is stored in `r15_thread`, so I guess I'm wondering if 
it's safe to store a `JavaThread*` instead of a `Thread*` in `r15`, and other 
code, which may expect `r15` to hold a `Thread*`, is guaranteed to keep 
working? (FWIW, after changing the return type to `JavaThread*` the tests that 
exercise this code still pass on Windows with MSVC, and on WSL Linux with GCC).

-

PR: https://git.openjdk.java.net/jdk17/pull/149



Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Jorn Vernee
On Thu, 15 Jul 2021 12:25:54 GMT, Jorn Vernee  wrote:

>> src/hotspot/share/prims/universalUpcallHandler.cpp line 76:
>> 
>>> 74: 
>>> 75: // modelled after JavaCallWrapper::JavaCallWrapper
>>> 76: Thread* 
>>> ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData* context) 
>>> {
>> 
>> This should return JavaThread not Thread.
>
> Thanks.
> 
> I've been careful here to return a `Thread*` since the result is stored in 
> `r15_thread` and I thought converting between sub and super types could 
> potentially result in different pointers due to the way super types are laid 
> out within a subtype. I thought it worked like this:
> 
> 
> Subclass
> +---
> | {Subclass vtable pointer}
> | +--- (base class Super)
> | | {Super vtable pointer}
> | +---
> +---
> 
> 
> So, I thought plainly using a `JavaThread*` in generated machine code where a 
> `Thread*` was expected could cause trouble, since the pointer needs to be 
> offset for the type conversion.
> 
> But now that I'm looking at some cases with compiler explorer, the pointer 
> offset only seems to be needed when using multiple inheritance, for instance:
> 
> 
> class SuperA {
> public:
> virtual void foo();
> };
> 
> class SuperB {
> public:
> virtual void bar();
> };
> 
> class Sub : public SuperA, public SuperB {
> public:
> virtual void baz();
> };
> 
> 
> Results in:
> 
> 
> class Sub size(16):
>   +---
>  0| +--- (base class SuperA)
>  0| | {vfptr}
>   | +---
>  8| +--- (base class SuperB)
>  8| | {vfptr}
>   | +---
>   +---
> 
> Sub::$vftable@SuperA@:
>   | _meta
>   |  0
>  0| ::foo 
>  1| ::baz 
> 
> Sub::$vftable@SuperB@:
>   | -8
>  0| ::bar 
> 
> Sub::baz this adjustor: 0
> 
> 
> (https://godbolt.org/z/1665fWzff)
> 
> It seems that the sub type just reuses the vtable pointer of the first super 
> type (probably to avoid having to do this pointer offsetting). Though, 
> converting between `SuperB*` and `Sub*` would require offsetting the pointer. 
> I'm still not sure this is guaranteed to work like this with all compilers 
> though (the example is with MSVC, which has options to dump class layouts).
> 
> The result of `on_entry` is stored in `r15_thread`, so I guess I'm wondering 
> if it's safe to store a `JavaThread*` instead of a `Thread*` in `r15`, and 
> other code, which may expect `r15` to hold a `Thread*`, is guaranteed to keep 
> working? (FWIW, after changing the return type to `JavaThread*` the tests 
> that exercise this code still pass on Windows with MSVC, and on WSL Linux 
> with GCC).

Sorry, I sent the wrong godbolt link: https://godbolt.org/z/1665fWzff

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Jorn Vernee
On Wed, 14 Jul 2021 00:58:38 GMT, David Holmes  wrote:

> The mapping to JavaCallWrapper seems reasonable but there are a few 
> differences that I'm now assuming stem from the fact upcalls start 
> _thread_in_native while JCW starts from _thread_in_vm?

The main difference stems from the fact that, since these upcalls can come from 
non-JNI native code, we can not assume that the thread is already attached to 
the VM, so we do that on the fly instead, and we detach the thread again after 
the upcall (if needed). Those are what the call to 
`maybe_attach_and_get_thread` at the start of `on_entry`, and `detach_thread` 
call at the end of `on_exit` are for.

The other thing is that there is no stack watermark check at the end of 
`on_exit`. This check is guarded by a check if the thread has any pending 
exceptions in `JavaCallWrapper`, but since a panama upcall is not allowed to 
throw any exceptions, I've changed that to an `assert` that checks that there 
are no pending exceptions at that point instead.

The last thing is that we transition directly from `_thread_in_native` to 
`_thread_in_Java`, which changes some of the thread transition code.

Other than that, I've tried to mimic what `JavaCallWrapper` does as closely as 
possible.

Is there anything else that looks different?

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Jorn Vernee
On Wed, 14 Jul 2021 00:47:47 GMT, David Holmes  wrote:

>> Jorn Vernee has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Assert frame is correct type in frame_data_for_frame
>
> src/hotspot/share/prims/universalUpcallHandler.cpp line 76:
> 
>> 74: 
>> 75: // modelled after JavaCallWrapper::JavaCallWrapper
>> 76: Thread* 
>> ProgrammableUpcallHandler::on_entry(OptimizedEntryBlob::FrameData* context) {
> 
> This should return JavaThread not Thread.

Thanks.

I've been careful here to return a `Thread*` since the result is stored in 
`r15_thread` and I thought converting between sub and super types could 
potentially result in different pointers due to the way super types are laid 
out within a subtype. I thought it worked like this:


Subclass
+---
| {Subclass vtable pointer}
| +--- (base class Super)
| | {Super vtable pointer}
| +---
+---


So, I thought plainly using a `JavaThread*` in generated machine code where a 
`Thread*` was expected could cause trouble, since the pointer needs to be 
offset for the type conversion.

But now that I'm looking at some cases with compiler explorer, the pointer 
offset only seems to be needed when using multiple inheritance, for instance:


class SuperA {
public:
virtual void foo();
};

class SuperB {
public:
virtual void bar();
};

class Sub : public SuperA, public SuperB {
public:
virtual void baz();
};


Results in:


class Sub   size(16):
+---
 0  | +--- (base class SuperA)
 0  | | {vfptr}
| +---
 8  | +--- (base class SuperB)
 8  | | {vfptr}
| +---
+---

Sub::$vftable@SuperA@:
| _meta
|  0
 0  | ::foo 
 1  | ::baz 

Sub::$vftable@SuperB@:
| -8
 0  | ::bar 

Sub::baz this adjustor: 0


(https://godbolt.org/z/rq9bT8d9d)

It seems that the sub type just reuses the vtable pointer of the first super 
type (probably to avoid having to do this pointer offsetting). Though, 
converting between `SuperB*` and `Sub*` would require offsetting the pointer. 
I'm still not sure this is guaranteed to work like this with all compilers 
though (the example is with MSVC, which has options to dump class layouts).

The result of `on_entry` is stored in `r15_thread`, so I guess I'm wondering if 
it's safe to store a `JavaThread*` instead of a `Thread*` in `r15`, and other 
code, which may expect `r15` to hold a `Thread*`, is guaranteed to keep 
working? (FWIW, after changing the return type to `JavaThread*` the tests that 
exercise this code still pass on Windows with MSVC, and on WSL Linux with GCC).

-

PR: https://git.openjdk.java.net/jdk17/pull/149


RFR: 8270547: java.util.Random contains unnecessary @SuppressWarnings("exports")

2021-07-15 Thread Jan Lahoda
Removing unnecessary `@SuppressWarnings("exports")`. The reason for the 
suppress warnings was, as far as I can tell, removed by 
https://bugs.openjdk.java.net/browse/JDK-8265137.

-

Commit messages:
 - 8270547: java.util.Random contains unnecessary @SuppressWarnings("exports")

Changes: https://git.openjdk.java.net/jdk/pull/4793/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4793=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8270547
  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4793.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4793/head:pull/4793

PR: https://git.openjdk.java.net/jdk/pull/4793


Re: RFR: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt() [v3]

2021-07-15 Thread Сергей Цыпанов
> `AbstractStringBuilder.charAt(int)` does bounds check before calling 
> `charAt()` (for non-latin Strings):
> 
> @Override
> public char charAt(int index) {
> checkIndex(index, count);
> if (isLatin1()) {
> return (char)(value[index] & 0xff);
> }
> return StringUTF16.charAt(value, index);
> }
> 
> This can be improved by removing bounds check from ASB.charAt() in favour of 
> one in String*.charAt(). This gives slight improvement:
> 
> before
> Benchmark   Mode  Cnt  Score   Error  Units
> StringBuilderCharAtBenchmark.latin  avgt   50  2,827 ± 0,024  ns/op
> StringBuilderCharAtBenchmark.utfavgt   50  2,985 ± 0,020  ns/op
> 
> after
> Benchmark   Mode  Cnt  Score   Error  Units
> StringBuilderCharAtBenchmark.latin  avgt   50  2,434 ± 0,004  ns/op
> StringBuilderCharAtBenchmark.utfavgt   50  2,631 ± 0,004  ns/op

Сергей Цыпанов has refreshed the contents of this pull request, and previous 
commits have been removed. The incremental views will show differences compared 
to the previous content of the PR. The pull request contains two new commits 
since the last revision:

 - Merge branch 'master' into 8270160
   
   # Conflicts:
   #src/java.base/share/classes/java/lang/StringLatin1.java
 - 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt()

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/4738/files
  - new: https://git.openjdk.java.net/jdk/pull/4738/files/9f30e621..b7b01ff4

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=4738=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4738=01-02

  Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4738.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4738/head:pull/4738

PR: https://git.openjdk.java.net/jdk/pull/4738


Re: RFR: 8270160: Remove redundant bounds check from AbstractStringBuilder.charAt() [v2]

2021-07-15 Thread Сергей Цыпанов
> `AbstractStringBuilder.charAt(int)` does bounds check before calling 
> `charAt()` (for non-latin Strings):
> 
> @Override
> public char charAt(int index) {
> checkIndex(index, count);
> if (isLatin1()) {
> return (char)(value[index] & 0xff);
> }
> return StringUTF16.charAt(value, index);
> }
> 
> This can be improved by removing bounds check from ASB.charAt() in favour of 
> one in String*.charAt(). This gives slight improvement:
> 
> before
> Benchmark   Mode  Cnt  Score   Error  Units
> StringBuilderCharAtBenchmark.latin  avgt   50  2,827 ± 0,024  ns/op
> StringBuilderCharAtBenchmark.utfavgt   50  2,985 ± 0,020  ns/op
> 
> after
> Benchmark   Mode  Cnt  Score   Error  Units
> StringBuilderCharAtBenchmark.latin  avgt   50  2,434 ± 0,004  ns/op
> StringBuilderCharAtBenchmark.utfavgt   50  2,631 ± 0,004  ns/op

Сергей Цыпанов has updated the pull request with a new target base due to a 
merge or a rebase. The pull request now contains two commits:

 - Merge branch 'master' into 8270160
   
   # Conflicts:
   #src/java.base/share/classes/java/lang/StringLatin1.java
 - 8270160 Remove redundant bounds check from AbstractStringBuilder.charAt()

-

Changes: https://git.openjdk.java.net/jdk/pull/4738/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4738=01
  Stats: 7 lines in 2 files changed: 1 ins; 3 del; 3 mod
  Patch: https://git.openjdk.java.net/jdk/pull/4738.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/4738/head:pull/4738

PR: https://git.openjdk.java.net/jdk/pull/4738


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Jorn Vernee
On Wed, 14 Jul 2021 00:50:44 GMT, David Holmes  wrote:

>> src/hotspot/share/runtime/safepoint.cpp line 931:
>> 
>>> 929: // See if return type is an oop.
>>> 930: bool return_oop = nm->method()->is_returning_oop();
>>> 931: HandleMark hm(self);
>> 
>> I was seeing an `assert(_handle_mark_nesting > 1) failed: memory leak: 
>> allocating handle outside HandleMark` when the existing code allocates the 
>> Handle for `return_value` in the code down below. It seems to be a simple 
>> case of a missing handle mark, so I've added it here. (looking at the stack 
>> trace in the error log, the caller frame is native code, so I don't think we 
>> can expect the caller to have a HandleMark).
>
> How does native code reach a safepoint polling point?

The stack trace looks like this:


Current thread (0x02a2489f0b50):  JavaThread "Thread-4551" 
[_thread_in_Java, id=24920, stack(0x00d9e050,0x00d9e060)]

Stack: [0x00d9e050,0x00d9e060]
Native frames: (J=compiled Java code, j=interpreted, Vv=VM code, C=native code)
V  [jvm.dll+0xae6651]  os::platform_print_native_stack+0xf1  
(os_windows_x86.cpp:235)
V  [jvm.dll+0xda3f25]  VMError::report+0x1005  (vmError.cpp:739)
V  [jvm.dll+0xda58ae]  VMError::report_and_die+0x7fe  (vmError.cpp:1549)
V  [jvm.dll+0xda5fe4]  VMError::report_and_die+0x64  (vmError.cpp:1330)
V  [jvm.dll+0x4ceca7]  report_vm_error+0xb7  (debug.cpp:282)
V  [jvm.dll+0x6511be]  HandleArea::allocate_handle+0x3e  (handles.cpp:35)
V  [jvm.dll+0xb8e334]  
ThreadSafepointState::handle_polling_page_exception+0x314  (safepoint.cpp:939)
C  0x02a035d8caa7

Java frames: (J=compiled Java code, j=interpreted, Vv=VM code)
v  ~SafepointBlob
J 639 c1 
java.lang.invoke.LambdaForm$MH+0x00080101b800.invoke(Ljava/lang/Object;)V 
java.base@18-internal (87 bytes) @ 0x02a03635f7b4 
[0x02a03635f4a0+0x0314]
J 620 c1 
java.lang.invoke.LambdaForm$MH+0x000801018c00.invoke(Ljava/lang/Object;)V 
java.base@18-internal (37 bytes) @ 0x02a036353e0c 
[0x02a036353720+0x06ec]
v  ~BufferBlob::mэYЫGў


So I think the 'native code' is something being called by the safepoint blob, 
but I'm not sure why it's marked with a `C` instead of `V` in the stack trace 
(maybe just a stack unwind failure?).

FWIW, this part has already been fixed as part of: 
https://github.com/openjdk/jdk17/pull/173 (not sure why it still shows up in 
the diff)

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: [jdk17] RFR: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC [v2]

2021-07-15 Thread Jorn Vernee
On Wed, 14 Jul 2021 00:24:38 GMT, David Holmes  wrote:

>> Jorn Vernee has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Assert frame is correct type in frame_data_for_frame
>
> src/hotspot/share/prims/universalUpcallHandler.cpp line 86:
> 
>> 84:   context->new_handles = JNIHandleBlock::allocate_block(thread);
>> 85: 
>> 86:   // After this, we are official in Java Code. This needs to be done 
>> before we change any of the thread local
> 
> typo: s/official/officially/
> (I see this was copied from Javacalls)

Thanks, I wasn't sure whether to leave the comments in or not.

> src/hotspot/share/prims/universalUpcallHandler.cpp line 114:
> 
>> 112:   thread->set_active_handles(context->new_handles); // install new 
>> handle block and reset Java frame linkage
>> 113: 
>> 114:   assert (thread->thread_state() != _thread_in_native, "cannot set 
>> native pc to NULL");
> 
> You transitioned to _thread_in_Java above so how can you possibly have 
> changed state again ?? (okay again copied from javaCalls ...)

Yeah, it seemed paranoid to me as well. I'll remove this

> src/hotspot/share/prims/universalUpcallHandler.cpp line 121:
> 
>> 119:   }
>> 120: 
>> 121:   MACOS_AARCH64_ONLY(thread->enable_wx(WXExec));
> 
> Not clear why this is needed? JavaCallWrapper doesn't use it.

I took this from JavaCallWrapper. Seems to have been added in 17 for the mach 
aarch64 port: 
https://github.com/openjdk/jdk17/blame/a32d2eefea12771522b942b32985df0fe50119e8/src/hotspot/share/runtime/javaCalls.cpp#L112

-

PR: https://git.openjdk.java.net/jdk17/pull/149


Re: RFR: 8266054: VectorAPI rotate operation optimization [v10]

2021-07-15 Thread Jatin Bhateja
> Current VectorAPI Java side implementation expresses rotateLeft and 
> rotateRight operation using following operations:-
> 
> vec1 = lanewise(VectorOperators.LSHL, n)
> vec2 = lanewise(VectorOperators.LSHR, n)
> res = lanewise(VectorOperations.OR, vec1 , vec2)
> 
> This patch moves above handling from Java side to C2 compiler which 
> facilitates dismantling the rotate operation if target ISA does not support a 
> direct rotate instruction.
> 
> AVX512 added vector rotate instructions vpro[rl][v][dq] which operate over 
> long and integer type vectors. For other cases (i.e. sub-word type vectors or 
> for targets which do not support direct rotate operations )   instruction 
> sequence comprising of vector SHIFT (LEFT/RIGHT) and vector OR is emitted.
> 
> Please find below the performance data for included JMH benchmark.
> Machine:  Cascade Lake Server (Intel(R) Xeon(R) Platinum 8280 CPU @ 2.70GHz)
> 
> 
> Benchmark | (TESTSIZE) | Shift | Baseline AVX3 (ops/ms) | Withopt  AVX3 
> (ops/ms) | Gain % | Baseline AVX2 (ops/ms) | Withopt AVX2 (ops/ms) | Gain %
> -- | -- | -- | -- | -- | -- | -- | -- | --
>   |   |   |   |   |   |   |   |  
> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 17223.35 | 17094.69 | -0.75 
> | 17008.32 | 17488.06 | 2.82
> RotateBenchmark.testRotateLeftB | 128.00 | 7.00 | 8944.98 | 8811.34 | -1.49 | 
> 8878.17 | 9218.68 | 3.84
> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 17195.75 | 17137.32 | 
> -0.34 | 16789.01 | 17780.34 | 5.90
> RotateBenchmark.testRotateLeftB | 128.00 | 15.00 | 9052.67 | 8838.60 | -2.36 
> | 8814.62 | 9206.01 | 4.44
> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 17100.19 | 16950.64 | 
> -0.87 | 16827.73 | 17720.37 | 5.30
> RotateBenchmark.testRotateLeftB | 128.00 | 31.00 | 9079.95 | 8471.26 | -6.70 
> | .44 | 9167.68 | 3.14
> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 21231.33 | 21513.08 | 1.33 
> | 21824.51 | 21479.48 | -1.58
> RotateBenchmark.testRotateLeftB | 256.00 | 7.00 | 11103.62 | 11180.16 | 0.69 
> | 11173.67 | 11529.22 | 3.18
> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 21119.14 | 21552.04 | 2.05 
> | 21693.05 | 21915.37 | 1.02
> RotateBenchmark.testRotateLeftB | 256.00 | 15.00 | 11048.68 | 11094.20 | 0.41 
> | 11049.90 | 11439.07 | 3.52
> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 21506.31 | 21391.41 | 
> -0.53 | 21263.18 | 21986.29 | 3.40
> RotateBenchmark.testRotateLeftB | 256.00 | 31.00 | 11056.12 | 11232.78 | 1.60 
> | 10941.59 | 11397.09 | 4.16
> RotateBenchmark.testRotateLeftB | 512.00 | 7.00 | 17976.56 | 18180.85 | 1.14 
> | 1212.26 | 2533.34 | 108.98
> RotateBenchmark.testRotateLeftB | 512.00 | 15.00 | 17553.70 | 18219.07 | 3.79 
> | 1256.73 | 2537.41 | 101.91
> RotateBenchmark.testRotateLeftB | 512.00 | 31.00 | 17618.03 | 17738.15 | 0.68 
> | 1214.69 | 2533.83 | 108.60
> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 7258.87 | 7468.88 | 2.89 | 
> 7115.12 | 7117.26 | 0.03
> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 3586.65 | 3950.85 | 10.15 | 
> 3532.17 | 3595.80 | 1.80
> RotateBenchmark.testRotateLeftI | 128.00 | 7.00 | 1835.07 | 1999.68 | 8.97 | 
> 1789.90 | 1819.93 | 1.68
> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 7273.36 | 7410.91 | 1.89 | 
> 7198.60 | 6994.79 | -2.83
> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 3674.98 | 3926.27 | 6.84 | 
> 3549.90 | 3755.09 | 5.78
> RotateBenchmark.testRotateLeftI | 128.00 | 15.00 | 1840.94 | 1882.25 | 2.24 | 
> 1801.56 | 1872.89 | 3.96
> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 7457.11 | 7361.48 | -1.28 
> | 6975.33 | 7385.94 | 5.89
> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 3570.74 | 3929.30 | 10.04 
> | 3635.37 | 3736.67 | 2.79
> RotateBenchmark.testRotateLeftI | 128.00 | 31.00 | 1902.32 | 1960.46 | 3.06 | 
> 1812.32 | 1813.88 | 0.09
> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 11174.24 | 12044.52 | 7.79 
> | 11509.87 | 11273.44 | -2.05
> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 5981.47 | 6073.70 | 1.54 | 
> 5593.66 | 5661.93 | 1.22
> RotateBenchmark.testRotateLeftI | 256.00 | 7.00 | 2932.49 | 3069.54 | 4.67 | 
> 2950.86 | 2892.42 | -1.98
> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 11764.11 | 12098.63 | 2.84 
> | 11069.52 | 11476.93 | 3.68
> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 5855.20 | 6080.40 | 3.85 | 
> 5919.11 | 5607.04 | -5.27
> RotateBenchmark.testRotateLeftI | 256.00 | 15.00 | 2989.05 | 3048.56 | 1.99 | 
> 2902.63 | 2821.83 | -2.78
> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 11652.84 | 11965.40 | 2.68 
> | 11525.62 | 11459.83 | -0.57
> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 5851.82 | 6164.94 | 5.35 | 
> 5882.60 | 5842.30 | -0.69
> RotateBenchmark.testRotateLeftI | 256.00 | 31.00 | 3015.99 | 3043.79 | 0.92 | 
> 2963.71 | 2947.97 | -0.53
> RotateBenchmark.testRotateLeftI | 512.00 | 7.00 | 16029.15 | 16189.79 | 1.00 
> | 860.43 | 2339.32 | 171.88
> RotateBenchmark.testRotateLeftI | 512.00 | 7.00 | 8078.25 | 8081.84 | 0.04 | 
> 427.39