Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing [v2]
On Fri, 31 May 2024 16:18:33 GMT, Maurizio Cimadamore
wrote:
>> This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`.
>> The cache was moved to `ValueLayouts::varHandle` as part of
>> [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we
>> want to optimize the common case like:
>>
>>
>> ValueLayout layout = ...
>> layout.varHandle().get(...)
>>
>>
>> And that caching more complex var handles didn't seem to add value, given
>> that, for these var handles, the logic in `LayoutPath` needs to adapt the
>> returned var handle anyways.
>>
>> But, `TestAccessModes` revealed a different picture - w/o any cache in
>> `Utils` the test end up allocating 8963 var handle instances (instead of
>> just 4), in each of the 4 runs the test includes. While this is admittedly a
>> stress test, it seems nice to restore the level of sharing we had before
>> [pull/19251](https://git.openjdk.org/jdk/pull/19251).
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments
Thanks for adding the comment. Looks good
src/java.base/share/classes/jdk/internal/foreign/Utils.java line 98:
> 96: *
> 97: * We provide two level of caching of the generated var handles.
> First, the var handle associated
> 98: * to a {@link ValueLayout#varHandle()} call is cached inside a
> stable field of the value layout implementation.
Suggestion:
* with a {@link ValueLayout#varHandle()} call is cached inside a stable
field of the value layout implementation.
-
Marked as reviewed by jvernee (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/19485#pullrequestreview-2091379947
PR Review Comment: https://git.openjdk.org/jdk/pull/19485#discussion_r1622749916
Re: RFR: 8333236: Test java/foreign/TestAccessModes.java is timing out after passing
On Thu, 30 May 2024 16:15:22 GMT, Maurizio Cimadamore wrote: > This PR restores a var handle cache in `Utils::makeSegmentViewVarHandle`. The > cache was moved to `ValueLayouts::varHandle` as part of > [pull/19251](https://git.openjdk.org/jdk/pull/19251), on the basis that we > want to optimize the common case like: > > > ValueLayout layout = ... > layout.varHandle().get(...) > > > And that caching more complex var handles didn't seem to add value, given > that, for these var handles, the logic in `LayoutPath` needs to adapt the > returned var handle anyways. > > But, `TestAccessModes` revealed a different picture - w/o any cache in > `Utils` the test end up allocating 8963 var handle instances (instead of just > 4), in each of the 4 runs the test includes. While this is admittedly a > stress test, it seems nice to restore the level of sharing we had before > [pull/19251](https://git.openjdk.org/jdk/pull/19251). I suggest leaving a comment to document which cases this cache is trying to address. I think that's mainly cases where the same ValueLayout is created many times in different places (instead of the same layout being reused, for which we already have a cache field on the layout instance itself). - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/19485#pullrequestreview-201021
Re: RFR: 8332826: Make hashCode methods in ArraysSupport friendlier [v2]
On Mon, 27 May 2024 20:55:29 GMT, Pavel Rappo wrote:
>> Please review this PR, which supersedes a now withdrawn
>> https://github.com/openjdk/jdk/pull/14831.
>>
>> This PR replaces `ArraysSupport.vectorizedHashCode` with a set of more
>> user-friendly methods. Here's a summary:
>>
>> - Made the operand constants (i.e. `T_BOOLEAN` and friends) and the
>> `vectorizedHashCode` method private
>>
>> - Made the `vectorizedHashCode` method private, but didn't rename it.
>> Renaming would dramatically increase this PR review cost, because that
>> method's name is used by a lot of VM code. On a bright side, since the
>> method is now private, it's no longer callable by clients of
>> `ArraysSupport`, thus a problem of an inaccurate name is less severe.
>>
>> - Made the `ArraysSupport.utf16HashCode` method private
>>
>> - Moved tiny cases (i.e. 0, 1, 2) to `ArraysSupport`
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix incorrect utf16 hashCode adaptation
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 252:
> 250: return switch (length) {
> 251: case 0 -> initialValue;
> 252: case 1 -> 31 * initialValue + (int) a[fromIndex];
Suggestion:
case 1 -> 31 * initialValue + (int) a[fromIndex]; // sign extension
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 275:
> 273: return switch (length) {
> 274: case 0 -> initialValue;
> 275: case 1 -> 31 * initialValue + (a[fromIndex] & 0xff);
For clarity, if you think it helps:
Suggestion:
case 1 -> 31 * initialValue + Byte.toUnsignedInt(a[fromIndex]);
src/java.base/share/classes/jdk/internal/util/ArraysSupport.java line 301:
> 299: return switch (length) {
> 300: case 0 -> initialValue;
> 301: case 1 -> 31 * initialValue + JLA.getUTF16Char(a, fromIndex);
There seems to be a mismatch here with the original code in StringUTF16, where
the length that is tested for is `2` instead of `1`.
test/hotspot/jtreg/compiler/intrinsics/TestArraysHashCode.java line 88:
> 86: private static int testIntrinsic(byte[] bytes, int type)
> 87: throws InvocationTargetException, IllegalAccessException {
> 88: return (int) vectorizedHashCode.invoke(null, bytes, 0, 256, 1,
> type);
Better to just call `hashCodeOfUnsigned` here I think.
The test for the non-constant type could be dropped. That is no longer a part
of the 'API' of `ArraySupport`. It looks like the intrinsic bails out when the
basic type is not constant any ways:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/library_call.cpp#L6401-L6404
-
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778741
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617778493
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r161798
PR Review Comment: https://git.openjdk.org/jdk/pull/19414#discussion_r1617784996
Re: RFR: 8331865: Consolidate size and alignment checks in LayoutPath [v3]
On Tue, 21 May 2024 10:20:27 GMT, Maurizio Cimadamore
wrote:
>> When creating a nested memory access var handle, we ensure that the segment
>> is accessed at the correct alignment for the root layout being accessed. But
>> we do not ensure that the segment has at least the size of the accessed root
>> layout. Example:
>>
>>
>> MemoryLayout LAYOUT = sequenceLayout(2, structLayout(JAVA_INT.withName("x"),
>> JAVA_INT.withName("y")));
>> VarHandle X_HANDLE = LAYOUT.varHandle(PathElement.sequenceElement(),
>> PathElement.groupElement("x"));
>>
>>
>> If I have a memory segment whose size is 12, I can successfully call
>> X_HANDLE on it with index 1, like so:
>>
>>
>> MemorySegment segment = Arena.ofAuto().allocate(12);
>> int x = (int)X_HANDLE.get(segment, 0, 1);
>>
>>
>> This seems incorrect: after all, the provided segment doesn't have enough
>> space to fit _two_ elements of the nested structs.
>>
>> This PR adds checks to detect this kind of scenario earlier, thus improving
>> usability. To achieve this we could, in principle, just tweak
>> `LayoutPath::checkEnclosingLayout` and add the required size check.
>>
>> But doing so will add yet another redundant check on top of the other
>> already added by [pull/19124](https://git.openjdk.org/jdk/pull/19124).
>> Instead, this PR rethinks how memory segment var handles are created, in
>> order to eliminate redundant checks.
>>
>> The main observation is that size and alignment checks depend on an
>> _enclosing_ layout. This layout *might* be the very same value layout being
>> accessed (this is the case when e.g. using `JAVA_INT.varHandle()`). But, in
>> the general case, the accessed value layout and the enclosing layout might
>> differ (e.g. think about accessing a struct field).
>>
>> Furthermore, the enclosing layout check depends on the _base_ offset at
>> which the segment is accessed, _prior_ to any index computation that occurs
>> if the accessed layout path has any open elements. It is important to notice
>> that this base offset is only available when looking at the var handle that
>> is returned to the user. For instance, an indexed var handle with
>> coordinates:
>>
>>
>> (MemorySegment, long /* base */, long /* index 1 */, long /* index 2 */,
>> long /* index 3 */)
>>
>>
>> Is obtained through adaptation, by taking a more basic var handle of the
>> form:
>>
>>
>> (MemorySegment, long /* offset */)
>>
>>
>> And then injecting the result of the index multiplication into `offset`. As
>> such, we can't add an enclosing layout check inside the var handle returned
>> by `VarHandles::memorySegmentViewHandle`, as doing so will end up seeing the
>> *wrong* off...
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix typo in javadoc
Marked as reviewed by jvernee (Reviewer).
src/java.base/share/classes/java/lang/invoke/X-VarHandleSegmentView.java.template
line 123:
> 121: static $type$ get(VarHandle ob, Object obb, long base) {
> 122: VarHandleSegmentViewBase handle = (VarHandleSegmentViewBase)ob;
> 123: AbstractMemorySegmentImpl bb = checkReadOnly(obb, true);
For getter methods, which pass a constant `true` here, `checkReadOnly`
essentially just does a null check and cast on the segment. Not sure if it's
worth simplifying... (I'm happy if you want to leave it like this as well)
-
PR Review: https://git.openjdk.org/jdk/pull/19251#pullrequestreview-2072366007
PR Review Comment: https://git.openjdk.org/jdk/pull/19251#discussion_r1610647133
Re: RFR: 8332547: Unloaded signature classes in DirectMethodHandles
On Tue, 21 May 2024 18:02:45 GMT, Vladimir Ivanov wrote: > I can definitely name it differently (e.g, ensureTypeVisible), but making a > separate class loading pass across signature classes doesn't make much sense. Ok, in that case I suggest also renaming `MemberName::checkForTypeAlias`, maybe to `ensureTypeVisible` as well. - PR Comment: https://git.openjdk.org/jdk/pull/19319#issuecomment-2123368855
Re: RFR: 8332547: Unloaded signature classes in DirectMethodHandles
On Mon, 20 May 2024 21:29:20 GMT, Vladimir Ivanov wrote: > JVM routinely installs loader constraints for unloaded signature classes when > method resolution takes place. MethodHandle resolution took a different route > and eagerly resolves signature classes instead (see > `java.lang.invoke.MemberName$Factory::resolve` and > `sun.invoke.util.VerifyAccess::isTypeVisible` for details). > > There's a micro-optimization which bypasses eager resolution for `java.*` > classes. The downside is that `java.*` signature classes can show up as > unloaded. It manifests as inlining failures during JIT-compilation and may > cause severe performance issues. > > Proposed fix removes the aforementioned special case logic during > `MethodHandle` resolution. > > In some cases it may slow down `MethodHandle` construction a bit (e.g., when > repeatedly constructing `DirectMethodHandle`s with lots of arguments), but > `MethodHandle` construction step is not performance critical. > > Testing: hs-tier1 - hs-tier4 Loading classes seems like a side-effect of `isTypeVisible`. I suggest adding a separate pass that explicitly loads all the signature classes in `MemberName$Factory::resolve`. I think that would make the intent clearer. - PR Comment: https://git.openjdk.org/jdk/pull/19319#issuecomment-2122077296
Re: RFR: 8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types [v3]
On Fri, 26 Apr 2024 11:51:51 GMT, Claes Redestad wrote:
>> This PR makes ClassDesc.ofDescriptor return the shared constant for
>> primitive descriptor strings ("I" etc..), and leverages this further by
>> refactoring `MethodTypeDescImpl.ofDescriptor` to avoid the intermediate
>> substring for primitives.
>>
>> Microbenchmarks results look good with expected speedups and allocation
>> reductions any time a primitive is part of the descriptor string, and a
>> non-significant cost otherwise:
>>
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> ClassDescFactories.ofDescriptor
>> Ljava/lang/Object; 6 13,941 ± 1,643 14,071 ± 1,333 ns/op
>> 0,99x (p = 0,681 )
>> :gc.alloc.rate.norm
>>16,000 ± 0,000 16,000 ± 0,000 B/op 1,00x (p = 0,617 )
>> ClassDescFactories.ofDescriptor
>> V 69,212 ± 1,045 1,405 ± 0,049 ns/op 6,55x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> ClassDescFactories.ofDescriptor
>> I 69,009 ± 0,035 1,431 ± 0,192 ns/op 6,30x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 182,050 ± 4,333141,644 ±
>> 2,685 ns/op 1,29x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 360,001 ± 0,000264,001 ± 0,000 B/op 0,73x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 17,169 ± 2,008 9,915 ± 0,018 ns/op 1,73x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 120,000 ± 0,000104,000 ± 0,000 B/op 0,87x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 270,372 ± 3,624217,050 ±
>> 3,170 ns/op 1,25x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 520,002 ± 0,000328,001 ± 0,000 B/op 0,63x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 35,036 ± 0,351 36...
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Copyrights and other comments from @JornVernee
LGTM
-
Marked as reviewed by jvernee (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18971#pullrequestreview-2024881469
Re: RFR: 8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types [v2]
On Fri, 26 Apr 2024 11:49:19 GMT, Claes Redestad wrote: > > Does removing the explicit null checks make that much difference for > > performance? They are kind of nice for clarity. > > It helps startup at least. The redundant array depth check mattered a bit for > peak performance, but not much. I don't have a strong opinion really, just > find them a bit superfluous when there's a dereference on the next line. Sure, I'm mostly talking about cases where the value is passed to another method. - PR Comment: https://git.openjdk.org/jdk/pull/18971#issuecomment-2079249354
Re: RFR: 8331187: Optimize MethodTypeDesc and ClassDesc.ofDescriptor for primitive types [v2]
On Fri, 26 Apr 2024 10:54:49 GMT, Claes Redestad wrote:
>> This PR makes ClassDesc.ofDescriptor return the shared constant for
>> primitive descriptor strings ("I" etc..), and leverages this further by
>> refactoring `MethodTypeDescImpl.ofDescriptor` to avoid the intermediate
>> substring for primitives.
>>
>> Microbenchmarks results look good with expected speedups and allocation
>> reductions any time a primitive is part of the descriptor string, and a
>> non-significant cost otherwise:
>>
>>
>> Name
>> (descString) Cnt Base Error Test Error Unit Change
>> ClassDescFactories.ofDescriptor
>> Ljava/lang/Object; 6 13,941 ± 1,643 14,071 ± 1,333 ns/op
>> 0,99x (p = 0,681 )
>> :gc.alloc.rate.norm
>>16,000 ± 0,000 16,000 ± 0,000 B/op 1,00x (p = 0,617 )
>> ClassDescFactories.ofDescriptor
>> V 69,212 ± 1,045 1,405 ± 0,049 ns/op 6,55x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> ClassDescFactories.ofDescriptor
>> I 69,009 ± 0,035 1,431 ± 0,192 ns/op 6,30x (p = 0,000*)
>> :gc.alloc.rate.norm
>>48,000 ± 0,000 0,000 ± 0,000 B/op 0,00x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> (Ljava/lang/Object;Ljava/lang/String;)I 6 182,050 ± 4,333141,644 ±
>> 2,685 ns/op 1,29x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 360,001 ± 0,000264,001 ± 0,000 B/op 0,73x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()V 6 17,169 ± 2,008 9,915 ± 0,018 ns/op 1,73x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 120,000 ± 0,000104,000 ± 0,000 B/op 0,87x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ([IJLjava/lang/String;Z)Ljava/util/List; 6 270,372 ± 3,624217,050 ±
>> 3,170 ns/op 1,25x (p = 0,000*)
>> :gc.alloc.rate.norm
>> 520,002 ± 0,000328,001 ± 0,000 B/op 0,63x (p = 0,000*)
>> MethodTypeDescFactories.ofDescriptor
>> ()[Ljava/lang/String; 6 35,036 ± 0,351 36...
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix ClassDescTest; remove redundant validation performed by
> ReferenceClassDescImpl
Looks like your `MethodTypeDescFactories` benchmark is missing form the PR?
Does removing the explicit null checks make that much difference for
performance? They are kind of nice for clarity.
Also, looks like the copyright year still needs to be updated on some of the
files.
src/java.base/share/classes/java/lang/constant/MethodTypeDescImpl.java line 97:
> 95: int args = ptypes.size() - 1;
> 96: ClassDesc[] paramTypes = args > 0
> 97: ? ptypes.subList(1, args + 1).toArray(new ClassDesc[0])
I suppose this could also use `EMPTY_CLASSDESC` instead of creating a new array
-
PR Review: https://git.openjdk.org/jdk/pull/18971#pullrequestreview-2024805435
PR Review Comment: https://git.openjdk.org/jdk/pull/18971#discussion_r1580881761
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v21]
On Fri, 19 Apr 2024 19:18:13 GMT, Scott Gibbons wrote: >> src/hotspot/share/opto/runtime.cpp line 786: >> >>> 784: fields[argp++] = TypePtr::NOTNULL;// dest >>> 785: fields[argp++] = TypeLong::LONG; // size >>> 786: fields[argp++] = Type::HALF; // size >> >> Since the size is a `size_t`, I don't think this is correct on 32-bit >> platforms. I think here we want `TypeX_X`, and then add the extra `HALF` >> only on 64-bit platforms. Similar to what we do in `make_arraycopy_Type`: >> https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/runtime.cpp#L799-L802 >> >> (Note that you will also have to adjust `argcnt` for this) > > I don't understand this well enough to be confident in the change. Can you > please verify that I've changed it properly? Your latest version looks good to me. - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572909435
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v22]
On Fri, 19 Apr 2024 16:25:28 GMT, Scott Gibbons wrote: >> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64. See >> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around >> this change. >> >> Overall, making this an intrinsic improves overall performance of >> `Unsafe::setMemory` by up to 4x for all buffer sizes. >> >> Tested with tier-1 (and full CI). I've added a table of the before and >> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`). >> >> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt) > > Scott Gibbons has updated the pull request incrementally with one additional > commit since the last revision: > > Address review comments; update copyright years I'm not really qualified as a compiler code reviewer, but I've left some comments to try and help this along. src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2523: > 2521: // Number of (8*X)-byte chunks into rScratch1 > 2522: __ movq(tmp, size); > 2523: __ shrq(tmp, 3); `shr` [sets the zero flag][1], so I think you can just move the jump to after the shift and avoid a separate comparison? ```suggestion // Number of (8*X)-byte chunks into rScratch1 __ movq(tmp, size); __ shrq(tmp, 3); __ jccb(Assembler::zero, L_Tail); [1]: https://www.felixcloutier.com/x86/sal:sar:shl:shr#flags-affected - PR Review: https://git.openjdk.org/jdk/pull/18555#pullrequestreview-2011751831 PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572712233
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v21]
On Tue, 16 Apr 2024 00:04:15 GMT, Scott Gibbons wrote:
>> This code makes an intrinsic stub for `Unsafe::setMemory` for x86_64. See
>> [this PR](https://github.com/openjdk/jdk/pull/16760) for discussion around
>> this change.
>>
>> Overall, making this an intrinsic improves overall performance of
>> `Unsafe::setMemory` by up to 4x for all buffer sizes.
>>
>> Tested with tier-1 (and full CI). I've added a table of the before and
>> after numbers for the JMH I ran (`MemorySegmentZeroUnsafe`).
>>
>> [setMemoryBM.txt](https://github.com/openjdk/jdk/files/14808974/setMemoryBM.txt)
>
> Scott Gibbons has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add enter() and leave(); remove Windows-specific register stuff
src/hotspot/cpu/x86/stubGenerator_x86_64.cpp line 4013:
> 4011: // Initialize table for unsafe copy memeory check.
> 4012: if (UnsafeMemoryAccess::_table == nullptr) {
> 4013: UnsafeMemoryAccess::create_table(26);
How did you arrive at a table size of 26?
src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2603:
> 2601: const Register wide_value = rax;
> 2602: const Register rScratch1 = r10;
> 2603:
Maybe put an `assert_different_registers` here for the above registers, just to
be sure. (I see you are avoiding the existing `rscratch1` already, because of a
conflict with `c_rarg2`)
src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2674:
> 2672: // Parameter order is (ptr, byteVal, size)
> 2673: __ xchgq(c_rarg1, c_rarg2);
> 2674: __ pop(rbp);// Clear effect of enter()
Why not just use `leave()` here?
src/hotspot/share/opto/library_call.cpp line 4959:
> 4957: if (stopped()) return true;
> 4958:
> 4959: if (StubRoutines::unsafe_setmemory() == nullptr) return false;
I don't see why this check is needed here, since we already check whether the
stub is there in `is_intrinsic_supported`.
Note that `inline_unsafe_copyMemory` also doesn't have this check. I think it
would be good to keep consistency between the two.
src/hotspot/share/opto/runtime.cpp line 780:
> 778: const TypeFunc* OptoRuntime::make_setmemory_Type() {
> 779: // create input type (domain)
> 780: int num_args = 4;
This variable seems redundant.
src/hotspot/share/opto/runtime.cpp line 786:
> 784: fields[argp++] = TypePtr::NOTNULL;// dest
> 785: fields[argp++] = TypeLong::LONG; // size
> 786: fields[argp++] = Type::HALF; // size
Since the size is a `size_t`, I don't think this is correct on 32-bit
platforms. I think here we want `TypeX_X`, and then add the extra `HALF` only
on 64-bit platforms. Similar to what we do in `make_arraycopy_Type`:
https://github.com/openjdk/jdk/blob/master/src/hotspot/share/opto/runtime.cpp#L799-L802
(Note that you will also have to adjust `argcnt` for this)
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572570842
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572578437
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572593795
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572556648
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572564382
PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1572562058
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v6]
On Thu, 18 Apr 2024 11:32:13 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Change exception type Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2008729407
Re: RFR: 8330467: NoClassDefFoundError when lambda is in a hidden class
On Wed, 17 Apr 2024 08:46:59 GMT, Adam Sotona wrote: > Current implementation of `LambdaMetafactory` does not allow to use lambdas > in hidden classes. Invocation throws `NoClassDefFoundError` instead. > > This patch includes lambda implementation in a hidden class under the special > handling of `useImplMethodHandle`. > The patch also fixes `j/l/i/defineHiddenClass/BasicTest::testLambda` to > correctly cover this test case. > > Please review. > > Thanks, > Adam Do we also need to check if the parameters or return type are hidden classes? I suppose if we compile a direct bytecode reference to the target method, all of them need to be referenceable from bytecode, not just the holder. - PR Review: https://git.openjdk.org/jdk/pull/18810#pullrequestreview-2005733945
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v4]
On Mon, 15 Apr 2024 14:02:56 GMT, Per Minborg wrote:
>> While `SymbolLookup` correctly uses an `Optional` return to denote whether a
>> symbol has been found by the lookup or not (which enables composition of
>> symbol lookups), many clients end up just calling `Optional::get`, or
>> `Optional::orElseThrow()` on the result.
>>
>> This PR proposes to add a convenience method `SymbolLookup::findOrThrow`
>> that will do a lookup and, if no symbol can be found, throws an
>> `IllegalArgumentException` with a relevant exception message.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java
>
> Co-authored-by: Maurizio Cimadamore
> <54672762+mcimadam...@users.noreply.github.com>
src/java.base/share/classes/java/lang/foreign/package-info.java line 114:
> 112: *
> 113: * Here, we obtain a {@linkplain java.lang.foreign.Linker#nativeLinker()
> native linker}
> 114: * and we use it to {@linkplain
> java.lang.foreign.SymbolLookup#findOrThrow(java.lang.String)}
Looks like the plain text was dropped here:
Suggestion:
* and we use it to {@linkplain
java.lang.foreign.SymbolLookup#findOrThrow(java.lang.String) look up}
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18474#discussion_r1567435862
Re: RFR: 8330272: Wrong javadoc for ValueLayout.JAVA_LONG/JAVA_DOUBLE on x86 32bit
On Tue, 16 Apr 2024 07:09:55 GMT, Per Minborg wrote: > This PR proposes to update the javadocs for `ValueLayout.JAVA_LONG` and > `ValueLayout.JAVA_DOUBLE` to reflect the changes made in > https://bugs.openjdk.org/browse/JDK-8326172 (we forgot to update the docs > when that issue was fixed) Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18791#pullrequestreview-2003521381
Re: RFR: 8329331: Intrinsify Unsafe::setMemory [v20]
On Mon, 15 Apr 2024 22:22:38 GMT, Sandhya Viswanathan wrote: >> Scott Gibbons has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Fix memory mark after sync to upstream > > src/hotspot/cpu/x86/stubGenerator_x86_64_arraycopy.cpp line 2686: > >> 2684: __ movq(rdx, rsi); >> 2685: restore_arg_regs(); >> 2686: #endif > > This is stubGenerator_x86_64.cpp 64bit specific, so WIN32 portion could be > removed? `_WIN32` is also defined for 64 bit Windows - PR Review Comment: https://git.openjdk.org/jdk/pull/18555#discussion_r1566523189
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints [v3]
On Mon, 15 Apr 2024 14:21:23 GMT, Per Minborg wrote: >> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that >> returns the maximum byte alignment of a segment (both heap and native >> segments). >> >> Clients can then use this method to determine if a segment is properly >> aligned for any given layout (e.g. following a `MemorySegment::reinterpret` >> operation). > > Per Minborg has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains ten additional > commits since the last revision: > > - Update after comments > - Merge branch 'master' into ms-reinterpret2 > - Update > src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java > >Co-authored-by: Jorn Vernee > - Update > src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java > >Co-authored-by: Jorn Vernee > - Update src/java.base/share/classes/java/lang/foreign/MemorySegment.java > >Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> > - Fix imports and copyright > - Update maxByteAlignment docs > - Improve doc and tests > - Add a MS::maxByteAlignment method Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18779#pullrequestreview-2001722291
Re: RFR: 8314592: Add shortcut to SymbolLookup::find [v4]
On Mon, 15 Apr 2024 14:02:56 GMT, Per Minborg wrote: >> While `SymbolLookup` correctly uses an `Optional` return to denote whether a >> symbol has been found by the lookup or not (which enables composition of >> symbol lookups), many clients end up just calling `Optional::get`, or >> `Optional::orElseThrow()` on the result. >> >> This PR proposes to add a convenience method `SymbolLookup::findOrThrow` >> that will do a lookup and, if no symbol can be found, throws an >> `IllegalArgumentException` with a relevant exception message. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Update src/java.base/share/classes/java/lang/foreign/SymbolLookup.java > > Co-authored-by: Maurizio Cimadamore > <54672762+mcimadam...@users.noreply.github.com> Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/18474#pullrequestreview-2001521250
Re: RFR: 8329997: Add provisions for checking memory segment alignment constraints
On Mon, 15 Apr 2024 07:50:24 GMT, Per Minborg wrote:
> This PR proposes to add a new method `MemorySegment::maxByteAlignment` that
> returns the maximum byte alignment of a segment (both heap and native
> segments).
>
> Clients can then use this method to determine if a segment is properly
> aligned for any given layout (e.g. following a `MemorySegment::reinterpret`
> operation).
src/java.base/share/classes/jdk/internal/foreign/HeapMemorySegmentImpl.java
line 86:
> 84: alignment = maxAlignMask();
> 85: }
> 86: return Math.min(maxAlignMask(), alignment);
Similarly here:
Suggestion:
return address() == 0
? maxAlignMask()
: Math.min(maxAlignMask(), Long.lowestOneBit(address()));
src/java.base/share/classes/jdk/internal/foreign/NativeMemorySegmentImpl.java
line 68:
> 66: ? 1L << 62
> 67: : alignment;
> 68: }
This relies on the fact that `lowestOneBit` returns `0` only if `address()`
returns `0`, but I suggest making the intent clearer by checking against
`address()` directly:
Suggestion:
return address() == 0
? 1L << 62
: Long.lowestOneBit(address());
(I also suggest introducing a name for the `1L << 62` constant, e.g.
`MAX_NATIVE_ALIGN`)
test/jdk/java/foreign/TestMemoryAlignment.java line 194:
> 192: new Object[]{MemorySegment.ofArray(new
> float[]{1}), Float.BYTES},
> 193: new Object[]{MemorySegment.ofArray(new
> double[]{1}), Double.BYTES},
> 194: new
> Object[]{MemorySegment.ofBuffer(ByteBuffer.allocate(8)), Byte.BYTES}
What about the other (heap) buffer types?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565638655
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565637521
PR Review Comment: https://git.openjdk.org/jdk/pull/18779#discussion_r1565647024
Integrated: 8330049: Remove unused AbstractLinker::linkerByteOrder
On Wed, 10 Apr 2024 15:38:22 GMT, Jorn Vernee wrote: > Please review this simple cleanup which removes the > `AbstractLinker::linkerByteOrder` method. It was only used in > `AixPPC64Linker`, where we know it will always return `ByteOrder.BIG_ENDIAN` > so we can just replace the call with that. > > Testing: Local run of `run-test-jdk_foreign`. This pull request has now been integrated. Changeset: 0db42906 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/0db42906e390a98b3a6be078f1b8c3f2a03a838f Stats: 69 lines in 12 files changed: 0 ins; 58 del; 11 mod 8330049: Remove unused AbstractLinker::linkerByteOrder Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/18726
Re: RFR: 8330049: Remove unused AbstractLinker::linkerByteOrder [v2]
> Please review this simple cleanup which removes the > `AbstractLinker::linkerByteOrder` method. It was only used in > `AixPPC64Linker`, where we know it will always return `ByteOrder.BIG_ENDIAN` > so we can just replace the call with that. > > Testing: Local run of `run-test-jdk_foreign`. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: update copyright years - Changes: - all: https://git.openjdk.org/jdk/pull/18726/files - new: https://git.openjdk.org/jdk/pull/18726/files/12ccb842..eff6f471 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18726=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18726=00-01 Stats: 10 lines in 10 files changed: 0 ins; 0 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/18726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18726/head:pull/18726 PR: https://git.openjdk.org/jdk/pull/18726
RFR: 8330049: Remove unused AbstractLinker::linkerByteOrder
Please review this simple cleanup which removes the `AbstractLinker::linkerByteOrder` method. It was only used in `AixPPC64Linker`, where we know it will always return `ByteOrder.BIG_ENDIAN` so we can just replace the call with that. Testing: Local run of `run-test-jdk_foreign`. - Commit messages: - Remove unused AbstractLinker::linkerByteOrder Changes: https://git.openjdk.org/jdk/pull/18726/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18726=00 Issue: https://bugs.openjdk.org/browse/JDK-8330049 Stats: 59 lines in 12 files changed: 0 ins; 58 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18726.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18726/head:pull/18726 PR: https://git.openjdk.org/jdk/pull/18726
Re: RFR: 8329997: Add MemorySegment::reinterpretate overloads for alignment constraints
On Wed, 10 Apr 2024 12:49:11 GMT, Per Minborg wrote:
> This PR proposes to add two overloads `MemorySegment::reinterpretate` to
> allow aligned reinterpretation.
src/java.base/share/classes/jdk/internal/foreign/SegmentFactories.java line 72:
> 70: sessionImpl.addCloseAction(action);
> 71: }
> 72: return new NativeMemorySegmentImpl(Utils.alignUp(min,
> byteAlignment), byteSize, false, sessionImpl);
This is not correct. The base address of the segment should not change after a
call to reinterpret. The alignment constraint should just be used for checking
that the base address is somewhat sane. Instead, I think `reinterpretInternal`
or the overloads that take a memory layout should do the check of the alignment
of `addres()` with the given alignment constraint.
test/jdk/java/foreign/TestSegments.java line 396:
> 394: AtomicInteger counter = new AtomicInteger();
> 395: try (Arena arena = Arena.ofConfined()){
> 396: var segment =
> MemorySegment.ofAddress(41).reinterpret(JAVA_INT);
i.e. this call (and the one below) should throw an exception.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18717#discussion_r1559643107
PR Review Comment: https://git.openjdk.org/jdk/pull/18717#discussion_r1559649552
Re: RFR: JDK-8319516 - Native library suffix impact on the library loading in AIX- Java Class Loader [v4]
On Mon, 18 Mar 2024 17:43:45 GMT, Suchismith Roy wrote: >> Allow support for both .a and .so files in AIX. >> If .so file is not found, allow fallback to .a extension. >> JBS Issue: [JDK-8319516](https://bugs.openjdk.org/browse/JDK-8319516) > > Suchismith Roy has updated the pull request incrementally with one additional > commit since the last revision: > > trraling spaces I think ideally the member suffix should be added by `System.mapLibraryName` since that takes care of mapping the platform agnostic library name to a platform specific string that identifies a library. Is it possible to infer the member name from the library name? (e.g. from `clang` infer `clang(libclang.so.16)`). FWIW, there exists an API that is a more direct proxy for dlopen, which is `SymbolLookup.libaryLookup`. - PR Comment: https://git.openjdk.org/jdk/pull/17945#issuecomment-2013435496
Integrated: 8327994: Update code gen in CallGeneratorHelper
On Wed, 13 Mar 2024 11:28:27 GMT, Jorn Vernee wrote: > Update the code gen code in CallGeneratorHelper to reflect the latest state > of the libTest(Downcall/Upcall)(Stack).c and shared.h files. > > - The previous code wanted users to pipe stdout into a file. But, since we > have 5 files that need to be created, and the names of those files is > important too, I've changed the script to write to those files directly > instead. > - I moved the definition of `S_` to libVarArgs.c where it's used, as it's > not actually generated by CallGeneratorHelper. > - GitHub doesn't render the changes to some of the files, but those files > only contain either white space changes (some missing spaces after `,`), and > copyright header updates. This pull request has now been integrated. Changeset: ac2f8e5a Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/ac2f8e5af8c88cd13038b113f82bb7c17a38aa40 Stats: 36078 lines in 6 files changed: 62 ins; 40 del; 35976 mod 8327994: Update code gen in CallGeneratorHelper Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/18269
Re: RFR: 8327994: Update code gen in CallGeneratorHelper [v2]
On Wed, 20 Mar 2024 17:49:51 GMT, Maurizio Cimadamore wrote: > No changes in libTestDowncallStack.c (not even minor ones) ? No, there was a 'missing' space between the prefix parameters and the actual parameters of the stack variants, and between the arguments passed when that callback was called in the upcall lib (So, white space only). libTestDowncall.c didn't have any changes. - PR Comment: https://git.openjdk.org/jdk/pull/18269#issuecomment-2010825750
Re: RFR: 8327994: Update code gen in CallGeneratorHelper [v2]
> Update the code gen code in CallGeneratorHelper to reflect the latest state > of the libTest(Downcall/Upcall)(Stack).c and shared.h files. > > - The previous code wanted users to pipe stdout into a file. But, since we > have 5 files that need to be created, and the names of those files is > important too, I've changed the script to write to those files directly > instead. > - I moved the definition of `S_` to libVarArgs.c where it's used, as it's > not actually generated by CallGeneratorHelper. > - GitHub doesn't render the changes to some of the files, but those files > only contain either white space changes (some missing spaces after `,`), and > copyright header updates. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Delete extra space Co-authored-by: Andrey Turbanov - Changes: - all: https://git.openjdk.org/jdk/pull/18269/files - new: https://git.openjdk.org/jdk/pull/18269/files/e66b7671..d282ecbd Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18269=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18269=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/18269.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18269/head:pull/18269 PR: https://git.openjdk.org/jdk/pull/18269
RFR: 8327994: Update code gen in CallGeneratorHelper
Update the code gen code in CallGeneratorHelper to reflect the latest state of the libTest(Downcall/Upcall)(Stack).c and shared.h files. - The previous code wanted users to pipe stdout into a file. But, since we have 5 files that need to be created, and the names of those files is important too, I've changed the script to write to those files directly instead. - I moved the definition of `S_` to libVarArgs.c where it's used, as it's not actually generated by CallGeneratorHelper. - GitHub doesn't render the changes to some of the files, but those files only contain either white space changes (some missing spaces after `,`), and copyright header updates. - Commit messages: - add more run instructions - simplify - use export.h - factor out some shared code - fix test file generation - remove dead code from CallGeneratorHelper Changes: https://git.openjdk.org/jdk/pull/18269/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18269=00 Issue: https://bugs.openjdk.org/browse/JDK-8327994 Stats: 36078 lines in 6 files changed: 62 ins; 40 del; 35976 mod Patch: https://git.openjdk.org/jdk/pull/18269.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18269/head:pull/18269 PR: https://git.openjdk.org/jdk/pull/18269
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Tue, 12 Mar 2024 13:51:28 GMT, Magnus Ihse Bursie wrote:
>> test/jdk/java/foreign/CallGeneratorHelper.java line 216:
>>
>>> 214: if (header) {
>>> 215: System.out.println(
>>> 216: "#include \"export.h\"\n"
>>
>> We don't generate these header files any more, so the changes to this file
>> are not really needed.
>
> I still wouldn't like to keep the bad hard-coded defines. Are you okay with
> me pushing these changes, and then you can remove the parts of the test that
> are not actually used anymore? If the code is not used it should not matter
> much to you either way.
>
> (I mean I could back out these changes, but then we'd have the bad code in
> place while waiting for you to remove it, putting pressure on you to actually
> remove it.)
Yes, that's fine. Sorry, I meant to file a JBS issue and come back with another
comment last time, but I got distracted. Filed:
https://bugs.openjdk.org/browse/JDK-8327994 now
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1521715617
Re: RFR: 8327460: Compile tests with the same visibility rules as product code [v2]
On Wed, 6 Mar 2024 13: 43: 00 GMT, Magnus Ihse Bursie wrote: >> Currently, our symbol visibility handling for tests are sloppy; we only handle it properly on Windows. We need to bring it up to the same levels as
ZjQcmQRYFpfptBannerStart
This Message Is From an External Sender
This message came from outside your organization.
Report Suspicious
ZjQcmQRYFpfptBannerEnd
On Wed, 6 Mar 2024 13:43:00 GMT, Magnus Ihse Bursie wrote:
>> Currently, our symbol visibility handling for tests are sloppy; we only handle it properly on Windows. We need to bring it up to the same levels as product code. This is a prerequisite for [JDK-8327045](https://bugs.openjdk.org/browse/JDK-8327045), which in turn is a building block for Hermetic Java.
>
> Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision:
>
> Update line number for dereference_null in TestDwarf
Thanks, this looks good to me.
test/jdk/java/foreign/CallGeneratorHelper.java line 216:
> 214: if (header) {
> 215: System.out.println(
> 216: "#include \"export.h\"\n"
We don't generate these header files any more, so the changes to this file are not really needed.
-
Marked as reviewed by jvernee (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/18135#pullrequestreview-1925503653
PR Review Comment: https://git.openjdk.org/jdk/pull/18135#discussion_r1518086954
Integrated: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
On Mon, 26 Feb 2024 13:24:11 GMT, Jorn Vernee wrote: > This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, > regardless of the underlying platform. This means that atomic access modes > work on memory segments wrapping `long[]` or `double[]`, as they already do > when using `MethodHandless::arrayAccessVarHandle`. > > After discussion, we came to the conclusion that it is reasonable for the JDK > to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It > is ultimately up to the JDK to set these requirements, which are for the VM > to implement. > > I was seeing a stack overflow when running > test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've > lowered the recursion to 50 (which is still more than enough I think). > > Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 > Linux (uses fallback linker) This pull request has now been integrated. Changeset: 2372aba6 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/2372aba6a21c569d4d724396e59b9fd1bec90682 Stats: 67 lines in 10 files changed: 18 ins; 23 del; 26 mod 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/18007
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 19:13:41 GMT, Jorn Vernee wrote: >> That doesn't seem to be the right PR link? > > Found the right one: > https://github.com/openjdk/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb Switched back to using the old generator (and removed the newer one): https://github.com/openjdk/jdk/pull/18007/commits/fad15a66b68b07b8bb17fce3c4a7d8788ef44015 - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503174593
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 19:10:39 GMT, Jorn Vernee wrote: >> test/jdk/java/foreign/TestLayouts.java line 164: >> >>> 162: ); >>> 163: assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8); >>> 164: assertEquals(struct.byteAlignment(), 8); >> >> Looking at this PR: >> https://github.com/openjdk/jdk/pull/18007 >> >> This test seemed to cover more case before that PR - e.g. a layout generator >> was tweaked to exclude long/doubles. I believe revert the test changes now? > > That doesn't seem to be the right PR link? Found the right one: https://github.com/openjdk/jdk/commit/44218b1c9e5daa33557aac9336251cf8398d81eb - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503166930
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v3]
> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, > regardless of the underlying platform. This means that atomic access modes > work on memory segments wrapping `long[]` or `double[]`, as they already do > when using `MethodHandless::arrayAccessVarHandle`. > > After discussion, we came to the conclusion that it is reasonable for the JDK > to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It > is ultimately up to the JDK to set these requirements, which are for the VM > to implement. > > I was seeing a stack overflow when running > test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've > lowered the recursion to 50 (which is still more than enough I think). > > Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 > Linux (uses fallback linker) Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: re-widen test - Changes: - all: https://git.openjdk.org/jdk/pull/18007/files - new: https://git.openjdk.org/jdk/pull/18007/files/b145a58b..fad15a66 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18007=02 - incr: https://webrevs.openjdk.org/?repo=jdk=18007=01-02 Stats: 10 lines in 1 file changed: 0 ins; 4 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 16:21:38 GMT, Maurizio Cimadamore wrote: >> Jorn Vernee has updated the pull request incrementally with one additional >> commit since the last revision: >> >> review comments > > test/jdk/java/foreign/TestLayouts.java line 164: > >> 162: ); >> 163: assertEquals(struct.byteSize(), 1 + 1 + 2 + 4 + 8); >> 164: assertEquals(struct.byteAlignment(), 8); > > Looking at this PR: > https://github.com/openjdk/jdk/pull/18007 > > This test seemed to cover more case before that PR - e.g. a layout generator > was tweaked to exclude long/doubles. I believe revert the test changes now? That doesn't seem to be the right PR link? - PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1503163065
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
On Mon, 26 Feb 2024 15:10:55 GMT, Maurizio Cimadamore
wrote:
>> Jorn Vernee has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review comments
>
> src/java.base/share/classes/java/lang/foreign/MemorySegment.java line 328:
>
>> 326: * physical address 1010.
>> 327: * The starting physical address of a {@code long[]} array will be
>> 8-byte aligned
>> 328: * (e.g. 1000), so that successive long elements occur at 8-byte
>> aligned addresses
>
> I believe there might be other changes required. I see the following
> sentences in the javadoc:
>
>
> * In other words, heap segments feature a (platform-dependent)
> maximum
> * alignment which is derived from the size of the elements of the Java array
> backing the
> * segment, as shown in the following table:
> ```
>
>
> * In such circumstances, clients have two options. They can use a heap
> segment backed
> * by a different array type (e.g. {@code long[]}), capable of supporting
> greater maximum
> * alignment. More specifically, the maximum alignment associated with {@code
> long[]} is
> * set to {@code ValueLayout.JAVA_LONG.byteAlignment()} which is a
> platform-dependent
> * value (set to {@code ValueLayout.ADDRESS.byteSize()}). That is, {@code
> long[]}) is
> * guaranteed to provide at least 8-byte alignment in 64-bit platforms, but
> only 4-byte
> * alignment in 32-bit platforms:
> ```
>
> ```
> * In practice, the Java runtime lays out arrays in memory so that each
> n-byte element
> * occurs at an n-byte aligned physical address (except for {@code long[]} and
> * {@code double[]}, where alignment is platform-dependent, as explained
> below).
> ```
I got the second one already. Will modify the 1st and 3rd
-
PR Review Comment: https://git.openjdk.org/jdk/pull/18007#discussion_r1502882211
Re: RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc [v2]
> This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, > regardless of the underlying platform. This means that atomic access modes > work on memory segments wrapping `long[]` or `double[]`, as they already do > when using `MethodHandless::arrayAccessVarHandle`. > > After discussion, we came to the conclusion that it is reasonable for the JDK > to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It > is ultimately up to the JDK to set these requirements, which are for the VM > to implement. > > I was seeing a stack overflow when running > test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've > lowered the recursion to 50 (which is still more than enough I think). > > Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 > Linux (uses fallback linker) Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: review comments - Changes: - all: https://git.openjdk.org/jdk/pull/18007/files - new: https://git.openjdk.org/jdk/pull/18007/files/787e32bb..b145a58b Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=18007=01 - incr: https://webrevs.openjdk.org/?repo=jdk=18007=00-01 Stats: 3 lines in 1 file changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
RFR: 8326172: Dubious claim on long[]/double[] alignment in MemorySegment javadoc
This patch changes the alignment for `JAVA_LONG` and `JAVA_DOUBLE` to 8, regardless of the underlying platform. This means that atomic access modes work on memory segments wrapping `long[]` or `double[]`, as they already do when using `MethodHandless::arrayAccessVarHandle`. After discussion, we came to the conclusion that it is reasonable for the JDK to require the elements of a `long[]` and `double[]` to be 8 byte aligned. It is ultimately up to the JDK to set these requirements, which are for the VM to implement. I was seeing a stack overflow when running test/jdk/java/foreign/stackwalk/TestReentrantUpcalls.java on x86, so I've lowered the recursion to 50 (which is still more than enough I think). Testing: `jdk_foreign` on x64 Windows, x64 Windows + fallback linker, and x86 Linux (uses fallback linker) - Commit messages: - add missing tag - Remove references to 32-bit difference in javadoc - Test fixes - Set alignment of JAVA_LONG and JAVA_DOUBLE to 8 bytes Changes: https://git.openjdk.org/jdk/pull/18007/files Webrev: https://webrevs.openjdk.org/?repo=jdk=18007=00 Issue: https://bugs.openjdk.org/browse/JDK-8326172 Stats: 54 lines in 10 files changed: 18 ins; 18 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/18007.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/18007/head:pull/18007 PR: https://git.openjdk.org/jdk/pull/18007
Re: RFR: 8326112: Javadoc snippet for Linker.Option.captureCallState is wrong
On Mon, 19 Feb 2024 08:19:58 GMT, Per Minborg wrote: > This PR proposes to add an offset parameter for a `VarHandle` invocation in > the Javadoc snippet for `Linker.Option.captureCallState()`. The offset > parameter was added in 22 but it was forgotten to add it in said Javadoc. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17909#pullrequestreview-1888395806
Re: RFR: 8326062: ProblemList jcstress tests that are failing due to JDK-8325984
On Fri, 16 Feb 2024 15:54:27 GMT, Daniel D. Daugherty wrote: > A trivial fix to ProblemList jcstress tests that are failing due to > JDK-8325984. Thanks! - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17890#pullrequestreview-1885494458
Integrated: 8318966: Some methods make promises about Java array element alignment that are too strong
On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee wrote: > See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... This pull request has now been integrated. Changeset: 9c852df6 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/9c852df6aa019f63d6fae733d7a73521b7151dd0 Stats: 5133 lines in 19 files changed: 612 ins; 3440 del; 1081 mod 8318966: Some methods make promises about Java array element alignment that are too strong Reviewed-by: psandoz, mcimadamore - PR: https://git.openjdk.org/jdk/pull/16681
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong [v3]
> See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 12 additional commits since the last revision: - Merge branch 'master' into AlignedOffset - Merge branch 'master' into AlignedOffset - Add api note pointing to alternatives for client that require non-plain access - simplify spec for alignmentOffset and alignedSlice - Merge note about misaligned access in byteBufferViewVarHandle - updated alignedSlice implNote as well - updated alignedOffset implNote - Use ISE for bbvvh instead of UOE - restore some tests for direct buffers - fix BAVV and BBVV impl and tests - ... and 2 more: https://git.openjdk.org/jdk/compare/a2273ed0...281deaa5 - Changes: - all: https://git.openjdk.org/jdk/pull/16681/files - new: https://git.openjdk.org/jdk/pull/16681/files/eccf4f41..281deaa5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16681=02 - incr: https://webrevs.openjdk.org/?repo=jdk=16681=01-02 Stats: 14804 lines in 723 files changed: 8780 ins; 2698 del; 3326 mod Patch: https://git.openjdk.org/jdk/pull/16681.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16681/head:pull/16681 PR: https://git.openjdk.org/jdk/pull/16681
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong [v2]
On Thu, 1 Feb 2024 20:10:29 GMT, Jorn Vernee wrote: >> See the JBS issue for an extended problem description. >> >> This patch changes the specification and implementation of >> `MethodHandles::byteArrayViewVarHandle`, >> `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and >> `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the >> alignment of Java array elements, in order to bring them in line with the >> guarantees made by an arbitrary JVM implementation (which makes no >> guarantees about array element alignment beyond them being aligned to their >> natural alignment). >> >> - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any >> alignment for the accesses. Which means we can only reliably support plain >> get and set access modes. The javadoc text explaining which other access >> modes are supported, or how to compute aligned offsets into the array is >> dropped, as it is not guaranteed to be correct on all JVM implementations. >> The implementation of the returned VarHandle is changed to throw an >> `UnsupportedOperationException` for the unsupported access modes, as >> mandated by the spec of `VarHandle` [1]. >> >> - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is >> incorrect when accessing a heap buffer (wrapping a byte[]), for the same >> reasons as `byteArrayViewVarHandle`. The spec is changed to specify that >> when accessing a _heap buffer_, only plain get and set access modes are >> supported. The implementation of the returned var handle is changed to throw >> an `IllegalStateException` when an access is attempted on a heap buffer >> using an access mode other than plain get or set. Note that we don't throw >> an outright `UnsupportedOperationException` for this case, since whether the >> access modes are supported depends on the byte buffer instance being used. >> >> - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former >> method depends directly on the latter for all its alignment computations. We >> change the implementation of the latter method to throw an >> `UnsupportedOperationException` for all unit sizes greater than 1, when the >> buffer is non-direct. This change is largely covered by the existing >> specification: >> >> >> * @throws UnsupportedOperationException >> * If the native platform does not guarantee stable alignment >> offset >> * values for the given unit size when managing the memory >> regions >> * of buffers of the same kind as this buffer (direct or >> * non-direct). For example, if garbage collection would... > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 11 additional > commits since the last revision: > > - Merge branch 'master' into AlignedOffset > - Add api note pointing to alternatives for client that require non-plain > access > - simplify spec for alignmentOffset and alignedSlice > - Merge note about misaligned access in byteBufferViewVarHandle > - updated alignedSlice implNote as well > - updated alignedOffset implNote > - Use ISE for bbvvh instead of UOE > - restore some tests for direct buffers > - fix BAVV and BBVV impl and tests > - regen test files > - ... and 1 more: https://git.openjdk.org/jdk/compare/1d01cf48...eccf4f41 I'll do one more batch of testing (after merging with the latest master) and then integrate - PR Comment: https://git.openjdk.org/jdk/pull/16681#issuecomment-1941613924
Re: RFR: 8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment [v3]
On Thu, 11 Jan 2024 16:21:38 GMT, Peter Levart wrote: >> I belive there is a bug in `AbstractMemorySegmentImpl#mismatch` method. It >> returns `-1` (meaning that regions are equal) when passing the same instance >> of MemorySegment as both `srcSegment` and `dstSegment` parameters regardless >> of whether `srcFromOffset` and `dstFromOffset` as well as `srcToOffset` and >> `dstToOffset` are also equal. >> >> Am I right? > > Peter Levart has updated the pull request incrementally with one additional > commit since the last revision: > > remove special-case check from instance method - test pending @plevart Are you planning to finish this? (add a test). Or maybe someone else can take over and do the rest? - PR Comment: https://git.openjdk.org/jdk/pull/17354#issuecomment-1934640546
Re: RFR: JDK-8325148: Enable restricted javac warning in java.base
On Thu, 1 Feb 2024 21:10:48 GMT, Joe Darcy wrote: > The restricted javac warning is disabled for java.base, but could be enabled > by suppressing the warning in a handful of files. This looks good to me. It will be easier to find where we are doing restricted operations like this. - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17677#pullrequestreview-1857741290
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong [v2]
> See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 11 additional commits since the last revision: - Merge branch 'master' into AlignedOffset - Add api note pointing to alternatives for client that require non-plain access - simplify spec for alignmentOffset and alignedSlice - Merge note about misaligned access in byteBufferViewVarHandle - updated alignedSlice implNote as well - updated alignedOffset implNote - Use ISE for bbvvh instead of UOE - restore some tests for direct buffers - fix BAVV and BBVV impl and tests - regen test files - ... and 1 more: https://git.openjdk.org/jdk/compare/0468810a...eccf4f41 - Changes: - all: https://git.openjdk.org/jdk/pull/16681/files - new: https://git.openjdk.org/jdk/pull/16681/files/d74f47f1..eccf4f41 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16681=01 - incr: https://webrevs.openjdk.org/?repo=jdk=16681=00-01 Stats: 235568 lines in 5388 files changed: 128775 ins; 77246 del; 29547 mod Patch: https://git.openjdk.org/jdk/pull/16681.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16681/head:pull/16681 PR: https://git.openjdk.org/jdk/pull/16681
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v4]
On Tue, 30 Jan 2024 16:58:35 GMT, Per Minborg wrote: >> This PR proposes to add an improved Improve >> `LayoutPath.PathElement::toString` method simplifying debugging. >> >> Opinions and suggestions for `static PathElement sequenceElement(long start, >> long step)` are welcome. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Rework the LayoutPath::toString output I like the latest approach. It leans on familiar names/syntax. I think that will be more intuitive than mimicking C syntax. - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/17417#pullrequestreview-1851899390
Re: RFR: 8200559: Java agents doing instrumentation need a means to define auxiliary classes [v2]
On Fri, 26 Jan 2024 09:12:53 GMT, Alan Bateman wrote: > The target class is transformed in such a way to call the auxiliary class, > which necessitates the the aux-class to be in the same classloader as the > target class. But because the aux-class is defined while the target class is > still being transformed during class-loading, we cannot have any look-up > instance pointinmg to it yet. It seems like you could lazily define the aux class when the target class first needs it, instead of eagerly while the target class is being defined. e.g. generate the class bytes for the aux class up front, and embed them in the target class (For instance as a Base64 encoded string, which fits well into the constant pool). Then, have the transformed code in the target class define the the aux class when it is first needed, at which point you do have access to a lookup. - PR Comment: https://git.openjdk.org/jdk/pull/3546#issuecomment-1912057333
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Tue, 23 Jan 2024 14:43:24 GMT, Chen Liang wrote: > Also curious, is there any documentation (like JVMS) that allows JVMs to make > no offset into byte arrays aligned for larger access? Would be helpful if we > can have a reference here. There is simply no guarantee in the JVMS that array elements are aligned more than their size. So, the public API may not assume that they are, since it needs to be implementable by an arbitrary JVM that is JVMS compliant. - PR Comment: https://git.openjdk.org/jdk/pull/16681#issuecomment-1906687425
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Tue, 23 Jan 2024 14:30:08 GMT, Chen Liang wrote: >> Good idea. Thanks > > Should we make these unaligned access modes throw ISE like before, when the > given index is unaligned? You mean `get` and `set`? They should never throw, as unaligned access is fine. For other access modes, we can never guarantee that an access is aligned, so UOE is appropriate. (IIRC this is mandated by existing spec. I'll try to find it again) - PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1463769755
Re: RFR: 8323601: Improve LayoutPath.PathElement::toString [v2]
On Mon, 15 Jan 2024 16:01:32 GMT, Per Minborg wrote: >> This PR proposes to add an improved Improve >> `LayoutPath.PathElement::toString` method simplifying debugging. >> >> Opinions and suggestions for `static PathElement sequenceElement(long start, >> long step)` are welcome. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Rework PathElement:toString src/java.base/share/classes/java/lang/foreign/MemoryLayout.java line 958: > 956: return new LayoutPath.PathElementImpl(PathKind.DEREF_ELEMENT, > 957: LayoutPath::derefElement, > 958: "*"); It seems that this would result in paths like `a.b*` rather than the `*a.b`, which is correct C syntax. - PR Review Comment: https://git.openjdk.org/jdk/pull/17417#discussion_r1452611709
Re: RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v3]
On Fri, 12 Jan 2024 13:06:39 GMT, Per Minborg wrote: >> This PR proposes to add a clarification that an `Arena` always returns >> zeroed-out segments for `Arena::allocate` methods. >> >> Note that other overloaded methods refer to the abstract `Arena::allocate` >> method via implementation notes. > > Per Minborg has updated the pull request incrementally with one additional > commit since the last revision: > > Move documentation of zero init to each Arena factory Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17308#pullrequestreview-1818263177
Re: RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate [v2]
On Thu, 11 Jan 2024 07:59:37 GMT, Per Minborg wrote:
>> This PR proposes to add a clarification that an `Arena` always returns
>> zeroed-out segments for `Arena::allocate` methods.
>>
>> Note that other overloaded methods refer to the abstract `Arena::allocate`
>> method via implementation notes.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Move and change text on zeroing out
src/java.base/share/classes/java/lang/foreign/Arena.java line 215:
> 213: * {@linkplain #ofConfined()} and, {@linkplain #ofShared()} will return
> segments
> 214: * that are zeroed out when invoking {@linkplain Arena#allocate(long,
> long) allocate()}.
> 215: *
I'd expect to find this in the javadoc of each factory method (not in an
implSpec). The note says something about those particular methods, not about
the Arena type as a whole.
I also think "zero intialized" sounds a bit more professional than "zeroed
out". I suggest the following phrasing:
Memory segments {@linkPlain #allocate(long, long) allocated} by the
returned arena are zero initialized
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17308#discussion_r1450270020
Re: RFR: 8323552: AbstractMemorySegmentImpl#mismatch returns -1 when comparing distinct areas of the same instance of MemorySegment [v2]
On Thu, 11 Jan 2024 13:30:44 GMT, Peter Levart wrote: >> I belive there is a bug in `AbstractMemorySegmentImpl#mismatch` method. It >> returns `-1` (meaning that regions are equal) when passing the same instance >> of MemorySegment as both `srcSegment` and `dstSegment` parameters regardless >> of whether `srcFromOffset` and `dstFromOffset` as well as `srcToOffset` and >> `dstToOffset` are also equal. >> >> Am I right? > > Peter Levart has updated the pull request incrementally with one additional > commit since the last revision: > > move special-case check for equal segments to the instance method (more > probable) src/java.base/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java line 299: > 297: checkValidState(); > 298: return -1; > 299: } I'm skeptical of trying to optimize this without a more thorough performance investigation. We shouldn't add this complexity on a whim. I'm more in favor of just dropping the check from the static mismatch method and leaving it at that. (And adding a test as Maurizio mentioned). - PR Review Comment: https://git.openjdk.org/jdk/pull/17354#discussion_r1449022827
[jdk22] Integrated: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
On Wed, 10 Jan 2024 13:18:17 GMT, Jorn Vernee wrote: > Hi all, > > This pull request contains a backport of commit > [d2d58dd6](https://github.com/openjdk/jdk/commit/d2d58dd6a8ec366a4bc3eb12a253b252de24557e) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jorn Vernee on 10 Jan 2024 and > was reviewed by Maurizio Cimadamore. > > This is a P2 test only change, and we are currently in Ramp Down Phase 1. The > release process allows P1-P5 test-only changes during RDP1: > https://openjdk.org/jeps/3#Quick-reference > > Thanks! This pull request has now been integrated. Changeset: db342758 Author:Jorn Vernee URL: https://git.openjdk.org/jdk22/commit/db3427582df5913e9299e8ecceecd9bddb3b7358 Stats: 16 lines in 1 file changed: 3 ins; 0 del; 13 mod 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process 8322637: java/foreign/critical/TestCriticalUpcall.java timed out Reviewed-by: mcimadamore Backport-of: d2d58dd6a8ec366a4bc3eb12a253b252de24557e - PR: https://git.openjdk.org/jdk22/pull/52
[jdk22] RFR: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
Hi all, This pull request contains a backport of commit [d2d58dd6](https://github.com/openjdk/jdk/commit/d2d58dd6a8ec366a4bc3eb12a253b252de24557e) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Jorn Vernee on 10 Jan 2024 and was reviewed by Maurizio Cimadamore. This is a P2 test only change, and we are currently in Ramp Down Phase 1. The release process allows P1-P5 test-only changes during RDP1: https://openjdk.org/jeps/3#Quick-reference Thanks! - Commit messages: - Backport d2d58dd6a8ec366a4bc3eb12a253b252de24557e Changes: https://git.openjdk.org/jdk22/pull/52/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=52=00 Issue: https://bugs.openjdk.org/browse/JDK-8322324 Stats: 16 lines in 1 file changed: 3 ins; 0 del; 13 mod Patch: https://git.openjdk.org/jdk22/pull/52.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/52/head:pull/52 PR: https://git.openjdk.org/jdk22/pull/52
Re: RFR: 8323529: Relativize test image dependencies in microbenchmarks
On Wed, 10 Jan 2024 15:10:58 GMT, Claes Redestad wrote: > JMH microbenchmarks may have dependencies on artifacts in the test image > outside of the benchmarks.jar. This includes native libraries (built into > `$TEST_IMAGE/micro/native`) and may soon include other test libraries like > wb.jar (built into `$TEST_IMAGE/lib-test/`) > > By moving execution to the test image root (currently we run out of the > `make` directory) we can make do with relative paths, which means benchmark > can supply a build-time constant flag themselves rather than have the test > runner provide it for you. And needing to be explicit about external > dependencies is good, I think. > > Taken together this makes the benchmarks.jar more self-contained and simpler > to run: all you need is to run `java -jar micro/benchmarks.jar` from the test > image root. > > This patch also drive-by fixes some lang.foreign tests that were printing > warnings due to a missing `--enable-native-access=ALL-UNNAMED` flag. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/17349#pullrequestreview-1813501669
Re: RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
On Wed, 10 Jan 2024 14:21:15 GMT, Maurizio Cimadamore wrote: > (how does a client know if what they get back is zeroed?) It seems similar to e.g. a HashMap vs. LinkedHashMap with regards to ordering. The creator of the Arena would decide the zeroing strategy. - PR Review Comment: https://git.openjdk.org/jdk/pull/17308#discussion_r1447463240
Re: RFR: 8323159: Consider adding some text re. memory zeroing in Arena::allocate
On Mon, 8 Jan 2024 16:16:50 GMT, Per Minborg wrote: > This PR proposes to add a clarification that an `Arena` always returns > zeroed-out segments for `Arena::allocate` methods. > > Note that other overloaded methods refer to the abstract `Arena::allocate` > method via implementation notes. src/java.base/share/classes/java/lang/foreign/Arena.java line 269: > 267: * @implSpec > 268: * Implementations of this method must return a native, > zero-initialized segment > 269: * featuring the requested size, and that is compatible with the > provided alignment Do we want to _mandate_ that all arenas return zero-initialized memory? Maybe it's enough to say that the default implementation does this. - PR Review Comment: https://git.openjdk.org/jdk/pull/17308#discussion_r1447440934
Integrated: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
On Tue, 9 Jan 2024 14:18:37 GMT, Jorn Vernee wrote: > The issue with this test, and the test of JDK-8322324, seems to be that the > forked processes write to stderr/stdout, without this output being read > before the process terminates. The buffer might fill up if the output is not > being read, which means that the process will stall when writing (see stack > trace in JBS issue). > > `OutputAnalyzer` has ways to prevent this by continuously reading from the > output streams in separate threads, but because the current code calls > `Process::waitFor` before creating the `OutputAnalyzer`, we never actually > read the written output of the fork, which occasionally results in a stall > and subsequent timeout. > > The fix proposed by this patch is to use `ProcessTools::startProcess`, > instead of `ProcessBuilder::start`, which will also start the necessary > reader threads, preventing a stall. Incidentally, `startProcess` also has > built-in timeout handling which we can use. > > Testing: > - 500 runs of both java/foreign/TestStubAllocFailure.java and > java/foreign/critical/TestCriticalUpcall on various Windows x64 hosts (100 > iterations was enough to observe the failure twice). > - `jdk_foreign` suite. This pull request has now been integrated. Changeset: d2d58dd6 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/d2d58dd6a8ec366a4bc3eb12a253b252de24557e Stats: 16 lines in 1 file changed: 3 ins; 0 del; 13 mod 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process 8322637: java/foreign/critical/TestCriticalUpcall.java timed out Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/17324
Re: RFR: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process [v2]
> The issue with this test, and the test of JDK-8322324, seems to be that the > forked processes write to stderr/stdout, without this output being read > before the process terminates. The buffer might fill up if the output is not > being read, which means that the process will stall when writing (see stack > trace in JBS issue). > > `OutputAnalyzer` has ways to prevent this by continuously reading from the > output streams in separate threads, but because the current code calls > `Process::waitFor` before creating the `OutputAnalyzer`, we never actually > read the written output of the fork, which occasionally results in a stall > and subsequent timeout. > > The fix proposed by this patch is to use `ProcessTools::startProcess`, > instead of `ProcessBuilder::start`, which will also start the necessary > reader threads, preventing a stall. Incidentally, `startProcess` also has > built-in timeout handling which we can use. > > Testing: > - 500 runs of both java/foreign/TestStubAllocFailure.java and > java/foreign/critical/TestCriticalUpcall on various Windows x64 hosts (100 > iterations was enough to observe the failure twice). > - `jdk_foreign` suite. Jorn Vernee has updated the pull request incrementally with one additional commit since the last revision: Update copyright year - Changes: - all: https://git.openjdk.org/jdk/pull/17324/files - new: https://git.openjdk.org/jdk/pull/17324/files/2ba2851c..58f5cabf Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=17324=01 - incr: https://webrevs.openjdk.org/?repo=jdk=17324=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/17324.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17324/head:pull/17324 PR: https://git.openjdk.org/jdk/pull/17324
RFR: 8322324: java/foreign/TestStubAllocFailure.java times out while waiting for forked process
The issue with this test, and the test of JDK-8322324, seems to be that the forked processes write to stderr/stdout, without this output being read before the process terminates. The buffer might fill up if the output is not being read, which means that the process will stall when writing (see stack trace in JBS issue). `OutputAnalyzer` has ways to prevent this by continuously reading from the output streams in separate threads, but because the current code calls `Process::waitFor` before creating the `OutputAnalyzer`, we never actually read the written output of the fork, which occasionally results in a stall and subsequent timeout. The fix proposed by this patch is to use `ProcessTools::startProcess`, instead of `ProcessBuilder::start`, which will also start the necessary reader threads, preventing a stall. Incidentally, `startProcess` also has built-in timeout handling which we can use. Testing: - 500 runs of both java/foreign/TestStubAllocFailure.java and java/foreign/critical/TestCriticalUpcall on various Windows x64 hosts (100 iterations was enough to observe the failure twice). - `jdk_foreign` suite. - Commit messages: - use ProcessTools.startProcess to avoid stderr getting stuck Changes: https://git.openjdk.org/jdk/pull/17324/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17324=00 Issue: https://bugs.openjdk.org/browse/JDK-8322324 Stats: 15 lines in 1 file changed: 3 ins; 0 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/17324.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17324/head:pull/17324 PR: https://git.openjdk.org/jdk/pull/17324
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Fri, 15 Dec 2023 05:15:17 GMT, David Holmes wrote: >> Any non-zero exit value is acceptable. The intent here is to check that the >> process didn't complete normally. > > A non-zero non-crashing value. Just wondering what that actually would be? For instance, when we exit due to an uncaught exception, the exit value is 1. - PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1427705400
[jdk22] Integrated: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 17:38:27 GMT, Jorn Vernee wrote: > Hi all, > > This pull request contains a backport of commit > [7ece9e90](https://github.com/openjdk/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd) > from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. > > The commit being backported was authored by Jorn Vernee on 13 Dec 2023 and > was reviewed by Maurizio Cimadamore. > > This is a test-only P4 change. So it's allowed under the release process in > RDP1: https://openjdk.org/jeps/3 > > Thanks! This pull request has now been integrated. Changeset: d62249a3 Author:Jorn Vernee URL: https://git.openjdk.org/jdk22/commit/d62249a3abec28be0b3b9797f899adbdfd941cbe Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion Reviewed-by: mcimadamore Backport-of: 7ece9e90c0198f92cdf8d620e346c4a9832724cd - PR: https://git.openjdk.org/jdk22/pull/11
Re: RFR: 8320919: Clarify Locale related system properties [v5]
On Wed, 13 Dec 2023 05:19:25 GMT, Stuart Marks wrote:
>> The `-D` command-line option is not a part of the Java SE specification but
>> an allowed behavior, so it may not be a normative description here.
>
> Right, I suggested putting that in parentheses. Historically we haven't been
> very formal about distinguishing between normative (Java SE) specifications
> and informative text that talks about implementations. In this case I felt
> that enclosing the text in parentheses and also adding hedging words
> ("typically") made it clear that this text isn't normative.
FWIW, for the FFM API, we describe the `--enable-native-access` command line
flag in a separate `@implNote`:
https://github.com/openjdk/jdk/blob/cf948548c390c42ca63525d41a9d63ff31349c3a/src/java.base/share/classes/java/lang/foreign/package-info.java#L164-L171
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17065#discussion_r1425900844
[jdk22] RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
Hi all, This pull request contains a backport of commit [7ece9e90](https://github.com/openjdk/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd) from the [openjdk/jdk](https://git.openjdk.org/jdk) repository. The commit being backported was authored by Jorn Vernee on 13 Dec 2023 and was reviewed by Maurizio Cimadamore. Thanks! - Commit messages: - Backport 7ece9e90c0198f92cdf8d620e346c4a9832724cd Changes: https://git.openjdk.org/jdk22/pull/11/files Webrev: https://webrevs.openjdk.org/?repo=jdk22=11=00 Issue: https://bugs.openjdk.org/browse/JDK-8321400 Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod Patch: https://git.openjdk.org/jdk22/pull/11.diff Fetch: git fetch https://git.openjdk.org/jdk22.git pull/11/head:pull/11 PR: https://git.openjdk.org/jdk22/pull/11
Integrated: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote: > Improve the test by being more lenient to related code cache exhaustion > errors. The important thing is that we don't terminate with a fatal error, > which the new code now checks for explicitly. The check for that is based on > what is done by > `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` . > > The existing `UpcallTestHelper.Output` class that was previously used to > assert on stdout/stderr contents did not have the capability to look for > patterns in the output. So, I've taken the opportunity to replace it with the > more canonical `OutputAnalyzer` which comes from the test library. > > Finally, I've also added back the test for downcall stub allocation failure > which was removed as part of the initial patch because it was too > inconsistent [1]. With the new approach, it should pass reliably as well. > > Testing: `jdk_foreign` suite (which contains all the affected tests) > > [1]: > https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64 This pull request has now been integrated. Changeset: 7ece9e90 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/7ece9e90c0198f92cdf8d620e346c4a9832724cd Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion Reviewed-by: mcimadamore - PR: https://git.openjdk.org/jdk/pull/17056
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Wed, 13 Dec 2023 07:08:53 GMT, David Holmes wrote:
>> Improve the test by being more lenient to related code cache exhaustion
>> errors. The important thing is that we don't terminate with a fatal error,
>> which the new code now checks for explicitly. The check for that is based on
>> what is done by
>> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>>
>> The existing `UpcallTestHelper.Output` class that was previously used to
>> assert on stdout/stderr contents did not have the capability to look for
>> patterns in the output. So, I've taken the opportunity to replace it with
>> the more canonical `OutputAnalyzer` which comes from the test library.
>>
>> Finally, I've also added back the test for downcall stub allocation failure
>> which was removed as part of the initial patch because it was too
>> inconsistent [1]. With the new approach, it should pass reliably as well.
>>
>> Testing: `jdk_foreign` suite (which contains all the affected tests)
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
>
> test/jdk/java/foreign/TestStubAllocFailure.java line 51:
>
>> 49: runInNewProcess(UpcallRunner.class, true,
>> List.of("-XX:ReservedCodeCacheSize=3M"), List.of())
>> 50: .shouldNotHaveExitValue(0)
>> 51: .shouldNotHaveFatalError();
>
> Just curious what non-zero exit value is actually expected here and below?
Any non-zero exit value is acceptable. The intent here is to check that the
process didn't complete normally.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1425357636
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Tue, 12 Dec 2023 12:09:50 GMT, Maurizio Cimadamore
wrote:
>> Improve the test by being more lenient to related code cache exhaustion
>> errors. The important thing is that we don't terminate with a fatal error,
>> which the new code now checks for explicitly. The check for that is based on
>> what is done by
>> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>>
>> The existing `UpcallTestHelper.Output` class that was previously used to
>> assert on stdout/stderr contents did not have the capability to look for
>> patterns in the output. So, I've taken the opportunity to replace it with
>> the more canonical `OutputAnalyzer` which comes from the test library.
>>
>> Finally, I've also added back the test for downcall stub allocation failure
>> which was removed as part of the initial patch because it was too
>> inconsistent [1]. With the now approach, it should pass reliably as well.
>>
>> Testing: `jdk_foreign` suite (which contains all the affected tests)
>>
>> [1]:
>> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
>
> test/lib/jdk/test/lib/process/OutputAnalyzer.java line 870:
>
>> 868: * Assert that we did not crash with a hard VM error (generating an
>> hs_err_pidXXX.log)
>> 869: */
>> 870: public void shouldNotHaveFatalError() {
>
> Looking at the usages, I wonder if this should be `checkFatalError(boolean)`
> and, similarly, for the exit value `checkExitValue(int value)` ?
Are you suggesting to rename the existing methods? I don't see
`checkExistValue(int value)` in `OutputAnalyzer`. The existing method naming
pattern for the assertions is always `should(Not)XXX` though, so that's what I
followed here.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1424066457
Re: RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
On Mon, 11 Dec 2023 13:01:25 GMT, Jorn Vernee wrote:
> Improve the test by being more lenient to related code cache exhaustion
> errors. The important thing is that we don't terminate with a fatal error,
> which the new code now checks for explicitly. The check for that is based on
> what is done by
> `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` .
>
> The existing `UpcallTestHelper.Output` class that was previously used to
> assert on stdout/stderr contents did not have the capability to look for
> patterns in the output. So, I've taken the opportunity to replace it with the
> more canonical `OutputAnalyzer` which comes from the test library.
>
> Finally, I've also added back the test for downcall stub allocation failure
> which was removed as part of the initial patch because it was too
> inconsistent [1]. With the now approach, it should pass reliably as well.
>
> Testing: `jdk_foreign` suite (which contains all the affected tests)
>
> [1]:
> https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64
test/jdk/java/foreign/TestStubAllocFailure.java line 64:
> 62: public static void main(String[] args) throws Throwable {
> 63: FunctionDescriptor descriptor = FunctionDescriptor.ofVoid();
> 64: MethodHandle target =
> MethodHandles.lookup().findStatic(UpcallRunner.class, "target",
> descriptor.toMethodType());
Here I'm extracting the method handle lookup from the loop to make it more
likely that we hit the code path that we are interested in, and don't get
another VirtualMachineError due to this method handle lookup.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/17056#discussion_r1422479110
RFR: 8321400: java/foreign/TestStubAllocFailure.java fails with code cache exhaustion
Improve the test by being more lenient to related code cache exhaustion errors. The important thing is that we don't terminate with a fatal error, which the new code now checks for explicitly. The check for that is based on what is done by `./test/hotspot/jtreg/runtime/ErrorHandling/HsErrFileUtils.java` . The existing `UpcallTestHelper.Output` class that was previously used to assert on stdout/stderr contents did not have the capability to look for patterns in the output. So, I've taken the opportunity to replace it with the more canonical `OutputAnalyzer` which comes from the test library. Finally, I've also added back the test for downcall stub allocation failure which was removed as part of the initial patch because it was too inconsistent [1]. With the now approach, it should pass reliably as well. Testing: `jdk_foreign` suite (which contains all the affected tests) [1]: https://github.com/openjdk/jdk/pull/16311/commits/9a1360598a91871ce6ec48330849c0e4e0279c64 - Commit messages: - use match - Improve stub alloc test Changes: https://git.openjdk.org/jdk/pull/17056/files Webrev: https://webrevs.openjdk.org/?repo=jdk=17056=00 Issue: https://bugs.openjdk.org/browse/JDK-8321400 Stats: 106 lines in 7 files changed: 47 ins; 42 del; 17 mod Patch: https://git.openjdk.org/jdk/pull/17056.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/17056/head:pull/17056 PR: https://git.openjdk.org/jdk/pull/17056
Re: RFR: 8321300: Cleanup TestHFA
On Mon, 4 Dec 2023 21:25:41 GMT, Martin Doerr wrote: > I'd like to clean this up: Use the canonical layouts which are available in > JDK22. Use try-with-resources for `Arena.ofConfined()`. Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16959#pullrequestreview-1763626270
Re: RFR: 8321159: SymbolLookup.libraryLookup(Path, Arena) Assumes default Filesystem [v3]
On Mon, 4 Dec 2023 09:10:07 GMT, Per Minborg wrote:
>> This PR proposes to reject paths provided to the
>> `SymbolLookup.libraryLookup(Path path, Arena arena)` method if a path is not
>> in the default file system.
>
> Per Minborg has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add fail statement
src/java.base/share/classes/java/lang/foreign/SymbolLookup.java line 288:
> 286: * is called from a thread {@code T}, other than the arena's
> owner thread
> 287: * @throws IllegalArgumentException if {@code path} is not in the
> default file system
> 288: * or if {@code path} does not point to a valid library
Please split this into separate `@throws IllegalArgumentException` tags, as
that is what we do everywhere else in the FFM API.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16944#discussion_r1414029801
Re: RFR: 8316641: VarHandle template classes can share code in the base class [v8]
On Mon, 4 Dec 2023 14:49:39 GMT, Jorn Vernee wrote:
>> Chen Liang 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' of https://github.com/openjdk/jdk into
>> cleanup/vh-template-share
>> - 8316641: VarHandle template classes can share code in the base class
>
> src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template
> line 594:
>
>> 592: @ForceInline
>> 593: static int indexRO(ByteBuffer bb, int index) {
>> 594: if (bb.isReadOnly())
>
> I have to think that there was a reason for using unsafe access in the
> current code. So, I'm not sure if it's safe to switch this to a direct
> method/field reference.
@PaulSandoz Do you remember the history here?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15854#discussion_r1414026084
Re: RFR: 8316641: VarHandle template classes can share code in the base class [v8]
On Mon, 6 Nov 2023 08:19:19 GMT, Chen Liang wrote:
>> VarHandle implementations have many static fields and methods that can be
>> pulled to the common superclass to avoid repeated initialization and code
>> duplication.
>>
>> In addition, the Unsafe-based Buffer field access are replaced by usage of
>> public methods or JavaNioAccess.
>
> Chen Liang 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' of https://github.com/openjdk/jdk into
> cleanup/vh-template-share
> - 8316641: VarHandle template classes can share code in the base class
Sorry, this fell off my radar.
src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template
line 594:
> 592: @ForceInline
> 593: static int indexRO(ByteBuffer bb, int index) {
> 594: if (bb.isReadOnly())
I have to think that there was a reason for using unsafe access in the current
code. So, I'm not sure if it's safe to switch this to a direct method/field
reference.
-
PR Review: https://git.openjdk.org/jdk/pull/15854#pullrequestreview-1762626598
PR Review Comment: https://git.openjdk.org/jdk/pull/15854#discussion_r1414012311
Integrated: 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms
On Thu, 30 Nov 2023 20:59:01 GMT, Jorn Vernee wrote: > Need to use `jlong_to_ptr` instead of raw cast. > > I've also fixed another latent issue in `CLayouts` where we were initializing > `C_LONG_LONG` with the `long` layout. This was resulting in a class cast > exception when running the XorTest benchmark. The size of `long` on Linux x86 > 32-bits is 4 bytes, so the returned layout has type `ValueLayout.OfInt` which > we then try to cast to `ValueLayout.OfLong`, resulting in a CCE. This would > also be an issue on Windows. > > Testing: running XorTest benchmark on linux-x86 and windows-x64. This pull request has now been integrated. Changeset: 3b30095a Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/3b30095a63bdf8224a5625507a69942210a203a0 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms Reviewed-by: mcimadamore, tschatzl - PR: https://git.openjdk.org/jdk/pull/16913
RFR: 8321130: Microbenchmarks do not build any more after 8254693 on 32 bit platforms
Need to use `jlong_to_ptr` instead of raw cast. I've also fixed another latent issue in `CLayouts` where we were initializing `C_LONG_LONG` with the `long` layout. This was resulting in a class cast exception when running the XorTest benchmark. The size of `long` on Linux x86 32-bits is 4 bytes, so the returned layout has type `ValueLayout.OfInt` which we then try to cast to `ValueLayout.OfLong`, resulting in a CCE. This would also be an issue on Windows. Testing: running XorTest benchmark on linux-x86 and windows-x64. - Commit messages: - Fix x86 running of XorTest Changes: https://git.openjdk.org/jdk/pull/16913/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16913=00 Issue: https://bugs.openjdk.org/browse/JDK-8321130 Stats: 5 lines in 2 files changed: 2 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/16913.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16913/head:pull/16913 PR: https://git.openjdk.org/jdk/pull/16913
Integrated: 8318586: Explicitly handle upcall stub allocation failure
On Mon, 23 Oct 2023 13:58:27 GMT, Jorn Vernee wrote: > Explicitly handle UpcallStub allocation failures by terminating. We currently > might try to use the returned `nullptr` which would fail sooner or later. > This patch just makes the termination explicit. This pull request has now been integrated. Changeset: e96e1919 Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/e96e19195287a065711358bffde753e9d83e5de3 Stats: 209 lines in 21 files changed: 189 ins; 2 del; 18 mod 8318586: Explicitly handle upcall stub allocation failure 8318653: UpcallTestHelper::runInNewProcess waits for forked process without timeout Reviewed-by: shade, mcimadamore - PR: https://git.openjdk.org/jdk/pull/16311
Re: RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 15:48:11 GMT, Jorn Vernee wrote: > This test is problematic on Zero due to > https://bugs.openjdk.org/browse/JDK-8321064 > > Disable it for now on that platform, until a Zero maintainer has a chance to > look into it. > > Testing: running TestHandshake on zero to see that it is skipped. Thanks for the reviews. I'll integrate this since it's trivial, and to quiet CI. - PR Comment: https://git.openjdk.org/jdk/pull/16906#issuecomment-1834120742
Integrated: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 15:48:11 GMT, Jorn Vernee wrote: > This test is problematic on Zero due to > https://bugs.openjdk.org/browse/JDK-8321064 > > Disable it for now on that platform, until a Zero maintainer has a chance to > look into it. > > Testing: running TestHandshake on zero to see that it is skipped. This pull request has now been integrated. Changeset: 8bedb28b Author:Jorn Vernee URL: https://git.openjdk.org/jdk/commit/8bedb28bbc0698dd0592f8e121ce0a4b7b0ac03d Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod 8321119: Disable java/foreign/TestHandshake.java on Zero VMs Reviewed-by: eosterlund, shade - PR: https://git.openjdk.org/jdk/pull/16906
Re: RFR: 8318586: Explicitly handle upcall stub allocation failure [v7]
> Explicitly handle UpcallStub allocation failures by terminating. We currently > might try to use the returned `nullptr` which would fail sooner or later. > This patch just makes the termination explicit. Jorn Vernee has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 14 commits: - Merge branch 'master' into UpcallStubAllocFailure - use alloc_fail_is_fatal = false on all platforms - add nullptr checks to other platforms - remove unused imports - remove flaky downcall stub oome test - remove @throws - Throw OOME for allocation failures - call fatal without holding code cache lock - Merge branch 'master' into UpcallStubAllocFailure - remove unused import - ... and 4 more: https://git.openjdk.org/jdk/compare/8b102ed6...0a10152b - Changes: https://git.openjdk.org/jdk/pull/16311/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16311=06 Stats: 209 lines in 21 files changed: 189 ins; 2 del; 18 mod Patch: https://git.openjdk.org/jdk/pull/16311.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16311/head:pull/16311 PR: https://git.openjdk.org/jdk/pull/16311
Re: RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
On Thu, 30 Nov 2023 16:14:37 GMT, Aleksey Shipilev wrote: > Hold on, can we figure out if Zero can actually be fixed before we go and > disable the test? I think we only disable the tests like this if there is an > intrinsic reason why the test does not apply to a platform. I would problem list it, but we can't problem list tests specifically on Zero. The issue seems to be in Zero not polling for safepoints enough. See some discussion on: https://bugs.openjdk.org/browse/JDK-8321064 - PR Comment: https://git.openjdk.org/jdk/pull/16906#issuecomment-1834092904
RFR: 8321119: Disable java/foreign/TestHandshake.java on Zero VMs
This test is problematic on Zero due to https://bugs.openjdk.org/browse/JDK-8321064 Disable it for now on that platform, until a Zero maintainer has a chance to look into it. Testing: running TestHandshake on zero to see that it is skipped. - Commit messages: - disable TestHandshake on zero Changes: https://git.openjdk.org/jdk/pull/16906/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16906=00 Issue: https://bugs.openjdk.org/browse/JDK-8321119 Stats: 1 line in 1 file changed: 1 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/16906.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16906/head:pull/16906 PR: https://git.openjdk.org/jdk/pull/16906
Re: RFR: 8310644: Make panama memory segment close use async handshakes [v3]
On Thu, 23 Nov 2023 16:34:23 GMT, Erik Österlund wrote: >> The current logic for closing memory in panama today is susceptible to live >> lock if we have a closing thread that wants to close the memory in a loop >> that keeps failing, and a bunch of accessing threads that want to perform >> accesses as long as the memory is alive. They can both create impediments >> for the other. >> >> By using asynchronous handshakes to install an exception onto threads that >> are in @Scoped memory accesses, we can have close always succeed, and the >> accessing threads bail out. The idea is that we perform a synchronous >> handshake first to find threads that are in scoped methods. They might >> however be in the middle of throwing an exception or something wild like >> there, where an exception can't be delivered. We install an async handshake >> that will roll us forward to the first place where we can indeed install >> exceptions, then we reevaluate if we still need to do that, or if we have >> unwound out from the scoped method. If we are still inside of it, we ensure >> an exception is installed so we don't continue executing bytecodes that >> might access the memory that we have freed. >> >> Tested tier 1-5 as well as running test/jdk/java/foreign/TestHandshake.java >> hundreds of times, which tests this API pretty well. > > Erik Österlund has updated the pull request incrementally with one additional > commit since the last revision: > > Comments from Jorn LGTM - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16792#pullrequestreview-1746784992
Re: RFR: 8310644: Make panama memory segment close use async handshakes
On Thu, 23 Nov 2023 11:14:29 GMT, Erik Österlund wrote:
> The current logic for closing memory in panama today is susceptible to live
> lock if we have a closing thread that wants to close the memory in a loop
> that keeps failing, and a bunch of accessing threads that want to perform
> accesses as long as the memory is alive. They can both create impediments for
> the other.
>
> By using asynchronous handshakes to install an exception onto threads that
> are in @Scoped memory accesses, we can have close always succeed, and the
> accessing threads bail out. The idea is that we perform a synchronous
> handshake first to find threads that are in scoped methods. They might
> however be in the middle of throwing an exception or something wild like
> there, where an exception can't be delivered. We install an async handshake
> that will roll us forward to the first place where we can indeed install
> exceptions, then we reevaluate if we still need to do that, or if we have
> unwound out from the scoped method. If we are still inside of it, we ensure
> an exception is installed so we don't continue executing bytecodes that might
> access the memory that we have freed.
>
> Tested tier 1-5 as well as running test/jdk/java/foreign/TestHandshake.java
> hundreds of times, which tests this API pretty well.
src/hotspot/share/prims/scopedMemoryAccess.cpp line 151:
> 149: ResourceMark rm;
> 150: if (_session != nullptr && last_frame.is_compiled_frame() &&
> last_frame.can_be_deoptimized()) {
> 151: CloseScopedMemoryFindOopClosure cl(_session);
Pre-existing, but this value (and class) is unused since we do an unconditional
deopt. If you feel like it, you could remove the
`CloseScopedMemoryFindOopClosure`. We can get it back from the git history
later when that bug is fixed (https://bugs.openjdk.org/browse/JDK-8290892)
src/java.base/share/classes/jdk/internal/foreign/SharedSession.java line 86:
> 84: throw alreadyAcquired(prevState);
> 85: }
> 86: SCOPED_MEMORY_ACCESS.closeScope(this);
拾
src/java.base/share/classes/jdk/internal/misc/X-ScopedMemoryAccess.java.template
line 87:
> 85:
> 86: public void closeScope(MemorySessionImpl session) {
> 87: closeScope0(session, MemorySessionImpl.ALREADY_CLOSED);
I suggest passing in the `ALREADY_CLOSED` instance as an argument to this
method instead. Then we can avoid making the field in `MemorySessionImpl`
public.
test/jdk/java/foreign/TestHandshake.java line 107:
> 105: if (!failed.get()) {
> 106: // ignore - this means segment was alive, but
> was closed while we were accessing it
> 107: // next isAlive test should fail
If we see the exception, we should be able to test that the scope is not alive
here as well
Suggestion:
// next isAlive test should fail
assertFalse(segment.scope().isAlive());
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403266514
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403255404
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403257610
PR Review Comment: https://git.openjdk.org/jdk/pull/16792#discussion_r1403330411
Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols [v7]
On Tue, 21 Nov 2023 19:30:30 GMT, suchismith1993 wrote: >> The math library in AIX specifically, is a static archive. Doing a -lm wont >> suffice, because when the symbols are looked up using dlsym or accessing >> native code through Java, it will lead to failures. >> Hence we had to come up with a list of symbols to allow math library symbols >> to be accesible. >> Also, there are parts of libc library that are static too, and hence those >> symbols also are present in this list. >> Without this change, the StdLibTest and multiple other tests which make >> native function calls using FFI, fail with NoSuchElementException. >> >> >> 1. Adding required compiler flags. >> 2. Adding required symbols. >> >> >> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799) > > suchismith1993 has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix Typos > - Remove unnecessary includes Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16414#pullrequestreview-1743738561
Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols [v7]
On Tue, 21 Nov 2023 19:30:30 GMT, suchismith1993 wrote: >> The math library in AIX specifically, is a static archive. Doing a -lm wont >> suffice, because when the symbols are looked up using dlsym or accessing >> native code through Java, it will lead to failures. >> Hence we had to come up with a list of symbols to allow math library symbols >> to be accesible. >> Also, there are parts of libc library that are static too, and hence those >> symbols also are present in this list. >> Without this change, the StdLibTest and multiple other tests which make >> native function calls using FFI, fail with NoSuchElementException. >> >> >> 1. Adding required compiler flags. >> 2. Adding required symbols. >> >> >> JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799) > > suchismith1993 has updated the pull request incrementally with two additional > commits since the last revision: > > - Fix Typos > - Remove unnecessary includes Note that on Windows we also have a lookup mechanism on the Java side: https://github.com/openjdk/jdk/blob/2c31ca525b1cd70c3dfcb0463c8c984bdd7c886a/src/java.base/share/classes/jdk/internal/foreign/SystemLookup.java#L159 On Windows we need to load the global array, and then grab functions from the array. Why isn't that needed on AIX? Is `dlsym` able to find the statically linked functions as well? - PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1822205442
Re: RFR: 8320131: Zero build fails on macOS after JDK-8254693
On Wed, 22 Nov 2023 02:06:29 GMT, Hao Sun wrote: > The fix is trivial. We should include the standard C header `stdlib.h` [1], > rather than non-standard one `malloc.h`. > > [1] https://en.cppreference.com/w/c/memory/malloc Thanks. - Marked as reviewed by jvernee (Reviewer). PR Review: https://git.openjdk.org/jdk/pull/16775#pullrequestreview-1743488802
Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols
On Mon, 30 Oct 2023 10:54:48 GMT, suchismith1993 wrote: > The math library in AIX specifically, is a static archive. Doing a -lm wont > suffice, because when the symbols are looked up using dlsym or accessing > native code through Java, it will lead to failures. > Hence we had to come up with a list of symbols to allow math library symbols > to be accesible. > Also, there are parts of libc library that are static too, and hence those > symbols also are present in this list. > Without this change, the StdLibTest and multiple other tests which make > native function calls using FFI, fail with NoSuchElementException. > > > 1. Adding required compiler flags. > 2. Adding required symbols. > > > JBS-ISSUE : [JDK-8317799](https://bugs.openjdk.org/browse/JDK-8317799) In the Windows syslookup.c file we declare a global array of `void*` that we fill with function pointers of the relevant functions [1]. That array itself is exported using __declspec (which I think is not required on AIX?) > The only way to avoid an external list is to actually call the functions in > the lookup file What about taking the address of the functions and storing them in an array? That should work as well I'd think. [1]: https://github.com/openjdk/jdk/blob/master/src/java.base/windows/native/libsyslookup/syslookup.c - PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1817478447
Re: RFR: JDK-8317799 : AIX PPC64: FFI symbol lookup doesn't find symbols
On Fri, 17 Nov 2023 13:02:22 GMT, Martin Doerr wrote: > My understanding is that a lot of symbols can be found out of the box because > they are in dynamically linked libraries. Only some libraries are special on > AIX which don't support dynamic linking and we need this workaround for them. That doesn't sound too dissimilar from the situation on Windows: most of the symbols we can dynamically look up in `ucrtbase.dll`, but some symbols are 'inline', so we create our own library that re-exports these functions through a table of addresses (take a look at `/src/java.base/windows/native/libsyslookup/syslookup.c` Maybe something similar could be done on AIX: i.e. include a fallback library that statically links the missing functions, and then re-exports them through a table or addresses. - PR Comment: https://git.openjdk.org/jdk/pull/16414#issuecomment-1816538275
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Thu, 16 Nov 2023 19:56:51 GMT, Phil Race wrote: > > > > > > layoutCnt=16000 total=193ms <<< app fully displayed > > > > > > vs > > > > > > layoutCnt=16000 total=453ms <<< app fully displayed > > > > > > > > > > > > > > > It looks strange that 16000 calls are not enough to warmup, and the > > > > > difference is so large. > > > > > > > > > > > > I am not a C2 expert, (not even an amateur), I just assume that it > > > > takes a lot of calls to be fully optimized. > > > > > > > > > @JornVernee this looks suspicious and seems unrelated to the cold startup > > > issues we discussed before. > > > > > > I suspect the benchmark might be measuring the java.lang.foreign code > > needing to be loaded as part of the benchmark. While for JNI, the > > initialization of all the JNI machinery is included in the startup of the > > application. Was the running time of the entire application/process > > measured? Or only from the start of the `main` method? > > Yes, that's correct, it includes all the startup costs in that number. So as > @jayathirthrao observed, the comment "16000 calls are not enough to warmup" > may be slightly off the mark since at this time, each 1,000 FFM calls is > already roughly as fast as each 1,000 JNI calls So we ARE warmed up by then, > but I have no idea what would be a normal expectation. Looking at the numbers > above it is roughly around 12,000 that we reach parity for the speed of each > incremental call. C2/fully optimized compilation kicks in after 10 000 calls, and is asynchronous by default (i.e. the rest of the application keeps running). So, 12,000 sounds relatively normal to me. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1815260564
Re: RFR: 8318364: Add an FFM-based implementation of harfbuzz OpenType layout [v4]
On Thu, 16 Nov 2023 01:54:29 GMT, Sergey Bylokhov wrote: > > > > So we have somewhere around a fixed 125ms startup cost for the FFM case > > > > - as measured on my Mac, > > > > but only 35-40ms of that is attributable to the specific needs of > > > > layout. > > > > > > > > > That looks unfortunate. I guess if we will start to use ffm in other > > > places we can easily spend of 1 second budget on startup=( > > > > > > Yes, this case is sufficiently uncommon, that it is OK, and is a decent way > > to help us track improvements to FFM. But it would be another matter to > > have to do it for however many of our core software loops and AWT window > > manager interaction calls we need to get running for a minimal app. > > > > layoutCnt=16000 total=193ms <<< app fully displayed > > > > vs > > > > layoutCnt=16000 total=453ms <<< app fully displayed > > > > > > > > > It looks strange that 16000 calls are not enough to warmup, and the > > > difference is so large. > > > > > > I am not a C2 expert, (not even an amateur), I just assume that it takes a > > lot of calls to be fully optimized. > > @JornVernee this looks suspicious and seems unrelated to the cold startup > issues we discussed before. I suspect the benchmark might be measuring the java.lang.foreign code needing to be loaded as part of the benchmark. While for JNI, the initialization of all the JNI machinery is included in the startup of the application. Was the running time of the entire application/process measured? Or only from the start of the `main` method? Secondly, we have not spent a lot of time optimizing the startup performance of FFM yet. There are things we could do such as pre-generating classes during jlink-time, similar to what we do for java.lang.invoke/lambda implementation classes. - PR Comment: https://git.openjdk.org/jdk/pull/15476#issuecomment-1815189946
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Thu, 16 Nov 2023 06:29:20 GMT, Alan Bateman wrote:
>> See the JBS issue for an extended problem description.
>>
>> This patch changes the specification and implementation of
>> `MethodHandles::byteArrayViewVarHandle`,
>> `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and
>> `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the
>> alignment of Java array elements, in order to bring them in line with the
>> guarantees made by an arbitrary JVM implementation (which makes no
>> guarantees about array element alignment beyond them being aligned to their
>> natural alignment).
>>
>> - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any
>> alignment for the accesses. Which means we can only reliably support plain
>> get and set access modes. The javadoc text explaining which other access
>> modes are supported, or how to compute aligned offsets into the array is
>> dropped, as it is not guaranteed to be correct on all JVM implementations.
>> The implementation of the returned VarHandle is changed to throw an
>> `UnsupportedOperationException` for the unsupported access modes, as
>> mandated by the spec of `VarHandle` [1].
>>
>> - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is
>> incorrect when accessing a heap buffer (wrapping a byte[]), for the same
>> reasons as `byteArrayViewVarHandle`. The spec is changed to specify that
>> when accessing a _heap buffer_, only plain get and set access modes are
>> supported. The implementation of the returned var handle is changed to throw
>> an `IllegalStateException` when an access is attempted on a heap buffer
>> using an access mode other than plain get or set. Note that we don't throw
>> an outright `UnsupportedOperationException` for this case, since whether the
>> access modes are supported depends on the byte buffer instance being used.
>>
>> - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former
>> method depends directly on the latter for all its alignment computations. We
>> change the implementation of the latter method to throw an
>> `UnsupportedOperationException` for all unit sizes greater than 1, when the
>> buffer is non-direct. This change is largely covered by the existing
>> specification:
>>
>>
>> * @throws UnsupportedOperationException
>> * If the native platform does not guarantee stable alignment
>> offset
>> * values for the given unit size when managing the memory
>> regions
>> * of buffers of the same kind as this buffer (direct or
>> * non-direct). For example, if garbage collection would...
>
> src/java.base/share/classes/java/nio/X-Buffer.java.template line 2250:
>
>> 2248: if (unitSize < 1 || (unitSize & (unitSize - 1)) != 0)
>> 2249: throw new IllegalArgumentException("Unit size not a power
>> of two: " + unitSize);
>> 2250: if (unitSize > 1 && !isDirect())
>
> This will require an update to the implNote as it documents 8.
Thanks, good catch
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1395682013
RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
See the JBS issue for an extended problem description. This patch changes the specification and implementation of `MethodHandles::byteArrayViewVarHandle`, `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the alignment of Java array elements, in order to bring them in line with the guarantees made by an arbitrary JVM implementation (which makes no guarantees about array element alignment beyond them being aligned to their natural alignment). - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment for the accesses. Which means we can only reliably support plain get and set access modes. The javadoc text explaining which other access modes are supported, or how to compute aligned offsets into the array is dropped, as it is not guaranteed to be correct on all JVM implementations. The implementation of the returned VarHandle is changed to throw an `UnsupportedOperationException` for the unsupported access modes, as mandated by the spec of `VarHandle` [1]. - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is incorrect when accessing a heap buffer (wrapping a byte[]), for the same reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when accessing a _heap buffer_, only plain get and set access modes are supported. The implementation of the returned var handle is changed to throw an `IllegalStateException` when an access is attempted on a heap buffer using an access mode other than plain get or set. Note that we don't throw an outright `UnsupportedOperationException` for this case, since whether the access modes are supported depends on the byte buffer instance being used. - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former method depends directly on the latter for all its alignment computations. We change the implementation of the latter method to throw an `UnsupportedOperationException` for all unit sizes greater than 1, when the buffer is non-direct. This change is largely covered by the existing specification: * @throws UnsupportedOperationException * If the native platform does not guarantee stable alignment offset * values for the given unit size when managing the memory regions * of buffers of the same kind as this buffer (direct or * non-direct). For example, if garbage collection would result * in the moving of a memory region covered by a non-direct buffer * from one location to another and both locations have different * alignment characteristics. However, the `@implNote` mentions that an `UnsupportedOperationException` will be thrown for unit sizes greater than 8. This is updated to say unit sizes greater than 1. Note that the testing code for these APIs is tightly coupled, so it is practically convenient to address all these issues together. Testing: local `java/lang/invoke/VarHandles`, more TODO [1]: https://github.com/openjdk/jdk/blob/ffa35d8cf181cfbcb54497e997dbd18a9b62b97e/src/java.base/share/classes/java/lang/invoke/VarHandle.java#L189-L191 - Commit messages: - Add api note pointing to alternatives for client that require non-plain access - simplify spec for alignmentOffset and alignedSlice - Merge note about misaligned access in byteBufferViewVarHandle - updated alignedSlice implNote as well - updated alignedOffset implNote - Use ISE for bbvvh instead of UOE - restore some tests for direct buffers - fix BAVV and BBVV impl and tests - regen test files - fix BasicByte test Changes: https://git.openjdk.org/jdk/pull/16681/files Webrev: https://webrevs.openjdk.org/?repo=jdk=16681=00 Issue: https://bugs.openjdk.org/browse/JDK-8318966 Stats: 5133 lines in 19 files changed: 612 ins; 3440 del; 1081 mod Patch: https://git.openjdk.org/jdk/pull/16681.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16681/head:pull/16681 PR: https://git.openjdk.org/jdk/pull/16681
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Thu, 16 Nov 2023 17:31:46 GMT, Paul Sandoz wrote:
>> See the JBS issue for an extended problem description.
>>
>> This patch changes the specification and implementation of
>> `MethodHandles::byteArrayViewVarHandle`,
>> `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and
>> `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the
>> alignment of Java array elements, in order to bring them in line with the
>> guarantees made by an arbitrary JVM implementation (which makes no
>> guarantees about array element alignment beyond them being aligned to their
>> natural alignment).
>>
>> - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any
>> alignment for the accesses. Which means we can only reliably support plain
>> get and set access modes. The javadoc text explaining which other access
>> modes are supported, or how to compute aligned offsets into the array is
>> dropped, as it is not guaranteed to be correct on all JVM implementations.
>> The implementation of the returned VarHandle is changed to throw an
>> `UnsupportedOperationException` for the unsupported access modes, as
>> mandated by the spec of `VarHandle` [1].
>>
>> - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is
>> incorrect when accessing a heap buffer (wrapping a byte[]), for the same
>> reasons as `byteArrayViewVarHandle`. The spec is changed to specify that
>> when accessing a _heap buffer_, only plain get and set access modes are
>> supported. The implementation of the returned var handle is changed to throw
>> an `IllegalStateException` when an access is attempted on a heap buffer
>> using an access mode other than plain get or set. Note that we don't throw
>> an outright `UnsupportedOperationException` for this case, since whether the
>> access modes are supported depends on the byte buffer instance being used.
>>
>> - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former
>> method depends directly on the latter for all its alignment computations. We
>> change the implementation of the latter method to throw an
>> `UnsupportedOperationException` for all unit sizes greater than 1, when the
>> buffer is non-direct. This change is largely covered by the existing
>> specification:
>>
>>
>> * @throws UnsupportedOperationException
>> * If the native platform does not guarantee stable alignment
>> offset
>> * values for the given unit size when managing the memory
>> regions
>> * of buffers of the same kind as this buffer (direct or
>> * non-direct). For example, if garbage collection would...
>
> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4518:
>
>> 4516: * Only plain {@linkplain VarHandle.AccessMode#GET get} and
>> {@linkplain VarHandle.AccessMode#SET set}
>> 4517: * access modes are supported by the returned var handle. For all
>> other access modes, an
>> 4518: * {@link UnsupportedOperationException} will be thrown.
>
> I recommend adding an api note explaining that native memory segments, direct
> byte buffers, or heap memory segments backed by long[] should be used if
> support for other access modes are required.
Good idea. Thanks
> src/java.base/share/classes/java/nio/X-Buffer.java.template line 2218:
>
>> 2216: * @implNote
>> 2217: * This implementation throws {@code
>> UnsupportedOperationException} for
>> 2218: * non-direct buffers when the given unit size is greater then
>> {@code 1}.
>
> This is no longer an implementation note, its now part of the specified API.
> So i think we can simplify the text of the `@throws UOE ...` to just say:
>
> @throws UOE if the buffer is non-direct and the unit size > 1
>
> Same for the other method.
Right. Good idea
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396147281
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396147495
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Thu, 16 Nov 2023 18:52:03 GMT, Maurizio Cimadamore
wrote:
>> src/java.base/share/classes/java/lang/invoke/MethodHandles.java line 4610:
>>
>>> 4608: * {@link Double#doubleToRawLongBits}, respectively).
>>> 4609: *
>>> 4610: * Access to heap byte buffers is always unaligned.
>>
>> I recommend merging that sentence into the paragraph on heap byte buffers
>> e.g.:
>>> For direct buffers, access of the bytes at an index is always misaligned.
>>> As a result only the plain...
>
> But... surely if I'm just accessing bytes the access cannot possibly be
> mis-aligned?
Right. I think there is a difference between 'unaligned' and 'misaligned'.
Unaligned means there is no effort made to align the access (so it _may_ be
misaligned). Misaligned means _definitely not_ aligned. At least, that is my
interpretation.
This is what I wrote in the latest version:
* For heap byte buffers, access is always unaligned. As a result, only the
plain
* {@linkplain VarHandle.AccessMode#GET get}
* and {@linkplain VarHandle.AccessMode#SET set} access modes are supported
by the
* returned var handle. For all other access modes, an {@link
IllegalStateException}
* will be thrown.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396215821
Re: RFR: 8318966: Some methods make promises about Java array element alignment that are too strong
On Wed, 15 Nov 2023 22:46:03 GMT, Jorn Vernee wrote: > See the JBS issue for an extended problem description. > > This patch changes the specification and implementation of > `MethodHandles::byteArrayViewVarHandle`, > `MethodHandles::byteBufferViewVarHandle`, `ByteBuffer::alignedSlice`, and > `ByteBuffer::alignmentOffset` to weaken the guarantees they make about the > alignment of Java array elements, in order to bring them in line with the > guarantees made by an arbitrary JVM implementation (which makes no guarantees > about array element alignment beyond them being aligned to their natural > alignment). > > - `MethodHandles::byteArrayViewVarHandle`: we can not guarantee any alignment > for the accesses. Which means we can only reliably support plain get and set > access modes. The javadoc text explaining which other access modes are > supported, or how to compute aligned offsets into the array is dropped, as it > is not guaranteed to be correct on all JVM implementations. The > implementation of the returned VarHandle is changed to throw an > `UnsupportedOperationException` for the unsupported access modes, as mandated > by the spec of `VarHandle` [1]. > > - `MethodHandles::byteBufferViewVarHandle`: the implementation/spec is > incorrect when accessing a heap buffer (wrapping a byte[]), for the same > reasons as `byteArrayViewVarHandle`. The spec is changed to specify that when > accessing a _heap buffer_, only plain get and set access modes are supported. > The implementation of the returned var handle is changed to throw an > `IllegalStateException` when an access is attempted on a heap buffer using an > access mode other than plain get or set. Note that we don't throw an outright > `UnsupportedOperationException` for this case, since whether the access modes > are supported depends on the byte buffer instance being used. > > - `ByteBuffer::alignedSlice` and `ByteBuffer::alignmentOffset`: The former > method depends directly on the latter for all its alignment computations. We > change the implementation of the latter method to throw an > `UnsupportedOperationException` for all unit sizes greater than 1, when the > buffer is non-direct. This change is largely covered by the existing > specification: > > > * @throws UnsupportedOperationException > * If the native platform does not guarantee stable alignment > offset > * values for the given unit size when managing the memory regions > * of buffers of the same kind as this buffer (direct or > * non-direct). For example, if garbage collection would result > * in the mo... src/java.base/share/classes/java/lang/invoke/X-VarHandleByteArrayView.java.template line 739: > 737: ByteBufferHandle handle = (ByteBufferHandle)ob; > 738: ByteBuffer bb = (ByteBuffer) Objects.requireNonNull(obb); > 739: #if[Object] Note that I removed this 'if Object' block (and the one below), as the public API does not support object access any way: https://github.com/openjdk/jdk/blob/9727f4bdddc071e6f59806087339f345405ab004/src/java.base/share/classes/java/lang/invoke/MethodHandles.java#L4591-L4593 test/jdk/java/nio/Buffer/BasicChar.java line 530: > 528: > 529: > 530: These white space changes come from re-generating the test files using the script included in the repo. I've kept the changes so that the next contributor won't have to remove them again. - PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396208790 PR Review Comment: https://git.openjdk.org/jdk/pull/16681#discussion_r1396210579
Re: RFR: 8319928: Exceptions thrown by cleanup actions should be handled correctly [v5]
On Thu, 16 Nov 2023 18:38:54 GMT, Maurizio Cimadamore wrote: >> This simplePR tweaks the factory which wraps custom cleanup actions passed >> to `MemorySegment::reinterpret`, so that any exception thrown by the custom >> cleanup is swallowed when the arena is closed. >> >> This aligns the behavior of confined/shared session with that of implicit >> session (as implicit sessions are backed by a `Cleaner`, there is no way for >> cleanup exception to bubble up). > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add test to check for self-suppression Marked as reviewed by jvernee (Reviewer). - PR Review: https://git.openjdk.org/jdk/pull/16619#pullrequestreview-1735258511
Re: RFR: 8319928: Exceptions thrown by cleanup actions should be handled correctly [v3]
On Thu, 16 Nov 2023 18:24:44 GMT, Maurizio Cimadamore
wrote:
>> This simplePR tweaks the factory which wraps custom cleanup actions passed
>> to `MemorySegment::reinterpret`, so that any exception thrown by the custom
>> cleanup is swallowed when the arena is closed.
>>
>> This aligns the behavior of confined/shared session with that of implicit
>> session (as implicit sessions are backed by a `Cleaner`, there is no way for
>> cleanup exception to bubble up).
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address CSR comments
src/java.base/share/classes/jdk/internal/foreign/MemorySessionImpl.java line
266:
> 264: pendingException = ex;
> 265: } else if (ex != pendingException) {
> 266: // note: self-suppression is not supported
Does the current test cover the `ex == pendingException` case?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16619#discussion_r1396165147
