Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Wed, 20 Apr 2022 00:25:26 GMT, Paul Sandoz wrote: > I am yet to be convinced we a 3rd vector right shift operator. We are talking > about narrow cases of correct use which i believe can be supported with the > existing operators. The user needs to think very careful when deviating from > common right shift patterns on sub-words, which when deviated from can often > imply misunderstanding and incorrect use, or an obtuse use. I would prefer to > stick the existing operators and clarify their use. As we can see, `VectorOperators` has directly supported all the unary/binary scalar operators in the Java language except for `>>>`. So it seems strange not to support `>>>` directly. Since you are a Vector API expert, you know the semantics of `LSHR` precisely. But for many Java developers, things are different. I'm afraid most of them don't know Vector API actually has extended semantics of `>>>` upon bytes/shorts with `LSHR`. To be honest, I didn't know it before my customer's bug even though I had spent enough time reading the Vector API doc. This is because for ordinary developers, they are only familiar with the common scalar `>>>`. So it seems easy to write bugs with the only `LSHR`, which is different with `>>>`. >From the developer's point of view, I strongly suggest providing the `>>>` >operator in Vector API. Not only because `>>>` is one of the basic operators in Java language, but also we can make it to be more friendly to so many ordinary developers. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature
On Sat, 9 Apr 2022 00:10:40 GMT, Sandhya Viswanathan
wrote:
>> Currently the vector load with mask when the given index happens out of the
>> array boundary is implemented with pure java scalar code to avoid the IOOBE
>> (IndexOutOfBoundaryException). This is necessary for architectures that do
>> not support the predicate feature. Because the masked load is implemented
>> with a full vector load and a vector blend applied on it. And a full vector
>> load will definitely cause the IOOBE which is not valid. However, for
>> architectures that support the predicate feature like SVE/AVX-512/RVV, it
>> can be vectorized with the predicated load instruction as long as the
>> indexes of the masked lanes are within the bounds of the array. For these
>> architectures, loading with unmasked lanes does not raise exception.
>>
>> This patch adds the vectorization support for the masked load with IOOBE
>> part. Please see the original java implementation (FIXME: optimize):
>>
>>
>> @ForceInline
>> public static
>> ByteVector fromArray(VectorSpecies species,
>>byte[] a, int offset,
>>VectorMask m) {
>> ByteSpecies vsp = (ByteSpecies) species;
>> if (offset >= 0 && offset <= (a.length - species.length())) {
>> return vsp.dummyVector().fromArray0(a, offset, m);
>> }
>>
>> // FIXME: optimize
>> checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
>> return vsp.vOp(m, i -> a[offset + i]);
>> }
>>
>> Since it can only be vectorized with the predicate load, the hotspot must
>> check whether the current backend supports it and falls back to the java
>> scalar version if not. This is different from the normal masked vector load
>> that the compiler will generate a full vector load and a vector blend if the
>> predicate load is not supported. So to let the compiler make the expected
>> action, an additional flag (i.e. `usePred`) is added to the existing
>> "loadMasked" intrinsic, with the value "true" for the IOOBE part while
>> "false" for the normal load. And the compiler will fail to intrinsify if the
>> flag is "true" and the predicate load is not supported by the backend, which
>> means that normal java path will be executed.
>>
>> Also adds the same vectorization support for masked:
>> - fromByteArray/fromByteBuffer
>> - fromBooleanArray
>> - fromCharArray
>>
>> The performance for the new added benchmarks improve about `1.88x ~ 30.26x`
>> on the x86 AVX-512 system:
>>
>> Benchmark before After Units
>> LoadMaskedIOOBEBenchmark.byteLoadArrayMaskIOOBE 737.542 1387.069 ops/ms
>> LoadMaskedIOOBEBenchmark.doubleLoadArrayMaskIOOBE 118.366 330.776 ops/ms
>> LoadMaskedIOOBEBenchmark.floatLoadArrayMaskIOOBE 233.832 6125.026 ops/ms
>> LoadMaskedIOOBEBenchmark.intLoadArrayMaskIOOBE233.816 7075.923 ops/ms
>> LoadMaskedIOOBEBenchmark.longLoadArrayMaskIOOBE 119.771 330.587 ops/ms
>> LoadMaskedIOOBEBenchmark.shortLoadArrayMaskIOOBE 431.961 939.301 ops/ms
>>
>> Similar performance gain can also be observed on 512-bit SVE system.
>
> src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java
> line 2861:
>
>> 2859: ByteSpecies vsp = (ByteSpecies) species;
>> 2860: if (offset >= 0 && offset <= (a.length -
>> species.vectorByteSize())) {
>> 2861: return vsp.dummyVector().fromByteArray0(a, offset, m, /*
>> usePred */ false).maybeSwap(bo);
>
> Instead of usePred a term like inRange or offetInRage or offsetInVectorRange
> would be easier to follow.
Thanks for the review. I will change it later.
-
PR: https://git.openjdk.java.net/jdk/pull/8035
Re: RFR: 8283667: [vectorapi] Vectorization for masked load with IOOBE with predicate feature
On Mon, 11 Apr 2022 09:04:36 GMT, Jatin Bhateja wrote:
>> The optimization for masked store is recorded to:
>> https://bugs.openjdk.java.net/browse/JDK-8284050
>
>> The blend should be with the intended-to-store vector, so that masked lanes
>> contain the need-to-store elements and unmasked lanes contain the loaded
>> elements, which would be stored back, which results in unchanged values.
>
> It may not work if memory is beyond legal accessible address space of the
> process, a corner case could be a page boundary. Thus re-composing the
> intermediated vector which partially contains actual updates but effectively
> perform full vector write to destination address may not work in all
> scenarios.
Thanks for the comment! So how about adding the check for the valid array range
like the masked vector load?
Codes like:
public final
void intoArray(byte[] a, int offset,
VectorMask m) {
if (m.allTrue()) {
intoArray(a, offset);
} else {
ByteSpecies vsp = vspecies();
if (offset >= 0 && offset <= (a.length - vsp.length())) { // a
full range check
intoArray0(a, offset, m, /* usePred */ false);
// can be vectorized by load+blend_store
} else {
checkMaskFromIndexSize(offset, vsp, m, 1, a.length);
intoArray0(a, offset, m, /* usePred */ true);
// only be vectorized by the predicated store
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/8035
RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner
Please review this change to replace the finalizer in `AbstractLdapNamingEnumeration` with a Cleaner. The pieces of state required for cleanup (`LdapCtx homeCtx`, `LdapResult res`, and `LdapClient enumClnt`) are moved to a static inner class . From there, the change is fairly mechanical. Details of note: 1. Some operations need to change the state values (the update() method is probably the most interesting). 2. Subclasses need to access `homeCtx`; I added a `homeCtx()` method to read `homeCtx` from the superclass's `state`. The test case is based on a copy of `com/sun/jndi/ldap/blits/AddTests/AddNewEntry.java`. A more minimal test case might be possible, but this was done for expediency. The test only confirms that the new Cleaner use does not keep the object reachable. It only tests `LdapSearchEnumeration` (not `LdapNamingEnumeration` or `LdapBindingEnumeration`, though all are subclasses of `AbstractLdapNamingEnumeration`). Thanks. - Commit messages: - fix whitespace - Merge branch 'master' into remove-finalizers - Test changes to test new cleaner code - Rename test to LdapEnumeration - Create copy of AddNewEntry test to test AbstractLdapNamingEnumeration Cleaner - Merge branch 'master' into remove-finalizers - Replace AbstractLdapNamingEnumeration finalizer with Cleaner Changes: https://git.openjdk.java.net/jdk/pull/8311/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8311=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8283660 Stats: 259 lines in 7 files changed: 192 ins; 20 del; 47 mod Patch: https://git.openjdk.java.net/jdk/pull/8311.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8311/head:pull/8311 PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Wed, 20 Apr 2022 00:46:32 GMT, Paul Sandoz wrote:
> The intended pattern for the operator tokens is to present a short symbolic
> description using Java operators and common methods. It would be good to try
> and keep with this pattern, and clarify for the extra cases. Here's what i
> had in mind:
>
> ```
> /** Produce {@code a>>>(n&(ESIZE*8-1))}. Integral only.
> *
> * For operand types {@code byte} and {@code short} the operation behaves as
> if the operand is first implicitly widened
> * to an {@code int} value with {@code (a & ((1 << ESIZE) - 1))} the result
> of which is then applied as the operand to this
> * operation, the result of the operation is then narrowed from {@code int}
> to the operand type using an explicit cast.
> */
> public static final /*bitwise*/ Binary LSHR;
> ```
This works only if people would like to read the detailed description of `LSHR`
carefully.
Actually, most developers would still see the brief description first.
https://user-images.githubusercontent.com/19923746/164127620-90a73d29-868e-46d8-9562-2c5b2021f13b.png;>
If they don't click out the detailed description further or don't read it
carefully, it's still misleading.
Maybe, we'd better not to use `>>>` in the brief description since `LSHR`
behaves differently with the common used `>>>`.
What do you think?
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v5]
> Created a test called NullCallerGetResource to test
> Module::getResourceAsStream and Class::getResourceAsStream from the native
> level.
>
> At the java level the test builds a test module called 'n' which opens the
> package 'open' to everyone. There is also a package 'closed' which is neither
> opened or exported. Both packages have a text file called 'test.txt'. The
> open package has a class called OpenResources and the closed package has a
> class called ClosedResources. The native test is launched with the test
> module n added.
>
> At the native level the test tries to read both the open and closed resource
> from both the classes and the module. It performs the equivalent of the
> following java operations:
>
> Class c = open.OpenResources.fetchClass();
> InputStream in = c.getResourceAsStream("test.txt");
> assert(in != null); in.close();
>
> Module n = c.getModule();
> in = n.getResourceAsStream("open/test.txt");
> assert(in != null); in.close();
>
> Class closed = closed.ClosedResources.fetchClass();
> assert(closedsStream("test.txt") == null);
> assert(n.getResourceAsStream("closed/test.txt") == null);
>
> The test initially threw an NPE when trying to fetch the open resource. The
> Module class was fixed by removing the fragment with the (caller == null)
> test in getResourceAsStream, and changing the call to isOpen(String,Module)
> to use EVERYONE_MODULE if the caller module is null.
Tim Prinzing has updated the pull request incrementally with one additional
commit since the last revision:
some cleanup of the test
-
Changes:
- all: https://git.openjdk.java.net/jdk/pull/8134/files
- new: https://git.openjdk.java.net/jdk/pull/8134/files/c5fef46b..15de2394
Webrevs:
- full: https://webrevs.openjdk.java.net/?repo=jdk=8134=04
- incr: https://webrevs.openjdk.java.net/?repo=jdk=8134=03-04
Stats: 62 lines in 4 files changed: 21 ins; 27 del; 14 mod
Patch: https://git.openjdk.java.net/jdk/pull/8134.diff
Fetch: git fetch https://git.openjdk.java.net/jdk pull/8134/head:pull/8134
PR: https://git.openjdk.java.net/jdk/pull/8134
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 08:41:50 GMT, Jie Fu wrote:
> Hi all,
>
> The Current Vector API doc for `LSHR` is
>
> Produce a>>>(n&(ESIZE*8-1)). Integral only.
>
>
> This is misleading which may lead to bugs for Java developers.
> This is because for negative byte/short elements, the results computed by
> `LSHR` will be different from that of `>>>`.
> For more details, please see
> https://github.com/openjdk/jdk/pull/8276#issue-1206391831 .
>
> After the patch, the doc for `LSHR` is
>
> Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only.
>
>
> Thanks.
> Best regards,
> Jie
The intended pattern for the operator tokens is to present a short symbolic
description using Java operators and common methods. It would be good to try
and keep with this pattern, and clarify for the extra cases. Here's what i had
in mind:
/** Produce {@code a>>>(n&(ESIZE*8-1))}. Integral only.
*
* For operand types {@code byte} and {@code short} the operation behaves
as if the operand is first implicitly widened
* to an {@code int} value with {@code (a & ((1 << ESIZE) - 1))} the
result of which is then applied as the operand to this
* operation, the result of the operation is then narrowed from {@code
int} to the operand type using an explicit cast.
*/
public static final /*bitwise*/ Binary LSHR;
-
PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
I am yet to be convinced we a 3rd vector right shift operator. We are talking
about narrow cases of correct use which i believe can be supported with the
existing operators. The user needs to think very careful when deviating from
common right shift patterns on sub-words, which when deviated from can often
imply misunderstanding and incorrect use, or an obtuse use. I would prefer to
stick the existing operators and clarify their use.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 16:11:37 GMT, Paul Sandoz wrote: > I need to think a little more about this. The specification is not accurate > and likely requires a CSR. > > My initial thoughts are I would prefer the operation to retain reference to > the succinct definition using the logical right shift operator but we add > additional specification explaining the cases for `byte` and `short`, namely > that the result is widened to an `int` as if by `(a & ((1 << ESIZE) - 1))` > and the result narrowed. That's commonly (at least for the widening part) how > `>>>` is used with `byte` and `short`, and i think would be clearer to be > explicit in that regard. OK. To avoid confusing I would prefer using the description of `>>>` in the new vector operator, which would perform the same behavior as the scalar `>>>`. - PR: https://git.openjdk.java.net/jdk/pull/8291
RFR: JDK-8280594: Refactor annotation invocation handler handling to use Objects.toIdentityString
Simple refactoring to use new-in19 library code. - Commit messages: - JDK-8280594: Refactor annotation invocation handler handling to use Objects.toIdentityString Changes: https://git.openjdk.java.net/jdk/pull/8310/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8310=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8280594 Stats: 10 lines in 1 file changed: 0 ins; 9 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8310.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8310/head:pull/8310 PR: https://git.openjdk.java.net/jdk/pull/8310
RFR: 8284930: Re-examine FilterInputStream mark/reset
Remove the `synchronized` keyword from the `mark(int)` and `reset()` methods of `java.io.FilterInputStream`. - Commit messages: - 8284930: Re-examine FilterInputStream mark/reset Changes: https://git.openjdk.java.net/jdk/pull/8309/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8309=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284930 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8309.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8309/head:pull/8309 PR: https://git.openjdk.java.net/jdk/pull/8309
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Tue, 19 Apr 2022 17:40:07 GMT, Paul Sandoz wrote: > Not yet talked with John, but i investigated further. The implementation of > the `LSHR` operation is behaving as intended, but is under specified with > regards to `byte` and `short` as you noted in #8291. > > This is a subtle area, but i am wondering if the user really means to use > arithmetic shift in this case? Since is not the following true for all values > of `e` and `c`, where `e` is a `byte` and `c` is the right shift count > ranging from 0 to 7: > > ``` > (byte) (e >>> c) == (byte) (e >> c) > ``` > > ? > > Then the user can use `VectorOperators.ASHR`. Yes, in theory, the user can use `ASHR`. But people have to be very careful about when to use `AHSR` and when to use `LSHR`, which is really inconvenient and easy to make a mistake. And not all the people are smart enough to know this skill for bytes/shorts. So to make it to be programmed more easily and also reduce the possibility to make mistakes, a new operator for scalar `>>>` would be helpful when vectorizing with Vector API. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Fri, 15 Apr 2022 18:56:37 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > change to liach operation. src/java.base/share/classes/java/io/InputStream.java line 62: > 60: > 61: /** Skip buffer, null until allocated */ > 62: private byte[] skipBuffer = null; This can be removed too as you have the soft reference already. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 18:04:03 GMT, XenoAmess wrote: > no, we use other way, but yes for we take care of thread safety. Fair enough. > Don't think it necessary... I think making it cannot touched by other object > (with security manager on) is enough. I was thinking more for heapdumps due to extending the life of the skip buffer in this patch. At least we don't have to waste more cycles. Oh I forgot last time but, it looks like MIN_SKIP_BUFFER_SIZE is not used. Unless I'm missing something I assume that is a bug. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8178355: IdentityHashMap uses identity-based comparison for values everywhere except remove(K,V) and replace(K,V,V)
On Fri, 15 Apr 2022 05:58:32 GMT, liach wrote: > Explicitly implement `remove` and `replace` in `IdentityHashMap` to compare > values by identity. Updated API documentation of these two methods > ([Preview](https://cr.openjdk.java.net/~liach/8178355/IdentityHashMap.html#remove(java.lang.Object,java.lang.Object))) > to mention such behavior. @stuart-marks Mind peek at this given you are the assignee of the original JBS issue? - PR: https://git.openjdk.java.net/jdk/pull/8259
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]
On Tue, 19 Apr 2022 20:15:08 GMT, XenoAmess wrote: >> as title. > > XenoAmess has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains two new > commits since the last revision: > > - migrate HashSet usages > - migrate LinkedHashSet usage Need to add apiNote documentation section to capacity-based constructors like for maps. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
On Thu, 14 Apr 2022 05:57:54 GMT, Daniel Jeliński wrote: > The benchmark results are quite unexpected. Would we benefit from reducing > the buffer size even further? I tested with smaller and smaller buffer sizes until the performance started to be affected which was about 64 KB. I have not changed this value in the patch just yet. - PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
On Thu, 14 Apr 2022 06:23:24 GMT, Alan Bateman wrote:
>> Modify native multi-byte read-write code used by the `java.io` classes to
>> limit the size of the allocated native buffer thereby decreasing off-heap
>> memory footprint and increasing throughput.
>
> src/java.base/share/native/libjava/io_util.c line 133:
>
>> 131: if (nread == 0)
>> 132: nread = -1;
>> 133: break;
>
> Can you prototype doing the loop in Java rather than in native code so that
> there is less native code to maintain?
I prototyped doing the read loop in Java and the performance seemed to be about
the same. The problem is that the loop needs to exit when the native `read()`
function performs a short read, i.e., fewer bytes are read than were requested.
Without this, at least one regression test fails. The reason is not completely
clear.
To detect such a short read in the Java layer would require some way for the
native layer to notify the Java layer. One potential such method is
boolean readBytes(byte[] b, int off, int len, int[] nread) {}
where the return value indicates whether all or only some of the `len` bytes
were read. If not all were read, then `nread[0]` would contain the number
actually read or `-1`.
Another possibility is
int readBytes(byte[] b, int off, int len, int maxBufferSize) {}
which is identical to the current `readBytes()` except that the maximum
transfer buffer size is specified as a method parameter instead of being
defined by a symbolic constant in the native layer. In this case a short read
would be detected if `len >= maxBufferSize` and the returned value is less than
`maxBufferSize`.
As for the read loop being in native code, note that the write loop is also
already in native code, so if the read loop were moved to Java, then probably
the write loop should be as well.
One advantage of the loops being in native code is that there is only one
`malloc()` per Java `read(byte[],int,int)` or `write(byte[],int,int)`
invocation.
-
PR: https://git.openjdk.java.net/jdk/pull/8235
Integrated: 8284880: Re-examine sun.invoke.util.Wrapper hash tables
On Thu, 14 Apr 2022 11:19:04 GMT, Claes Redestad wrote: > This patch examines and optimizes `Wrapper` lookups. > > First wrote a few simple microbenchmarks to verify there are actual speedups > from using perfect hash tables in `sun.invoke.util.Wrapper` compared to > simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ a > speed-up for the case of `char` -> `Wrapper`, but not when mapping from > `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case > didn't use the `FROM_CHAR` table for some reason, which is remedied. > > Micros show benefits across the board for warmed up case: > > > Baseline, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op > Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op > Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op > Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op > > Patch, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op > Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op > Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op > Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op > > > For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when > spinning up of MHs) there are decent or even great wins in all cases but > `forPrimitiveType` - which was changed from a simple switch to use the hash > lookup. Since the interpreter penalty is small in absolute terms and the win > on JITed code is significant this seems like a reasonable trade-off: > > > Baseline, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op > Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op > Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op > Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op > > Patch, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op > Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op > Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op > Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op This pull request has now been integrated. Changeset: 5df8bd6b Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/5df8bd6b4e15686aa7d72b3f5a977eb51b0befc3 Stats: 259 lines in 3 files changed: 158 ins; 59 del; 42 mod 8284880: Re-examine sun.invoke.util.Wrapper hash tables Reviewed-by: erikj, mchung - PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v3]
> This patch examines and optimizes `Wrapper` lookups. > > First wrote a few simple microbenchmarks to verify there are actual speedups > from using perfect hash tables in `sun.invoke.util.Wrapper` compared to > simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ a > speed-up for the case of `char` -> `Wrapper`, but not when mapping from > `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case > didn't use the `FROM_CHAR` table for some reason, which is remedied. > > Micros show benefits across the board for warmed up case: > > > Baseline, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op > Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op > Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op > Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op > > Patch, OOTB > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op > Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op > Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op > Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op > > > For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when > spinning up of MHs) there are decent or even great wins in all cases but > `forPrimitiveType` - which was changed from a simple switch to use the hash > lookup. Since the interpreter penalty is small in absolute terms and the win > on JITed code is significant this seems like a reasonable trade-off: > > > Baseline, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op > Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op > Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op > Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op > > Patch, -Xint > Benchmark Mode Cnt Score Error Units > Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op > Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op > Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op > Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op Claes Redestad has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'wrappers' of https://github.com/cl4es/jdk into wrappers - Copyrights - Changes: - all: https://git.openjdk.java.net/jdk/pull/8242/files - new: https://git.openjdk.java.net/jdk/pull/8242/files/97eec9d3..792c37e2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8242=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8242=01-02 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8242.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8242/head:pull/8242 PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]
On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad wrote: >> This patch examines and optimizes `Wrapper` lookups. >> >> First wrote a few simple microbenchmarks to verify there are actual speedups >> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to >> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ >> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from >> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case >> didn't use the `FROM_CHAR` table for some reason, which is remedied. >> >> Micros show benefits across the board for warmed up case: >> >> >> Baseline, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op >> Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op >> Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op >> Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op >> >> Patch, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op >> Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op >> Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op >> Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op >> >> >> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when >> spinning up of MHs) there are decent or even great wins in all cases but >> `forPrimitiveType` - which was changed from a simple switch to use the hash >> lookup. Since the interpreter penalty is small in absolute terms and the win >> on JITed code is significant this seems like a reasonable trade-off: >> >> >> Baseline, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op >> Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op >> Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op >> Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op >> >> Patch, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op >> Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op >> Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op >> Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add line break in make/test/BuildMicrobenchmark.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/8242
Integrated: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 08:50:09 GMT, Claes Redestad wrote: > There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. This pull request has now been integrated. Changeset: e307bc86 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/e307bc8694462568807021191f9653ee80a93ed1 Stats: 95 lines in 3 files changed: 88 ins; 3 del; 4 mod 8285001: Simplify StringLatin1.regionMatches Reviewed-by: rriggs, naoto - PR: https://git.openjdk.java.net/jdk/pull/8292
Re: RFR: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 08:50:09 GMT, Claes Redestad wrote: > There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. Thanks for reviews! - PR: https://git.openjdk.java.net/jdk/pull/8292
Re: RFR: 8285001: Simplify StringLatin1.regionMatches [v2]
> There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Copyright - Changes: - all: https://git.openjdk.java.net/jdk/pull/8292/files - new: https://git.openjdk.java.net/jdk/pull/8292/files/a8894f79..434a0fd2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8292=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8292=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8292/head:pull/8292 PR: https://git.openjdk.java.net/jdk/pull/8292
Integrated: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE
On Tue, 19 Apr 2022 13:19:31 GMT, Claes Redestad wrote: > Trivially fix the resolution of the `NF_UNSAFE` named function to use the > appropriate lookup mode. > > In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed > from regular reflection to use a `NamedFunction` for this field, but it > appears the lookup mode came out wrong. This mismatch appears to be benign > and ignored by HotSpot, but a user implementing an experimental JVM ran into > some issues (and additionally noticed the resolve throws the wrong > exception). > > This patch is a point fix to this particular issue, but I think we should > consider following up with a stronger assertion or test for proper staticness > of fields somewhere when resolving fields via > `MemberName.getFactory().resolveOrNull(..)`. This pull request has now been integrated. Changeset: 5d1ec54d Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/5d1ec54d6c20dfe67a459c9d102cdfa0394bcc1e Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE Reviewed-by: psandoz, mchung - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE [v2]
On Tue, 19 Apr 2022 17:33:04 GMT, Claes Redestad wrote: >> Trivially fix the resolution of the `NF_UNSAFE` named function to use the >> appropriate lookup mode. >> >> In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we >> changed from regular reflection to use a `NamedFunction` for this field, but >> it appears the lookup mode came out wrong. This mismatch appears to be >> benign and ignored by HotSpot, but a user implementing an experimental JVM >> ran into some issues (and additionally noticed the resolve throws the wrong >> exception). >> >> This patch is a point fix to this particular issue, but I think we should >> consider following up with a stronger assertion or test for proper >> staticness of fields somewhere when resolving fields via >> `MemberName.getFactory().resolveOrNull(..)`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Missed a spot! Thanks for reviewing! - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]
> as title. XenoAmess has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains two new commits since the last revision: - migrate HashSet usages - migrate LinkedHashSet usage - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/e052c34f..f051c1fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=02-03 Stats: 3 lines in 2 files changed: 0 ins; 1 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Integrated: 8284893: Fix typos in java.base
On Thu, 14 Apr 2022 19:07:09 GMT, Magnus Ihse Bursie wrote: > I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. This pull request has now been integrated. Changeset: fb469fb8 Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/fb469fb894ed84686f9fec5787ac99eb535fdd18 Stats: 369 lines in 162 files changed: 0 ins; 0 del; 369 mod 8284893: Fix typos in java.base Reviewed-by: iris, wetmore, lancea, mullan, naoto - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8282120: optimal capacity tests and test library need to be cleaned up
On Tue, 19 Apr 2022 18:12:04 GMT, Stuart Marks wrote: > The test `java/lang/Enum/ConstantDirectoryOptimalCapacity.java` was > problem-listed by > [JDK-8281631](https://bugs.openjdk.java.net/browse/JDK-8281631) and > essentially superseded by > [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). Given this, > the test should simply be removed. This test is the only test that depends on > a test library utility `test/lib/jdk/test/lib/util/OptimalCapacity.java` > which has a number of issues of its own, so it should simply be removed as > well. > > See the bug report, in particular the comment > > https://bugs.openjdk.java.net/browse/JDK-8282120?focusedCommentId=14489450=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14489450 > > for detailed discussion. Good cleanup! - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8303
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v3]
> as title. XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - migrate HashSet usages - migrate LinkedHashSet usage - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/b139366c..e052c34f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=01-02 Stats: 67 lines in 43 files changed: 1 ins; 9 del; 57 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by mullan (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v9]
On Tue, 19 Apr 2022 17:43:21 GMT, Naoto Sato wrote: >> Supporting `IsoFields` temporal fields in chronologies that are similar to >> ISO chronology. Corresponding CSR has also been drafted. > > Naoto Sato has updated the pull request incrementally with one additional > commit since the last revision: > > Addressing review comments Looks good, thanks - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Tue, 19 Apr 2022 18:04:03 GMT, XenoAmess wrote: > Reader uses a lock object and it appears that InputStream locks on this (per > make/reset) I would assume now that you have some object member fields you > need to hold some lock while accessing those. Even though single thread > access would be the expected case. Locks are expensive. We just use buffers that may or may not be shared by different threads (for the java memory model), because locking objects is too costly performance-wise. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v5]
> This is a (preliminary) patch for javac implementation for the third preview > of pattern matching for switch (type patterns in switches). > > Draft JLS: > http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html > > The changes are: > -there are no guarded patterns anymore, guards are not bound to the > CaseElement (JLS 15.28) > -a new contextual keyword `when` is used to add a guard, instead of `&&` > -`null` selector value is handled on switch level (if a switch has `case > null`, it is used, otherwise a NPE is thrown), rather than on pattern > matching level. > -total patterns are allowed in `instanceof` > -`java.lang.MatchException` is added for the case where a switch is > exhaustive (due to sealed types) at compile-time, but not at runtime. > > Feedback is welcome! > > Thanks! Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Cleanup - more total -> unconditional pattern renaming. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8182/files - new: https://git.openjdk.java.net/jdk/pull/8182/files/311d68a6..dc001541 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8182=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8182=03-04 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8182.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8182/head:pull/8182 PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v2]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: fix LinkedHashSet constructor - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/436273ed..b139366c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8302=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8302=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
RFR: 8282120: optimal capacity tests and test library need to be cleaned up
The test `java/lang/Enum/ConstantDirectoryOptimalCapacity.java` was problem-listed by [JDK-8281631](https://bugs.openjdk.java.net/browse/JDK-8281631) and essentially superseded by [JDK-8186958](https://bugs.openjdk.java.net/browse/JDK-8186958). Given this, the test should simply be removed. This test is the only test that depends on a test library utility `test/lib/jdk/test/lib/util/OptimalCapacity.java` which has a number of issues of its own, so it should simply be removed as well. See the bug report, in particular the comment https://bugs.openjdk.java.net/browse/JDK-8282120?focusedCommentId=14489450=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14489450 for detailed discussion. - Commit messages: - Remove test Enum/ConstantDirectoryOptimalCapacity, utility, and problem list entry. Changes: https://git.openjdk.java.net/jdk/pull/8303/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8303=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8282120 Stats: 355 lines in 3 files changed: 0 ins; 355 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8303.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8303/head:pull/8303 PR: https://git.openjdk.java.net/jdk/pull/8303
RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet
as title. - Commit messages: - 8284780: Need methods to create pre-sized HashSet and LinkedHashSet Changes: https://git.openjdk.java.net/jdk/pull/8302/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8302=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284780 Stats: 34 lines in 2 files changed: 34 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284638: store skip buffers in InputStream Object [v7]
On Mon, 18 Apr 2022 20:08:23 GMT, jmehrens wrote: > > @jmehrens what about this then? I think it safe now(actually this mechanism > > is learned from Reader) > > Reader uses a lock object and it appears that InputStream locks on this (per > make/reset) I would assume now that you have some object member fields you > need to hold some lock while accessing those. Even though single thread > access would be the expected case. no, we use other way, but yes for we take care of thread safety. > Not related but, it looks like the static buffer issue has come up a few > times. Maybe time to add a test to: > https://github.com/openjdk/jdk/blob/master/test/jdk/java/io/InputStream/Skip.java > that would fail if the skip buffer is static? I think it is worthy to add some comments, but a test for it sound a little bit extreme. > Not really the primary issue but, it seems questionable to me that we don't > have to fill the skip buffer with zeros after the last read. That way any > sensitive information that was skipped would also be forgotten. Matching with > Reader seems more important for now. Don't think it necessary... I think making it cannot touched by other object (with security manager on) is enough. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: JDK-8281006 Module::getResourceAsStream should check if the resource is open unconditionally when caller is null [v4]
On Mon, 11 Apr 2022 00:48:34 GMT, Tim Prinzing wrote:
>> Created a test called NullCallerGetResource to test
>> Module::getResourceAsStream and Class::getResourceAsStream from the native
>> level.
>>
>> At the java level the test builds a test module called 'n' which opens the
>> package 'open' to everyone. There is also a package 'closed' which is
>> neither opened or exported. Both packages have a text file called
>> 'test.txt'. The open package has a class called OpenResources and the closed
>> package has a class called ClosedResources. The native test is launched with
>> the test module n added.
>>
>> At the native level the test tries to read both the open and closed resource
>> from both the classes and the module. It performs the equivalent of the
>> following java operations:
>>
>> Class c = open.OpenResources.fetchClass();
>> InputStream in = c.getResourceAsStream("test.txt");
>> assert(in != null); in.close();
>>
>> Module n = c.getModule();
>> in = n.getResourceAsStream("open/test.txt");
>> assert(in != null); in.close();
>>
>> Class closed = closed.ClosedResources.fetchClass();
>> assert(closedsStream("test.txt") == null);
>> assert(n.getResourceAsStream("closed/test.txt") == null);
>>
>> The test initially threw an NPE when trying to fetch the open resource. The
>> Module class was fixed by removing the fragment with the (caller == null)
>> test in getResourceAsStream, and changing the call to isOpen(String,Module)
>> to use EVERYONE_MODULE if the caller module is null.
>
> Tim Prinzing has updated the pull request incrementally with one additional
> commit since the last revision:
>
> requested changes
Marked as reviewed by mchung (Reviewer).
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 95:
> 93: jclass class_OpenResources = (*env)->FindClass(env,
> "open/OpenResources");
> 94: assert(class_OpenResources != NULL);
> 95: jmethodID mid_OpenResources_fetchClass =
> (*env)->GetStaticMethodID(env, class_OpenResources, "fetchClass",
> "()Ljava/lang/Class;" );
It seems that invoking `fetchClass` isn't necessary since you can simply use
`class_OpenResources`. Similarly for `class_ClosedResources`
test/jdk/java/lang/module/exeNullCallerGetResource/exeNullCallerGetResource.c
line 183:
> 181: exit(-1);
> 182: }
> 183: assert(in == NULL);
assert is typically used for sanity test. As part of the test validation,
e.g. in this case `in == NULL` or `in != NULL` in line 157, it may be clearer
if it's an explicit check and throw exception to indicate test failure
especially in case `#undef NDEBUG` may not be set in the test.
-
PR: https://git.openjdk.java.net/jdk/pull/8134
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v9]
> Supporting `IsoFields` temporal fields in chronologies that are similar to > ISO chronology. Corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7683/files - new: https://git.openjdk.java.net/jdk/pull/7683/files/c3f15f6a..45edbc3b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7683=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7683=07-08 Stats: 50 lines in 6 files changed: 18 ins; 7 del; 25 mod Patch: https://git.openjdk.java.net/jdk/pull/7683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7683/head:pull/7683 PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
Not yet talked with John, but i investigated further. The implementation of the
`LSHR` operation is behaving as intended, but is under specified with regards
to `byte` and `short` as you noted in #8291.
This is a subtle area, but i am wondering if the user really means to use
arithmetic shift in this case? Since is not the following true for all values
of `e` and `c`, where `e` is a `byte` and `c` is the right shift count ranging
from 0 to 7:
(byte) (e >>> c) == (byte) (e >> c)
?
Then the user can use `VectorOperators.ASHR`.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE [v2]
On Tue, 19 Apr 2022 17:01:38 GMT, Mandy Chung wrote: >> Claes Redestad has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Missed a spot! > > src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 901: > >> 899: MemberName member = new >> MemberName(MethodHandleStatics.class, "UNSAFE", Unsafe.class, REF_getStatic); >> 900: return new NamedFunction( >> 901: >> MemberName.getFactory().resolveOrFail(REF_getField, member, > > `REF_getField` passed to `resolveOrFail` should also be corrected? Yes, fixed. - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE [v2]
On Tue, 19 Apr 2022 17:29:12 GMT, Claes Redestad wrote: >> Trivially fix the resolution of the `NF_UNSAFE` named function to use the >> appropriate lookup mode. >> >> In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we >> changed from regular reflection to use a `NamedFunction` for this field, but >> it appears the lookup mode came out wrong. This mismatch appears to be >> benign and ignored by HotSpot, but a user implementing an experimental JVM >> ran into some issues (and additionally noticed the resolve throws the wrong >> exception). >> >> This patch is a point fix to this particular issue, but I think we should >> consider following up with a stronger assertion or test for proper >> staticness of fields somewhere when resolving fields via >> `MemberName.getFactory().resolveOrNull(..)`. > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Missed a spot! Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE [v2]
> Trivially fix the resolution of the `NF_UNSAFE` named function to use the > appropriate lookup mode. > > In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed > from regular reflection to use a `NamedFunction` for this field, but it > appears the lookup mode came out wrong. This mismatch appears to be benign > and ignored by HotSpot, but a user implementing an experimental JVM ran into > some issues (and additionally noticed the resolve throws the wrong > exception). > > This patch is a point fix to this particular issue, but I think we should > consider following up with a stronger assertion or test for proper staticness > of fields somewhere when resolving fields via > `MemberName.getFactory().resolveOrNull(..)`. Claes Redestad has updated the pull request incrementally with one additional commit since the last revision: Missed a spot! - Changes: - all: https://git.openjdk.java.net/jdk/pull/8297/files - new: https://git.openjdk.java.net/jdk/pull/8297/files/fa10026a..e4f37c49 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8297=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8297=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8297.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8297/head:pull/8297 PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284880: Re-examine sun.invoke.util.Wrapper hash tables [v2]
On Thu, 14 Apr 2022 13:59:57 GMT, Claes Redestad wrote: >> This patch examines and optimizes `Wrapper` lookups. >> >> First wrote a few simple microbenchmarks to verify there are actual speedups >> from using perfect hash tables in `sun.invoke.util.Wrapper` compared to >> simpler lookup mechanisms (such as if-else or switch). Turns out there _is_ >> a speed-up for the case of `char` -> `Wrapper`, but not when mapping from >> `Class` -> `Wrapper`, so let's drop those. The `forPrimitiveType` case >> didn't use the `FROM_CHAR` table for some reason, which is remedied. >> >> Micros show benefits across the board for warmed up case: >> >> >> Baseline, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.387 ? 0.127 ns/op >> Wrappers.forPrimitive avgt5 38.818 ? 0.592 ns/op >> Wrappers.forPrimitiveType avgt5 26.085 ? 2.291 ns/op >> Wrappers.forWrapperavgt5 44.459 ? 1.635 ns/op >> >> Patch, OOTB >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 14.357 ? 0.133 ns/op >> Wrappers.forPrimitive avgt5 23.930 ? 0.071 ns/op >> Wrappers.forPrimitiveType avgt5 14.343 ? 0.017 ns/op >> Wrappers.forWrapperavgt5 27.622 ? 0.022 ns/op >> >> >> For `-Xint` case (`Wrapper` use is prominent during warmup, e.g., when >> spinning up of MHs) there are decent or even great wins in all cases but >> `forPrimitiveType` - which was changed from a simple switch to use the hash >> lookup. Since the interpreter penalty is small in absolute terms and the win >> on JITed code is significant this seems like a reasonable trade-off: >> >> >> Baseline, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1246.144 ? 149.933 ns/op >> Wrappers.forPrimitive avgt5 4955.297 ? 329.869 ns/op >> Wrappers.forPrimitiveType avgt5 716.840 ? 62.568 ns/op >> Wrappers.forWrapperavgt5 5774.700 ? 367.627 ns/op >> >> Patch, -Xint >> Benchmark Mode Cnt Score Error Units >> Wrappers.forBasicType avgt5 1068.096 ? 101.728 ns/op >> Wrappers.forPrimitive avgt5 1146.670 ? 59.142 ns/op >> Wrappers.forPrimitiveType avgt5 998.037 ? 118.144 ns/op >> Wrappers.forWrapperavgt5 3581.226 ? 20.167 ns/op > > Claes Redestad has updated the pull request incrementally with one additional > commit since the last revision: > > Add line break in make/test/BuildMicrobenchmark.gmk > > Co-authored-by: Erik Joelsson <37597443+erik...@users.noreply.github.com> Looks good.The copyright header end-year needs update before you push. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8242
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
Fixing deserialization is not particularly hard. A longer term goal is to
declare a readResolve for proxies (presumably by extra methods on invocation
handlers) to convert serialization result to something else
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
On Sun, 17 Apr 2022 16:17:30 GMT, liach wrote:
> Convert dynamic proxies to hidden classes. Modifies the serialization of
> proxies (requires change in "Java Object Serialization Specification"). Makes
> the proxies hidden in stack traces. Removes duplicate logic in proxy building.
>
> The main compatibility changes and their rationales are:
> 1. Modification to the serialization specification: In the "An instance of
> the class is allocated... The contents restored appropriately" section, I
> propose explicitly state that handling of proxies are unspecified as to allow
> implementation freedom, though I've seen deliberate attempts for proxies to
> implement interfaces with `readResolve` in order to control their
> serialization behavior.
>- This is for the existing generated constructor accessor is
> bytecode-based, which cannot refer to hidden classes.
>- An alternative is to preserve the behavior, where the serialization
> constructor calls `invokespecial` on the closest serializable superclass'
> no-arg constructor, like in #1830 by @DasBrain.
>- My rationale against preservation is such super calls are unsafe and
> should be discouraged in the long term. Calling the existing constructor with
> a dummy argument, as in my implementation, would be more safe.
> 2. The disappearance of proxies in stack traces.
>- Same behavior exists in lambda expressions: For instance, in
> `((Runnable) () -> { throw new Error(); }).run();`, the `run` method,
> implemented by the lambda, will not appear in the stack trace, and isn't too
> problematic.
>
> A summary of the rest of the changes:
> 1. Merged the two passes of determining module and package of the proxy into
> one. This reduced a lot of code and allowed anchor class (for hidden class
> creation) selection be done together as well.
> 2. Exposed internal API for obtaining a full-privileged lookup to the rest of
> `java.base`. This API is intended for implementation of legacy (pre
> `MethodHandles.Lookup`) caller sensitive public APIs so they don't need more
> complex tricks to obtain proper permissions as lookups.
> 3. Implements [8229959](https://bugs.openjdk.java.net/browse/JDK-8229959):
> passes methods computed by proxy generator as class data to the hidden proxy
> class to reduce generated proxy class size and improve performance.
>
> In addition, since this change is somewhat large, should we keep the old
> proxy generator as well and have it toggled through a command-line flag (like
> the old v49 proxy generator or the old reflection implementation)?
>
> Please feel free to comment or review. This change definitely requires a CSR,
> but I have yet to determine what specifications should be changed.
About deserializing Proxies, this is currently done in 3 steps:
1. Finding the class
2. Allocating a new instance and invoke the constructor of the **first
non-serializable superclass**
3. Setting the fields of the instance
Only step 2 is problematic here:
1. For a proxy, the class is serialized using a TC_PROXYCLASSDESC. when
deserialized it invokes `Proxy.getProxyClass`
(https://github.com/openjdk/jdk/blob/13fb1eed52f1a9152242119969a9d4a0c0627513/src/java.base/share/classes/java/io/ObjectInputStream.java#L891).
For this step, it doesn't matter if `Proxy.getProxyClass` returns a normal
class or a hidden class.
2. Allocating and calling the constructor:
This part is currently implemented by spinning a class. The generated
bytecode looks more or less like this:
anew ProxyClass
invokespecial Object.()
The big lie here is that methods and constructors are different - but
constructors are just methods, and the verifier makes sure that a constructor
is called only once, exactly once, and that uninitialized instances don't
escape.
This doesn't work for hidden classes - as the hidden class can not be named
in an other class.
But MethodHandles can do this. Here is one of my old prototypes, based on a
prototype for implementing reflection using MethodHandles from Mandy Chung:
https://github.com/dasbrain/jdk/compare/ae45c5de7fc635df4dc86b764405158c245d2765...fbb0a63436f696a85e7039c0e109c379dfa1edce
3. Setting the fields uses deep reflection.
But the hidden class themself doesn't have any fields - just it's
superclass `java.lang.reflect.Proxy.h`.
This is not a problem if the class can be instantiated at all (see step 2).
-
PR: https://git.openjdk.java.net/jdk/pull/8278
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE
On Tue, 19 Apr 2022 13:19:31 GMT, Claes Redestad wrote: > Trivially fix the resolution of the `NF_UNSAFE` named function to use the > appropriate lookup mode. > > In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed > from regular reflection to use a `NamedFunction` for this field, but it > appears the lookup mode came out wrong. This mismatch appears to be benign > and ignored by HotSpot, but a user implementing an experimental JVM ran into > some issues (and additionally noticed the resolve throws the wrong > exception). > > This patch is a point fix to this particular issue, but I think we should > consider following up with a stronger assertion or test for proper staticness > of fields somewhere when resolving fields via > `MemberName.getFactory().resolveOrNull(..)`. src/java.base/share/classes/java/lang/invoke/DirectMethodHandle.java line 901: > 899: MemberName member = new > MemberName(MethodHandleStatics.class, "UNSAFE", Unsafe.class, REF_getStatic); > 900: return new NamedFunction( > 901: > MemberName.getFactory().resolveOrFail(REF_getField, member, `REF_getField` passed to `resolveOrFail` should also be corrected? - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284893: Fix typos in java.base [v4]
On Tue, 19 Apr 2022 16:50:12 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with two > additional commits since the last revision: > > - Update Oracle copyrights > - Also revert changes in ASM (3rd party code) Marked as reviewed by lancea (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Tue, 19 Apr 2022 10:49:14 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:
>>
>>> 59: private final Condition notEmpty;
>>> 60:
>>> 61: void signal() { notEmpty.signalAll(); }
>>
>> `signal()`, `await()` and `await(long)` methods are only used by
>> `ReferenceQueue`. Good to make them private.
>
> They are overridden so can't be private.
That's right, I missed it.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284893: Fix typos in java.base [v3]
On Tue, 19 Apr 2022 16:24:43 GMT, Magnus Ihse Bursie wrote: >> I ran `codespell` on the `src/java.base` directory, and accepted those >> changes where it indeed discovered real typos. >> >> (Due to false positives this can unfortunately not be run automatically) >> >> The majority of fixes are in comments. A handful is in strings, one in a >> local variable name, and a couple in parameter declarations. >> >> Annoyingly, there are several instances of "childs" (instead of "children") >> in the source code, but they were not local and I dared not change them. >> Someone braver than me might take a stab at it, perhaps.. > > Magnus Ihse Bursie has updated the pull request incrementally with one > additional commit since the last revision: > > Vertices -> vertices It's a bit annoying that 3rd party code is not more distinctly handled in the JDK source. :( I reverted the code pointed out by reviewers, but then later found the ASM code as well. If I feel really motivated (or bored) I might try to submit PRs with these fixes upstream. Or not. Everybody happy now? - PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284893: Fix typos in java.base [v4]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with two additional commits since the last revision: - Update Oracle copyrights - Also revert changes in ASM (3rd party code) - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/2b029279..a3f75247 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=02-03 Stats: 134 lines in 133 files changed: 0 ins; 0 del; 134 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v8]
> Supporting `IsoFields` temporal fields in chronologies that are similar to > ISO chronology. Corresponding CSR has also been drafted. Naoto Sato 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: - Revert to use the default method - Removed unnecessary package names - Modified class desc of `IsoFields` - abstract class -> top level interface - interface -> abstract class - Removed the method - Changed to use a type to determine ISO based or not - Renamed the new method - Addresses review comments - copyright year fix - ... and 1 more: https://git.openjdk.java.net/jdk/compare/583eba4e...c3f15f6a - Changes: - all: https://git.openjdk.java.net/jdk/pull/7683/files - new: https://git.openjdk.java.net/jdk/pull/7683/files/82339ec6..c3f15f6a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7683=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7683=06-07 Stats: 1815 lines in 155 files changed: 1085 ins; 236 del; 494 mod Patch: https://git.openjdk.java.net/jdk/pull/7683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7683/head:pull/7683 PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8284893: Fix typos in java.base [v3]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Vertices -> vertices - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/31baf0a3..2b029279 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE
On Tue, 19 Apr 2022 13:19:31 GMT, Claes Redestad wrote: > Trivially fix the resolution of the `NF_UNSAFE` named function to use the > appropriate lookup mode. > > In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed > from regular reflection to use a `NamedFunction` for this field, but it > appears the lookup mode came out wrong. This mismatch appears to be benign > and ignored by HotSpot, but a user implementing an experimental JVM ran into > some issues (and additionally noticed the resolve throws the wrong > exception). > > This patch is a point fix to this particular issue, but I think we should > consider following up with a stronger assertion or test for proper staticness > of fields somewhere when resolving fields via > `MemberName.getFactory().resolveOrNull(..)`. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8297
Re: RFR: 8284893: Fix typos in java.base [v2]
> I ran `codespell` on the `src/java.base` directory, and accepted those > changes where it indeed discovered real typos. > > (Due to false positives this can unfortunately not be run automatically) > > The majority of fixes are in comments. A handful is in strings, one in a > local variable name, and a couple in parameter declarations. > > Annoyingly, there are several instances of "childs" (instead of "children") > in the source code, but they were not local and I dared not change them. > Someone braver than me might take a stab at it, perhaps.. Magnus Ihse Bursie has updated the pull request incrementally with one additional commit since the last revision: Revert changes to 3rd party code - Changes: - all: https://git.openjdk.java.net/jdk/pull/8250/files - new: https://git.openjdk.java.net/jdk/pull/8250/files/5e3a01c6..31baf0a3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=8250=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=8250=00-01 Stats: 15 lines in 11 files changed: 0 ins; 0 del; 15 mod Patch: https://git.openjdk.java.net/jdk/pull/8250.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8250/head:pull/8250 PR: https://git.openjdk.java.net/jdk/pull/8250
Re: RFR: 8284992: Fix misleading Vector API doc for LSHR operator
On Tue, 19 Apr 2022 08:41:50 GMT, Jie Fu wrote: > Hi all, > > The Current Vector API doc for `LSHR` is > > Produce a>>>(n&(ESIZE*8-1)). Integral only. > > > This is misleading which may lead to bugs for Java developers. > This is because for negative byte/short elements, the results computed by > `LSHR` will be different from that of `>>>`. > For more details, please see > https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . > > After the patch, the doc for `LSHR` is > > Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. > > > Thanks. > Best regards, > Jie I need to think a little more about this. The specification is not accurate and likely requires a CSR. My initial thoughts are I would prefer the operation to retain reference to the succinct definition using the logical right shift operator but we add additional specification explaining the cases for `byte` and `short`, namely that the result is widened to an `int` as if by `(a & ((1 << ESIZE) - 1))` and the result narrowed. That's commonly (at least for the widening part) how `>>>` is used with `byte` and `short`, and i think would be clearer to be explicit in that regard. - PR: https://git.openjdk.java.net/jdk/pull/8291
Re: RFR: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 13:37:32 GMT, Roger Riggs wrote: > Nice Catch! +1 - PR: https://git.openjdk.java.net/jdk/pull/8292
Re: RFR: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 08:50:09 GMT, Claes Redestad wrote: > There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8292
Integrated: 8284856: Add a test case for checking UnicodeScript entity numbers
On Thu, 14 Apr 2022 20:52:44 GMT, Naoto Sato wrote: > Added the test case, and eliminated the immediate hashmap value, replaced > with the ordinal number of `Character.UnicodeScript.UNKNOWN`. This pull request has now been integrated. Changeset: eb9c457b Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/eb9c457b4141f2d253be14cbbad844bc4ba8c48d Stats: 13 lines in 2 files changed: 8 ins; 0 del; 5 mod 8284856: Add a test case for checking UnicodeScript entity numbers Reviewed-by: iris, smarks - PR: https://git.openjdk.java.net/jdk/pull/8253
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
I need to discuss with @rose00 on the history behind this before deciding what
to do.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v4]
On Mon, 7 Mar 2022 18:47:17 GMT, Naoto Sato wrote:
>> src/java.base/share/classes/java/time/chrono/IsoChronology.java line 689:
>>
>>> 687: */
>>> 688: @Override
>>> 689: public boolean isIsoBased() {
>>
>> Is this meant to be 'default'? The CSR indicated adding default methods.
>
> The `default` method has been added to `java.time.chrono.Chronology`
> interface. This is its overridden method implemented in `IsoChronology`
> concrete class.
To make navigation easier, change the @code references to be @links and include
the IsoField name.
I don't think the @implspec is needed, its redundant with @return.
Apply similar changes to the other ISO chronologies javadoc.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v7]
On Fri, 15 Apr 2022 18:47:53 GMT, Naoto Sato wrote:
>> Supporting `IsoFields` temporal fields in chronologies that are similar to
>> ISO chronology. Corresponding CSR has also been drafted.
>
> Naoto Sato 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 15 additional commits since
> the last revision:
>
> - Revert to use the default method
> - Merge branch 'master' into JDK-8279185
> - Removed unnecessary package names
> - Modified class desc of `IsoFields`
> - abstract class -> top level interface
> - interface -> abstract class
> - Merge branch 'master' into JDK-8279185
> - Removed the method
> - Merge branch 'master' into JDK-8279185
> - Changed to use a type to determine ISO based or not
> - ... and 5 more:
> https://git.openjdk.java.net/jdk/compare/5ee1a816...82339ec6
src/java.base/share/classes/java/time/chrono/Chronology.java line 794:
> 792: * @since 19
> 793: */
> 794: default boolean isIsoBased() {
Can the description be more specific. Each of the chronologies mentioned
say they have the same number of months, the number of days in each month, and
day-of-year and leap years are the same as ISO. The week-based fields in
IsoFields also depend ISO defined concepts.
Perhaps it should say that this method should return true only if all of the
fields of IsoFields are supported for the chronology.
The chronology names could be links and omit the @see tags, including
ISOChronology.
In the @return add ", otherwise return {@code false}."
src/java.base/share/classes/java/time/temporal/IsoFields.java line 600:
> 598: if (!isIsoBased(temporal)) {
> 599: throw new DateTimeException("Resolve requires ISO based
> chronology: " +
> 600: Chronology.from(temporal));
The name change doesn't add much, I'd leave both `ensureIso` and `isIso` method
names unchanged.
src/java.base/share/classes/java/time/temporal/IsoFields.java line 739:
> 737:
> 738: static boolean isIsoBased(TemporalAccessor temporal) {
> 739: return Chronology.from(temporal).isIsoBased();
Can this method be private static?
Also, move the `ensureIso` method to be next to `isIso`.
-
PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8285001: Simplify StringLatin1.regionMatches
On Tue, 19 Apr 2022 08:50:09 GMT, Claes Redestad wrote: > There is no pair of character values in the latin1 range (0-255) that will > make the condition at line 401 in `StringLatin1.java` true, so this test can > be removed. > > Added a test and a microbenchmark (which as expected sees a few ns/op > improvement from this change). Took the opportunity to tune the default > settings for the micro to reduce runtime from 30+ minutes to 3 with no > discernible degradation of quality. Nice Catch! - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8292
RFR: 8285007: Use correct lookup mode for MethodHandleStatics.UNSAFE
Trivially fix the resolution of the `NF_UNSAFE` named function to use the appropriate lookup mode. In [JDK-8187826](https://bugs.openjdk.java.net/browse/JDK-8187826) we changed from regular reflection to use a `NamedFunction` for this field, but it appears the lookup mode came out wrong. This mismatch appears to be benign and ignored by HotSpot, but a user implementing an experimental JVM ran into some issues (and additionally noticed the resolve throws the wrong exception). This patch is a point fix to this particular issue, but I think we should consider following up with a stronger assertion or test for proper staticness of fields somewhere when resolving fields via `MemberName.getFactory().resolveOrNull(..)`. - Commit messages: - Use correct lookup mode for MethodHandleStatics.UNSAFE Changes: https://git.openjdk.java.net/jdk/pull/8297/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8297=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285007 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8297.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8297/head:pull/8297 PR: https://git.openjdk.java.net/jdk/pull/8297
RFR: 8285001: Simplify StringLatin1.regionMatches
There is no pair of character values in the latin1 range (0-255) that will make the condition at line 401 in `StringLatin1.java` true, so this test can be removed. Added a test and a microbenchmark (which as expected sees a few ns/op improvement from this change). Took the opportunity to tune the default settings for the micro to reduce runtime from 30+ minutes to 3 with no discernible degradation of quality. - Commit messages: - Add tests and micro - Simplify latin1 to latin1 locale-independent comparisons in StringLatin1.regionMatchesCI Changes: https://git.openjdk.java.net/jdk/pull/8292/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8292=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8285001 Stats: 94 lines in 3 files changed: 88 ins; 3 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8292.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8292/head:pull/8292 PR: https://git.openjdk.java.net/jdk/pull/8292
Re: RFR: 8284932: [Vector API] Incorrect implementation of LSHR operator for negative byte/short elements
On Sun, 17 Apr 2022 14:35:14 GMT, Jie Fu wrote:
> Hi all,
>
> According to the Vector API doc, the `LSHR` operator computes
> `a>>>(n&(ESIZE*8-1))`.
> However, current implementation is incorrect for negative bytes/shorts.
>
> The background is that one of our customers try to vectorize `urshift` with
> `urshiftVector` like the following.
>
> 13 public static void urshift(byte[] src, byte[] dst) {
> 14 for (int i = 0; i < src.length; i++) {
> 15 dst[i] = (byte)(src[i] >>> 3);
> 16 }
> 17 }
> 18
> 19 public static void urshiftVector(byte[] src, byte[] dst) {
> 20 int i = 0;
> 21 for (; i < spec.loopBound(src.length); i +=spec.length()) {
> 22 var va = ByteVector.fromArray(spec, src, i);
> 23 var vb = va.lanewise(VectorOperators.LSHR, 3);
> 24 vb.intoArray(dst, i);
> 25 }
> 26
> 27 for (; i < src.length; i++) {
> 28 dst[i] = (byte)(src[i] >>> 3);
> 29 }
> 30 }
>
>
> Unfortunately and to our surprise, code@line28 computes different results
> with code@line23.
> It took quite a long time to figure out this bug.
>
> The root cause is that current implemenation of Vector API can't compute the
> unsigned right shift results as what is done for scalar `>>>` for negative
> byte/short elements.
> Actually, current implementation will do `(a & 0xFF) >>> (n & 7)` [1] for all
> bytes, which is unable to compute the vectorized `>>>` for negative bytes.
> So this seems unreasonable and unfriendly to Java developers.
> It would be better to fix it.
>
> The key idea to support unsigned right shift of negative bytes/shorts is just
> to replace the unsigned right shift operation with the signed right shift
> operation.
> This logic is:
> - For byte elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 24.
> - For short elements, unsigned right shift is equal to signed right shift if
> the shift_cnt <= 16.
> - For Vector API, the shift_cnt will be masked to shift_cnt <= 7 for bytes
> and shift_cnt <= 15 for shorts.
>
> I just learned this idea from https://github.com/openjdk/jdk/pull/7979 .
> And many thanks to @fg1417 .
>
>
> Thanks.
> Best regards,
> Jie
>
> [1]
> https://github.com/openjdk/jdk/blob/master/src/jdk.incubator.vector/share/classes/jdk/incubator/vector/ByteVector.java#L935
We plan to fix the doc: https://github.com/openjdk/jdk/pull/8291 first.
Then, let's see what @PaulSandoz would think of adding a new operator for the
scalar `>>>`.
Thanks.
-
PR: https://git.openjdk.java.net/jdk/pull/8276
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v4]
On Tue, 19 Apr 2022 00:09:04 GMT, Mandy Chung wrote:
>> Alan Bateman has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Refresh
>
> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 61:
>
>> 59: private final Condition notEmpty;
>> 60:
>> 61: void signal() { notEmpty.signalAll(); }
>
> `signal()`, `await()` and `await(long)` methods are only used by
> `ReferenceQueue`. Good to make them private.
They are overridden so can't be private.
> src/java.base/share/classes/java/lang/ref/ReferenceQueue.java line 206:
>
>> 204: throws IllegalArgumentException, InterruptedException {
>> 205: if (timeout < 0) throw new IllegalArgumentException("Negative
>> timeout value");
>> 206: if (timeout == 0) return remove();
>
> Nit: use the same formatting as in `NativeReferenceQueue::remove` and in this
> file.
>
>
> if (timeout < 0)
> throw new IllegalArgumentException("Negative timeout value");
> if (timeout == 0)
> return remove();
okay
> src/java.management/share/classes/java/lang/management/ThreadMXBean.java line
> 655:
>
>> 653: * synchronization control. It might be an expensive operation.
>> 654: * Its behavior with cycles involving virtual threads is not defined
>> 655: * in this release.
>
> What does the current implementation return if the virtual threads are
> involved in a deadlock?The class spec says "ThreadMXBean does not support
> monitoring or management of virtual threads" while this javadoc says it's
> undefined.I wonder if it's helpful to include `@implNote` if appropriate.
If there is cycle involving a virtual thread then the thread id of its carrier
will be included in array. I think we can do better and include the thread id
of the mounted thread.
> src/jdk.management/share/classes/com/sun/management/HotSpotDiagnosticMXBean.java
> line 138:
>
>> 136: *
>> 137: * @param outputFile the path to the file to create
>> 138: * @param format the format to use (TEXT_PLAIN or JSON)
>
> It's useful to link to `ThreadDumpFormat#TEXT_PLAN` and
> `ThreadDumpFormat#JSON`
I think it might be better if the description is changed to "the format to
use". There is already a link to the ThreadDumpFormat enum in method signature.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284435: Add dedicated filler objects for known dead Java heap areas [v3]
On Mon, 11 Apr 2022 21:51:32 GMT, David Holmes wrote: > Do you really need to define a real `FillerObject.java` class? Can't you use > an internal-only variant of a hidden class to represent this? I am not sure I understand this request, but of course I am open to try different approaches. An existing example would be fine to get me on track; but maybe I am only not understanding your terminology: do you mean to have a special subclass of instanceKlass for `FillerObject`? I had that, but that has been much much more effort (in terms of code, maintainability, ...) than having such an additional Java class file in the JDK and use the existing macros to use it everywhere. - PR: https://git.openjdk.java.net/jdk/pull/8156
RFR: 8284992: Fix misleading Vector API doc for LSHR operator
Hi all, The Current Vector API doc for `LSHR` is Produce a>>>(n&(ESIZE*8-1)). Integral only. This is misleading which may lead to bugs for Java developers. This is because for negative byte/short elements, the results computed by `LSHR` will be different from that of `>>>`. For more details, please see https://github.com/openjdk/jdk/pull/8276#issue-1206391831 . After the patch, the doc for `LSHR` is Produce zero-extended right shift of a by (n&(ESIZE*8-1)) bits. Integral only. Thanks. Best regards, Jie - Commit messages: - 8284992: Fix misleading Vector API doc for LSHR operator Changes: https://git.openjdk.java.net/jdk/pull/8291/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8291=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284992 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/8291.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8291/head:pull/8291 PR: https://git.openjdk.java.net/jdk/pull/8291
RFR: 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false]
Please review these small changes to address intermittent failures, as of JDK-8274517. - Usage of jdk.test.lib.RandomFactory for reproducible random generation. - Slightly less restrictive assertion about badParallelStreamError on L94 (former L88). - Verbatim copy of computeFinalSum() from j.u.s.Collectors 18. While these changes do not necessarily guarantee absence of intermittent failures, the usage of jdk.test.lib.RandomFactory should at least help to pin down specific double sequences that do not pass the test. There is still an inherent variability due to the use of parallel streams, though. As double addition is not perfectly associative, even a fully known sequence of doubles may lead to slightly different results with parallelization. - Commit messages: - 8274517: java/util/DoubleStreamSums/CompensatedSums.java fails with expected [true] but found [false] Changes: https://git.openjdk.java.net/jdk/pull/8290/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=8290=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8274517 Stats: 13 lines in 1 file changed: 7 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8290.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8290/head:pull/8290 PR: https://git.openjdk.java.net/jdk/pull/8290
Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
- Original Message -
> From: "liach"
> To: "core-libs-dev" , "security-dev"
>
> Sent: Tuesday, April 19, 2022 3:31:24 AM
> Subject: Re: RFR: JDK-8242888: Convert dynamic proxy to hidden classes
> On Mon, 18 Apr 2022 20:42:48 GMT, Remi Forax wrote:
>
>> The third parameter of defineProxy() is a lambda which is called for each
>> method
>> that can be overriden, either toString/equals/hashCode but also any default
>> methods,
> if the lambda return true, the method is overriden, otherwise the default
> implementation is used.
>
> Not quite, I mean calling default implementations in the super interface,
> consider:
>
> interface Root { void cleanUp(); }
> interface FeatureOne extends Root { default void cleanUp() { /* do something
> */
> } }
> interface FeatureTwo extends Root { default void cleanUp() { /* do something
> */
> } }
>
> My proxy implements both feature one and two, but in your API, there is no way
> for my `cleanUp` to call both `FeatureOne.super.cleanUp();` and
> `FeatureTwo.super.cleanUp();`. You should probably expose the lookup in your
> linker too, especially that you enabled nest access and you can just use that
> lookup to resolve, say, an implementation static method in the proxy user
> class.
yes, you are right, i should send the Lookup too.
>
> Also the "delegate" in your API would significantly benefit from
> https://bugs.openjdk.java.net/browse/JDK-8282798 (#7744), too.
I don't think i need the carrier API, the idea is to have only one field in the
proxy, this field can be a value type (exactly a primitive class) in the
future, so being expanded into multiple fields by the VM at runtime.
>
> -
>
> PR: https://git.openjdk.java.net/jdk/pull/8278
