Re: RFR: 8302791: Add specific ClassLoader object to Proxy IllegalArgumentException message [v3]
On Wed, 22 Feb 2023 07:24:48 GMT, Alan Bateman wrote:
>> Ravali Yatham has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix Indentation
>
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 881:
>
>> 879: if (type != c) {
>> 880: throw new IllegalArgumentException(c.getName() +
>> 881: " referenced from a method is not visible from
>> class loader: " + ld);
>
> The ClassLoader string representation may or may not be useful here. One
> suggestion is to use the class loader name (ld.getName()) if not null,
> otherwise Objects.toIdentityString(ld).
@AlanBateman @mlchung - Thanks for reviewing. I've incorporated review comments
wherein using `@` if not null, otherwise
`Objects.toIdentityString(ld)`. PTAL.
-
PR: https://git.openjdk.org/jdk/pull/12641
Re: RFR: 8302791: Add specific ClassLoader object to Proxy IllegalArgumentException message [v3]
> Added specific class loader object to proxy IllegalArgumentException from > which the class was not visible > > The bug report for the same: https://bugs.openjdk.org/browse/JDK-8302791 Ravali Yatham has updated the pull request incrementally with one additional commit since the last revision: Fix Indentation - Changes: - all: https://git.openjdk.org/jdk/pull/12641/files - new: https://git.openjdk.org/jdk/pull/12641/files/2682d88c..c2516408 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12641&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12641&range=01-02 Stats: 5 lines in 1 file changed: 0 ins; 0 del; 5 mod Patch: https://git.openjdk.org/jdk/pull/12641.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12641/head:pull/12641 PR: https://git.openjdk.org/jdk/pull/12641
Re: RFR: 8302791: Add specific ClassLoader object to Proxy IllegalArgumentException message [v2]
> Added specific class loader object to proxy IllegalArgumentException from > which the class was not visible > > The bug report for the same: https://bugs.openjdk.org/browse/JDK-8302791 Ravali Yatham has updated the pull request incrementally with one additional commit since the last revision: Use classloaderName if not Null otherwise Objects.toIdentityString(ld) - Changes: - all: https://git.openjdk.org/jdk/pull/12641/files - new: https://git.openjdk.org/jdk/pull/12641/files/1ed3c72d..2682d88c Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12641&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12641&range=00-01 Stats: 9 lines in 1 file changed: 8 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12641.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12641/head:pull/12641 PR: https://git.openjdk.org/jdk/pull/12641
Re: RFR: 8302204: Optimize BigDecimal.divide
On Fri, 17 Feb 2023 15:24:26 GMT, Raffaello Giulietti wrote: >>> After making sure that `intVal` is even, and before attempting a division >>> by a power of 10, it might help to check if 5 divides `intVal` in the first >>> place. If it doesn't, there no point in performing the division. >>> >>> It can be shown that 5 divides `intVal` if and only if it divides the >>> `long` sum of all `int`s in the `mag` array of `intVal`. >>> >>> I didn't try out if this contributes to improve the overall performance, >>> but it might be worth giving a try. >> >> Unfortunately, it seems this division by 5 doesn't give an obvious >> performance improvement. Maybe previous lowest bit check has filtered some >> cases. > >> > After making sure that `intVal` is even, and before attempting a division >> > by a power of 10, it might help to check if 5 divides `intVal` in the >> > first place. If it doesn't, there no point in performing the division. >> > It can be shown that 5 divides `intVal` if and only if it divides the >> > `long` sum of all `int`s in the `mag` array of `intVal`. >> > I didn't try out if this contributes to improve the overall performance, >> > but it might be worth giving a try. >> >> Unfortunately, it seems this division by 5 doesn't give an obvious >> performance improvement. Maybe previous lowest bit check has filtered some >> cases. > > Thanks for trying out. @rgiulietti Hi, could you please review this pr - PR: https://git.openjdk.org/jdk/pull/12509
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Fri, 24 Feb 2023 07:17:30 GMT, Jorn Vernee wrote: >> Some more remarks about other issues: >> - Uploaded my simple reproducer to >> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017) >> - Using oversized load / stores is problematic. Don't forget that OpenJDK >> still supports Big Endian platforms (AIX, s390x). >> - The result of `NativeCallingConvention::calling_convention` is interpreted >> as size, but it returns the max offset. That's off by one slot. Should I >> file a bug for that? (PPC64 is not affected because it doesn't use the >> result.) >> - Since the membar on the return path was mentioned: I think it would be >> good to enable UseSystemMemoryBarrier by default on operating systems which >> support it. Maybe we should discuss this with @robehn. > >> * Uploaded my simple reproducer to >> [JDK-8303017](https://bugs.openjdk.org/browse/JDK-8303017) > > Thanks! > >> * Using oversized load / stores is problematic. Don't forget that >> OpenJDK still supports Big Endian platforms (AIX, s390x). > > You're right. I realized that it's also problematic for heap segments, for > which we can't do oversized accesses. I am working on another solution that > splits up the loads/stores into power-of-two sized chunks: > https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:OOB > That patch is just a POC at this point though. Also, I don't think it works > for BE at the moment (need to flip the offset for BE, I think. Just like we > do in Unsafe). > >> * The result of `NativeCallingConvention::calling_convention` is >> interpreted as size, but it returns the max offset. That's off by one slot. >> Should I file a bug for that? (PPC64 is not affected because it doesn't use >> the result.) > > I'm not sure there's an issue there. Note that the 'max offset' is computed > as `reg.offset() + reg.stack_size()`, so that should get us the size we need > to allocate for the stack arguments. (e.g. 2 ints being passed at offset 0 > and 4, would make max offset 4 + 4 = 8, which gives the size needed for the 2 > ints). Computing the max offset instead of just summing the sizes of the > stack arguments is needed since stack arguments can be sparsely placed in > some cases on Mac/AArch64. > >> * Since the membar on the return path was mentioned: I think it would be >> good to enable UseSystemMemoryBarrier by default on operating systems which >> support it. Maybe we should discuss this with @robehn. > > ~I don't think we've done that much testing with UseSystemMemoryBarrier since > it was added~. I'm a bit nervous about turning it on by default since it's > currently also used for JNI. Let's see what Robbin thinks. @JornVernee: Thanks a lot for your detailed review! I have quite a few TODOs which include: - Include my tests for the HFA corner cases. - Try to improve handling of the overlapping registers as you suggested. - Check nesting of HFA. There will surely be more when looking into Big Endian support after merging with your recent work on https://github.com/openjdk/panama-foreign/compare/foreign-memaccess+abi...JornVernee:panama-foreign:OOB We should get rid of oversized accesses on PPC64, too. Thanks for sharing your plans to intrisify `linkToNative` in C2 later. I guess we should do more preparation work on all platforms when that gets addressed. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:55:52 GMT, Paul Sandoz wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Uppercase Thorn is 0xDE
>> - Update 'a' to 'A' and 'z' to 'Z' in comments
>
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 67:
>
>> 65: public void scalar(Blackhole blackhole) {
>> 66: blackhole.consume(scalarEqualsIgnoreCase(a, b, len));
>> 67: }
>
> If you like there is no need to explicitly use a black hole, instead declare
> the benchmark method to return `boolean` and return the result of the call.
> JMH will do the right thing. I find that a little more concise and easier to
> read.
This was much cleaner, thank you!
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v4]
On Wed, 1 Mar 2023 02:32:23 GMT, Xiaohong Gong wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Adjust whitespace as suggested in review
>> - Replace Blackhold.consume with return values
>
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 61:
>
>> 59: len = a.length;
>> 60: }
>> 61: @Benchmark
>
> Style: Insert one blank line between line 60-61?
I have inserted a new line. Thanks for your close reading!
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 124:
>
>> 122: }
>> 123:
>> 124: public boolean scalarEqualsIgnoreCase(byte[] a, byte[] b, int len)
>> {
>
> Style: remove one more space between "`public boolean`" please?
Extra space removed.
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 147:
>
>> 145: return false; // Low ASCII
>> 146: }
>> 147: return ( U <= 'Z' // In range A-Z
>
> Style: remove one more space between `( U` please?
Thanks, removed the extra space.
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v4]
> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of > benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves > as an example of how vectorization can be useful also in the area of text > processing. It takes advantage of the fact that ASCII and Latin-1 were > designed to optimize case-twiddling operations. > > The code came about during the work on #12632, where vectorization was deemed > out of scope. > > Benchmark results: > > > Benchmark (size) Mode Cnt Score Error > Units > EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 > ns/op > EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 > ns/op > EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 > ns/op > EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 > ns/op > EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 > ns/op > EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 > ns/op Eirik Bjorsnos has updated the pull request incrementally with two additional commits since the last revision: - Adjust whitespace as suggested in review - Replace Blackhold.consume with return values - Changes: - all: https://git.openjdk.org/jdk/pull/12790/files - new: https://git.openjdk.org/jdk/pull/12790/files/6bc04db0..00253baa Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=02-03 Stats: 7 lines in 1 file changed: 1 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12790.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12790/head:pull/12790 PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Tue, 28 Feb 2023 20:00:15 GMT, Jorn Vernee wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove size restriction for structs. Add TODO for Big Endian.
>
> src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 240:
>
>> 238: __ ld(call_target_address, in_bytes(Method::from_compiled_offset()),
>> R19_method);
>> 239: __ mtctr(call_target_address);
>> 240: __ bctrl();
>
> Ok, I see. I guess there is some special purpose register called `CTR` which
> we are moving to for `bctrl` here. Does ABIv2 require that move to always
> come from `R12`? (from the comment in downcallLinker).
>
> (I'm trying to understand the requirements for possibly tweaking shared code).
There's no instruction which can use a GP reg as branch target directly. That's
why we use CTR.
In this case, using R12 is not required since we call Java and our PPC64 VM
code does not rely on it.
If we were calling C, using R12 would be required by ABI v2.
> src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 347:
>
>> 345: FunctionDescriptor* fd = (FunctionDescriptor*)fd_addr;
>> 346: fd->set_entry(fd_addr + sizeof(FunctionDescriptor));
>> 347: #endif
>
> Had to do a double take. Looks like we're not the only one who are using the
> name `FunctionDescriptor` :)
Yeah, ABI v1 (Big Endian) treats function pointers as pointers to a structure
called `FunctionDescriptor` which contains the entry point plus additional
information like a pointer to a constant table.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 286:
>
>> 284: // "no partial DW rule": Mark first stack slot to get
>> filled.
>> 285: // Note: Can only happen with forArguments = true.
>> 286: VMStorage overlappingReg = null;
>
> `overlappingReg` is initialized along all branches, so it's not needed to
> assign `null` here (and then javac will check it is actually assigned before
> use)
Removed.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/TypeClass.java
> line 66:
>
>> 64: }
>> 65:
>> 66: static boolean isHomogeneousFloatAggregate(MemoryLayout type,
>> boolean useABIv2) {
>
> Note that we had to make some changes to this routine on AArch64, since it
> didn't properly account for nested structs/unions and arrays. See:
> https://github.com/openjdk/panama-foreign/pull/780
>
> Just as a heads up, in case PPC needs changes too.
Thanks for the hint! I just added the code since it is known to be needed,
small and doesn't break any of the existing tests. I'll need to test and
possibly debug the various cases, yet.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: JDK-8302027: Port fdlibm trig functions (sin, cos, tan) to Java
On Wed, 1 Mar 2023 05:28:34 GMT, Joe Darcy wrote:
> Last and certainly not least in the port of FDLIBM to Java, the
> transcendental methods for sin, cos, and tan.
>
> Some more tests are to be written in the StrictMath directory to verify that
> the StrictMath algorihtm for sin/cos/tan is being used rather than a
> different one. However, I wanted to get the rest of the change out for review
> first.
>
> The sin/cos/tan methods are grouped together since they share the same
> argument reduction logic. Argument reduction is the process of mapping an
> argument of a function to an argument in a restricted range (and possibly
> returning some function of the reduced argument). For sin, cos, and tan,
> since they are fundamentally periodic with respect to a multiple of pi,
> argument reduction is done to find the remainder of the original argument
> with respect to pi/2.
And the shared remainder-pi-over-2 code:
$ diff -w RemPio2.c RemPio2.translit.java
1c1
< /* __ieee754_rem_pio2(x,y)
---
> /** __ieee754_rem_pio2(x,y)
6,8c6
<
< #include "fdlibm.h"
<
---
> static class RemPio2 {
12,16c10
< #ifdef __STDC__
< static const int two_over_pi[] = {
< #else
< static int two_over_pi[] = {
< #endif
---
> private static final int[] two_over_pi = {
30,34c24
< #ifdef __STDC__
< static const int npio2_hw[] = {
< #else
< static int npio2_hw[] = {
< #endif
---
> private static final int[] npio2_hw = {
53,57c43
< #ifdef __STDC__
< static const double
< #else
< static double
< #endif
---
> private static final double
69,77c55,57
< #ifdef __STDC__
< int __ieee754_rem_pio2(double x, double *y)
< #else
< int __ieee754_rem_pio2(x,y)
< double x,y[];
< #endif
< {
< double z,w,t,r,fn;
< double tx[3];
---
> static int __ieee754_rem_pio2(double x, double[] y) {
> double z = 0.0,w,t,r,fn;
> double[] tx = new double[3];
110c90
< t = fabs(x);
---
> t = Math.abs(x);
148c128,129
< __LO(z) = __LO(x);
---
> // __LO(z) = __LO(x);
> z = __LO(z, __LO(x));
150c131,132
< __HI(z) = ix - (e0<<20);
---
> // __HI(z) = ix - (e0<<20);
> z = __HI(z, ix - (e0<<20));
158c140
< n = __kernel_rem_pio2(tx,y,e0,nx,2,two_over_pi);
---
> n = KernelRemPio2.__kernel_rem_pio2(tx,y,e0,nx,2,two_over_pi);
162c144,147
< /*
---
>
> }
>
> /**
268,269c253
<
<
---
> static class KernelRemPio2 {
278c262
< #include "fdlibm.h"
---
> private static final int[] init_jk = {2,3,4,6}; /* initial value for
> jk */
280,290c264
< #ifdef __STDC__
< static const int init_jk[] = {2,3,4,6}; /* initial value for jk */
< #else
< static int init_jk[] = {2,3,4,6};
< #endif
<
< #ifdef __STDC__
< static const double PIo2[] = {
< #else
< static double PIo2[] = {
< #endif
---
> private static final double[] PIo2 = {
300,305c274
<
< #ifdef __STDC__
< static const double
< #else
< static double
< #endif
---
> static final double
311,319c280,286
< #ifdef __STDC__
< int __kernel_rem_pio2(double *x, double *y, int e0, int nx, int prec,
const int *ipio2)
< #else
< int __kernel_rem_pio2(x,y,e0,nx,prec,ipio2)
< double x[], y[]; int e0,nx,prec; int ipio2[];
< #endif
< {
< int jz,jx,jv,jp,jk,carry,n,iq[20],i,j,k,m,q0,ih;
< double z,fw,f[20],fq[20],q[20];
---
> static int __kernel_rem_pio2(double[] x, double[] y, int e0, int nx,
> int prec, final int[] ipio2) {
> int jz,jx,jv,jp,jk,carry,n,i,j,k,m,q0,ih;
> int[] iq = new int[20];
> double z,fw;
> double [] f = new double[20];
> double [] fq= new double[20];
> double [] q = new double[20];
341c308
< recompute:
---
> while(true) {
350,351c317,318
< z = scalbn(z,q0); /* actual value of z */
< z -= 8.0*floor(z*0.125);/* trim off integer >= 8 */
---
> z = Math.scalb(z,q0); /* actual value of z */
> z -= 8.0*Math.floor(z*0.125); /* trim off integer
> >= 8 */
383c350
< if(carry!=0) z -= scalbn(one,q0);
---
> if(carry!=0) z -= Math.scalb(one,q0);
400,401c367,369
< goto recompute;
< }
---
> continue;
> } else { break;}
> } else {break;}
409c377
< z = scalbn(z,-q0);
---
> z = Math.scalb(z,-q0);
419c387
< fw = scalbn(one,q0);
---
> fw = Math.scalb(one,q0);
465a434
> }
Yes, dear reader, the original C code contained a goto to implement a looping
contract. To maintain close code correspondence, I wrapped with code in
question with a "while(true){...}" and then "break"-ed or "continue"-d to
iterate or exit the loop.
$ diff -w RemPio2.translit.java RemPio2.fdlibm.java
44,53c4
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Tue, 28 Feb 2023 19:45:28 GMT, Jorn Vernee wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Remove size restriction for structs. Add TODO for Big Endian.
>
> src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 133:
>
>> 131: Register callerSP= R2, // C/C++ uses R2 as TOC, but we
>> can reuse it here
>> 132:tmp = R11_scratch1, // same as shuffle_reg
>> 133:call_target_address = R12_scratch2; // same as
>> _abi._scratch2 (ABIv2 requires this reg!)
>
> Do I understand correctly that the ABI requires the register to be used for
> the call to be `R12`? How does that make a difference? I guess in some cases
> the callee might want to know the address through which it is called? (so it
> looks at `R12`)
ABI v2 requires R12 to point to the function entry point. It is used to access
constants relative to it.
> src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 154:
>
>> 152: // (abi_reg_args is abi_minframe plus space for 8 argument register
>> spill slots)
>> 153: assert(_abi._shadow_space_bytes == frame::abi_minframe_size,
>> "expected space according to ABI");
>> 154: int allocated_frame_size = frame::abi_minframe_size +
>> MAX2(_input_registers.length(), 8) * BytesPerWord;
>
> This is hard-coding an assumption about the ABI that's being called. Ok for
> now.
>
> If it needs to be addressed in the future, it could be done by adding another
> field to `ABIDescriptor` like `min_stack_arg_bytes`, or something like that
> (which is set to zero for other ABIs). It seems to be different from
> `shadow_space` since it's also used by the caller to put stack arguments.
Yeah, I think this should be done on demand.
> src/hotspot/cpu/ppc/foreignGlobals_ppc.cpp line 229:
>
>> 227:
>> 228: void ArgumentShuffle::pd_generate(MacroAssembler* masm, VMStorage tmp,
>> int in_stk_bias, int out_stk_bias, const StubLocations& locs) const {
>> 229: Register callerSP = as_Register(tmp); // preset
>
> It looks like `tmp` is being used to hold the caller's SP. I'm guessing this
> can not be computed the same way as we do on x86 and aarch64? (based on
> `RBP`, `RFP_BIAS`)
>
> If you want, you could add another register argument to `pd_generate` that is
> just invalid/unused on other platforms. That way you could use `tmp` for the
> shuffling instead of having to go through the stack. (looks like `R0` is
> already used in some cases as a temp register)
There's no BP register. It could get loaded from the back chain, but would
still need a register. R0 is always available as scratch reg, so there's no
need to pass it. (Note that I'm not going through stack because of the lack of
registers.)
Yeah, maybe passing callerSP separately would make it better readable on PPC64.
Not sure if it's worth changing.
> src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 137:
>
>> 135: ArgumentShuffle arg_shuffle(in_sig_bt, total_in_args, out_sig_bt,
>> total_out_args, &in_conv, &out_conv, shuffle_reg);
>> 136: // The Java call uses the JIT ABI, but we also call C.
>> 137: int out_arg_area = MAX2(frame::jit_out_preserve_size +
>> arg_shuffle.out_arg_bytes(), (int)frame::abi_reg_args_size);
>
> We need `frame::abi_reg_args_size` since we call `on_entry`/`on_exit` which
> require the stack space I guess?
Correct. C functions are allowed to write into the space which is part of the
caller's frame.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: JDK-8302027: Port fdlibm trig functions (sin, cos, tan) to Java
On Wed, 1 Mar 2023 05:28:34 GMT, Joe Darcy wrote:
> Last and certainly not least in the port of FDLIBM to Java, the
> transcendental methods for sin, cos, and tan.
>
> Some more tests are to be written in the StrictMath directory to verify that
> the StrictMath algorihtm for sin/cos/tan is being used rather than a
> different one. However, I wanted to get the rest of the change out for review
> first.
>
> The sin/cos/tan methods are grouped together since they share the same
> argument reduction logic. Argument reduction is the process of mapping an
> argument of a function to an argument in a restricted range (and possibly
> returning some function of the reduced argument). For sin, cos, and tan,
> since they are fundamentally periodic with respect to a multiple of pi,
> argument reduction is done to find the remainder of the original argument
> with respect to pi/2.
And for cos:
$ diff -w Cos.c Cos.translit.java
1c1
< /* cos(x)
---
> /** cos(x)
31,41c31,34
<
< #include "fdlibm.h"
<
< #ifdef __STDC__
< double cos(double x)
< #else
< double cos(x)
< double x;
< #endif
< {
< double y[2],z=0.0;
---
> static class Cos {
> static double compute(double x) {
> double[] y = new double[2];
> double z=0.0;
56c49
< n = __ieee754_rem_pio2(x,y);
---
> n = RemPio2.__ieee754_rem_pio2(x,y);
58,60c51,53
< case 0: return __kernel_cos(y[0],y[1]);
< case 1: return -__kernel_sin(y[0],y[1],1);
< case 2: return -__kernel_cos(y[0],y[1]);
---
> case 0: return Cos.__kernel_cos(y[0],y[1]);
> case 1: return -Sin.__kernel_sin(y[0],y[1],1);
> case 2: return -Cos.__kernel_cos(y[0],y[1]);
62c55
< return __kernel_sin(y[0],y[1],1);
---
> return Sin.__kernel_sin(y[0],y[1],1);
66c59,60
< /*
---
>
> /**
100,107c94
<
< #include "fdlibm.h"
<
< #ifdef __STDC__
< static const double
< #else
< static double
< #endif
---
> private static final double
116,123c103,104
< #ifdef __STDC__
< double __kernel_cos(double x, double y)
< #else
< double __kernel_cos(x, y)
< double x,y;
< #endif
< {
< double a,hz,z,r,qx;
---
> static double __kernel_cos(double x, double y) {
> double a,hz,z,r,qx = 0.0;
137,138c118,121
< __HI(qx) = ix-0x0020; /* x/4 */
< __LO(qx) = 0;
---
> //__HI(qx) = ix-0x0020; /* x/4 */
> qx = __HI(qx, ix-0x0020);
> // __LO(qx) = 0;
> qx = __LO(qx, 0);
144a128
> }
$ diff -w Cos.translit.java Cos.fdlibm.java
31a32,33
> private Cos() {throw new UnsupportedOperationException();}
>
37c39
< /* High word of x. */
---
> // High word of x.
40,48c42,48
< /* |x| ~< pi/4 */
< ix &= 0x7fff;
< if(ix <= 0x3fe921fb) return __kernel_cos(x,z);
<
< /* cos(Inf or NaN) is NaN */
< else if (ix>=0x7ff0) return x-x;
<
< /* argument reduction needed */
< else {
---
> // |x| ~< pi/4
> ix &= 0x7fff_;
> if (ix <= 0x3fe9_21fb) {
> return __kernel_cos(x, z);
> } else if (ix >= 0x7ff0_) { // cos(Inf or NaN) is NaN
> return x-x;
> } else { // argument reduction needed
95,101c95,100
< one = 1.e+00, /* 0x3FF0, 0x */
< C1 = 4.16019037e-02, /* 0x3FA5, 0x554C */
< C2 = -1.388741095749e-03, /* 0xBF56C16C, 0x16C15177 */
< C3 = 2.48015872894767294178e-05, /* 0x3EFA01A0, 0x19CB1590 */
< C4 = -2.75573143513906633035e-07, /* 0xBE927E4F, 0x809C52AD */
< C5 = 2.08757232129817482790e-09, /* 0x3E21EE9E, 0xBDB4B1C4 */
< C6 = -1.13596475577881948265e-11; /* 0xBDA8FAE9, 0xBE8838D4 */
---
> C1 = 0x1.5554cp-5, // 4.16019037e-02
> C2 = -0x1.6c16c16c15177p-10, // -1.388741095749e-03
> C3 = 0x1.a01a019cb159p-16, // 2.48015872894767294178e-05
> C4 = -0x1.27e4f809c52adp-22, // -2.75573143513906633035e-07
> C5 = 0x1.1ee9ebdb4b1c4p-29, // 2.08757232129817482790e-09
> C6 = -0x1.8fae9be8838d4p-37; // -1.13596475577881948265e-11
106,108c105,109
< ix = __HI(x)&0x7fff;/* ix = |x|'s high word*/
< if(ix<0x3e40) { /* if x < 2**27 */
< if(((int)x)==0) return one; /* generate inexact */
---
> ix = __HI(x) & 0x7fff_; // ix = |x|'s high word
> if (ix < 0x3e40_) { // if x < 2**27
> if (((int)x) == 0) {
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v5]
On Wed, 22 Feb 2023 20:25:07 GMT, Jorn Vernee wrote:
>> Martin Doerr has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Minor cleanup.
>
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/CallArranger.java
> line 81:
>
>> 79: new VMStorage[] { f1, f2, f3, f4, f5, f6, f7, f8 }, // FP output
>> 80: new VMStorage[] { r0, r2, r3, r4, r5, r6, r7, r8, r9, r10, r11,
>> r12 }, // volatile GP
>> 81: new VMStorage[] { f0, f1, f2, f3, f4, f5, f6, f7, f8, f9, f10,
>> f11, f12, f13 }, // volatile FP
>
> Note that argument registers are assumed volatile, so they don't have to be
> duplicated here.
Removed.
> src/java.base/share/classes/jdk/internal/foreign/abi/ppc64/linux/LinuxPPC64CallArranger.java
> line 33:
>
>> 31: * PPC64 CallArranger specialized for Linux ABI.
>> 32: */
>> 33: public class LinuxPPC64CallArranger extends CallArranger {
>
> I don't really see the point in having a separate subclass with
> `CallArranger` being abstract, unless you are planning to add other
> implementations later?
>
> (edit: see also later comment in CallArranger
> https://github.com/openjdk/jdk/pull/12708#discussion_r1120753657)
AIX support will need to get implemented, yet. I guess @backwaterred will work
on it.
-
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Tue, 28 Feb 2023 16:54:50 GMT, Jorn Vernee wrote: >> Martin Doerr has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Remove size restriction for structs. Add TODO for Big Endian. > > src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 343: > >> 341: >> 342: __ flush(); >> 343: // Disassembler::decode((u_char*)start, (u_char*)__ pc(), tty); > > Leftover commented code? > > (note that the stub can also be disassembled with > `-Xlog:foreign+downcall=trace` now) Removed. Thanks for the hint! > src/hotspot/cpu/ppc/upcallLinker_ppc.cpp line 356: > >> 354: } >> 355: #endif >> 356: //blob->print_on(tty); > > Leftover commented code? Removed. - PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v5]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
Minor cleanup.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/b461d80c..75b5c78f
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12708&range=04
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12708&range=03-04
Stats: 6 lines in 3 files changed: 0 ins; 2 del; 4 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: JDK-8302027: Port fdlibm trig functions (sin, cos, tan) to Java
On Wed, 1 Mar 2023 05:28:34 GMT, Joe Darcy wrote:
> Last and certainly not least in the port of FDLIBM to Java, the
> transcendental methods for sin, cos, and tan.
>
> Some more tests are to be written in the StrictMath directory to verify that
> the StrictMath algorihtm for sin/cos/tan is being used rather than a
> different one. However, I wanted to get the rest of the change out for review
> first.
>
> The sin/cos/tan methods are grouped together since they share the same
> argument reduction logic. Argument reduction is the process of mapping an
> argument of a function to an argument in a restricted range (and possibly
> returning some function of the reduced argument). For sin, cos, and tan,
> since they are fundamentally periodic with respect to a multiple of pi,
> argument reduction is done to find the remainder of the original argument
> with respect to pi/2.
Various diffs to aid the review:
$ diff -w Sin.c Sin.translit.java
1c1
< /* sin(x)
---
> /** sin(x)
31,41c31,34
<
< #include "fdlibm.h"
<
< #ifdef __STDC__
< double sin(double x)
< #else
< double sin(x)
< double x;
< #endif
< {
< double y[2],z=0.0;
---
> static class Sin {
> static double compute(double x) {
> double[] y = new double[2];
> double z=0.0;
56c49
< n = __ieee754_rem_pio2(x,y);
---
> n = RemPio2.__ieee754_rem_pio2(x,y);
58,60c51,53
< case 0: return __kernel_sin(y[0],y[1],1);
< case 1: return __kernel_cos(y[0],y[1]);
< case 2: return -__kernel_sin(y[0],y[1],1);
---
> case 0: return Sin.__kernel_sin(y[0],y[1],1);
> case 1: return Cos.__kernel_cos(y[0],y[1]);
> case 2: return -Sin.__kernel_sin(y[0],y[1],1);
62c55
< return -__kernel_cos(y[0],y[1]);
---
> return -Cos.__kernel_cos(y[0],y[1]);
66c59,60
< /* __kernel_sin( x, y, iy)
---
>
> /** __kernel_sin( x, y, iy)
93,100c87
<
< #include "fdlibm.h"
<
< #ifdef __STDC__
< static const double
< #else
< static double
< #endif
---
> private static final double
109,115c96
< #ifdef __STDC__
< double __kernel_sin(double x, double y, int iy)
< #else
< double __kernel_sin(x, y, iy)
< double x,y; int iy; /* iy=0 if y is zero */
< #endif
< {
---
> static double __kernel_sin(double x, double y, int iy) {
126a108
> }
$ diff -w Sin.translit.java Sin.fdlibm.java
31a32,33
> private Sin() {throw new UnsupportedOperationException();}
>
37c39
< /* High word of x. */
---
> // High word of x.
40,48c42,48
< /* |x| ~< pi/4 */
< ix &= 0x7fff;
< if(ix <= 0x3fe921fb) return __kernel_sin(x,z,0);
<
< /* sin(Inf or NaN) is NaN */
< else if (ix>=0x7ff0) return x-x;
<
< /* argument reduction needed */
< else {
---
> // |x| ~< pi/4
> ix &= 0x7fff_;
> if (ix <= 0x3fe9_21fb) {
> return __kernel_sin(x, z, 0);
> } else if (ix>=0x7ff0_) { // sin(Inf or NaN) is NaN
> return x - x;
> } else { // argument reduction needed
88,94c88,93
< half = 5.e-01, /* 0x3FE0, 0x */
< S1 = -1.66324348e-01, /* 0xBFC5, 0x5549 */
< S2 = 8.332248946124e-03, /* 0x3F81, 0x1110F8A6 */
< S3 = -1.98412698298579493134e-04, /* 0xBF2A01A0, 0x19C161D5 */
< S4 = 2.75573137070700676789e-06, /* 0x3EC71DE3, 0x57B1FE7D */
< S5 = -2.50507602534068634195e-08, /* 0xBE5AE5E6, 0x8A2B9CEB */
< S6 = 1.58969099521155010221e-10; /* 0x3DE5D93A, 0x5ACFD57C */
---
> S1 = -0x1.55549p-3, // -1.66324348e-01
> S2 = 0x1.0f8a6p-7, // 8.332248946124e-03
> S3 = -0x1.a01a019c161d5p-13, // -1.98412698298579493134e-04
> S4 = 0x1.71de357b1fe7dp-19, // 2.75573137070700676789e-06
> S5 = -0x1.ae5e68a2b9cebp-26, // -2.50507602534068634195e-08
> S6 = 0x1.5d93a5acfd57cp-33; // 1.58969099521155010221e-10
99,101c98,102
< ix = __HI(x)&0x7fff;/* high word of x */
< if(ix<0x3e40) /* |x| < 2**-27 */
< {if((int)x==0) return x;}/* generate inexact */
---
> ix = __HI(x) & 0x7fff_;// high word of x
> if (ix < 0x3e40_) {// |x| < 2**-27
> if ((int)x == 0) // generate inexact
> return x;
> }
105,106c106,110
< if(iy==0) return x+v*(S1+z*r);
< else return x-((z*(half*y-v*r)-y)-v*S1);
---
> if (iy == 0) {
> return x + v
RFR: JDK-8302027: Port fdlibm trig functions (sin, cos, tan) to Java
Last and certainly not least in the port of FDLIBM to Java, the transcendental methods for sin, cos, and tan. Some more tests are to be written in the StrictMath directory to verify that the StrictMath algorihtm for sin/cos/tan is being used rather than a different one. However, I wanted to get the rest of the change out for review first. The sin/cos/tan methods are grouped together since they share the same argument reduction logic. Argument reduction is the process of mapping an argument of a function to an argument in a restricted range (and possibly returning some function of the reduced argument). For sin, cos, and tan, since they are fundamentally periodic with respect to a multiple of pi, argument reduction is done to find the remainder of the original argument with respect to pi/2. - Commit messages: - Appease jcheck. - Add tests. - Checkpoint Fdlibm.java - Merge branch 'master' into JDK-8302027 - Updates. - Initial port attempt for sin, cos, tan; not working yet. Changes: https://git.openjdk.org/jdk/pull/12800/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12800&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302027 Stats: 1976 lines in 6 files changed: 1954 ins; 12 del; 10 mod Patch: https://git.openjdk.org/jdk/pull/12800.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12800/head:pull/12800 PR: https://git.openjdk.org/jdk/pull/12800
Re: RFR: JDK-8302027: Port fdlibm trig functions (sin, cos, tan) to Java
On Wed, 1 Mar 2023 05:28:34 GMT, Joe Darcy wrote: > Last and certainly not least in the port of FDLIBM to Java, the > transcendental methods for sin, cos, and tan. > > Some more tests are to be written in the StrictMath directory to verify that > the StrictMath algorihtm for sin/cos/tan is being used rather than a > different one. However, I wanted to get the rest of the change out for review > first. > > The sin/cos/tan methods are grouped together since they share the same > argument reduction logic. Argument reduction is the process of mapping an > argument of a function to an argument in a restricted range (and possibly > returning some function of the reduced argument). For sin, cos, and tan, > since they are fundamentally periodic with respect to a multiple of pi, > argument reduction is done to find the remainder of the original argument > with respect to pi/2. The "exhausting" test comparing the results on every float argument passes for: * the transliteration port compared to the existing C code (i.e. exhausting test run with a JDK 20 build) * the java.lang.Fdlibm.java port compared to the transliteration port - PR: https://git.openjdk.org/jdk/pull/12800
Re: RFR: 8299807: newStringNoRepl should avoid copying arrays for ASCII compatible charsets [v4]
On Sat, 28 Jan 2023 19:54:32 GMT, Glavo wrote:
>> This is the javadoc of `JavaLangAccess::newStringNoRepl`:
>>
>>
>> /**
>> * Constructs a new {@code String} by decoding the specified subarray of
>> * bytes using the specified {@linkplain java.nio.charset.Charset
>> charset}.
>> *
>> * The caller of this method shall relinquish and transfer the ownership
>> of
>> * the byte array to the callee since the later will not make a copy.
>> *
>> * @param bytes the byte array source
>> * @param cs the Charset
>> * @return the newly created string
>> * @throws CharacterCodingException for malformed or unmappable bytes
>> */
>>
>>
>> It is recorded in the document that it should be able to directly construct
>> strings with parameter byte array to reduce array allocation.
>>
>> However, at present, `newStringNoRepl` always copies arrays for UTF-8 or
>> other ASCII compatible charsets.
>>
>> This PR fixes this problem.
>
> Glavo has updated the pull request incrementally with one additional commit
> since the last revision:
>
> update
Does anyone review this PR again?
-
PR: https://git.openjdk.org/jdk/pull/12119
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v4]
> Implementation of "Foreign Function & Memory API" for linux on Power (Little
> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>
> This PR does not include code for VaList support because it's supposed to get
> removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736). I've
> kept the related tests disabled for this platform and throw an exception
> instead. Note that the ABI doesn't precisely specify variable argument lists.
> Instead, it refers to `` (2.2.4 Variable Argument Lists).
>
> Big Endian support is implemented to some extend, but not complete. E.g.
> structs with size not divisible by 8 are not passed correctly (see `useABIv2`
> in CallArranger.java). Big Endian is excluded by selecting
> `ARCH.equals("ppc64le")` (CABI.java) only.
>
> There's another limitation: This PR only accepts structures with size
> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
> think arbitrary sizes are not usable on other platforms, either, because
> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>
> The ABI has some tricky corner cases related to HFA (Homogeneous Float
> Aggregate). The same argument may need to get passed in both, a FP reg and a
> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
> by the existing tests.
>
> I had to make changes to shared code and code for other platforms:
> 1. Pass type information when creating `VMStorage` objects from `VMReg`. This
> is needed for the following reasons:
> - PPC64 ABI requires integer types to get extended to 64 bit (also see
> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
> know the type or at least the bit width for that.
> - Floating point load / store instructions need the correct width to select
> between the correct IEEE 754 formats. The register representation in single
> FP registers is always IEEE 754 double precision on PPC64.
> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
> loading 4 Bytes yields different values than on Little Endian!
> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer (with
> byteSize() == 0) while running TestUpcallScope. Hence, existing size checks
> don't work (see MemorySegment.java). As a workaround, I'm just skipping the
> check in this particular case. Please check if this makes sense or if there's
> a better fix (possibly as separate RFE).
Martin Doerr has updated the pull request incrementally with one additional
commit since the last revision:
HFA: Add support for nested structures. See JDK-8300294.
-
Changes:
- all: https://git.openjdk.org/jdk/pull/12708/files
- new: https://git.openjdk.org/jdk/pull/12708/files/a4d844f7..b461d80c
Webrevs:
- full: https://webrevs.openjdk.org/?repo=jdk&pr=12708&range=03
- incr: https://webrevs.openjdk.org/?repo=jdk&pr=12708&range=02-03
Stats: 37 lines in 2 files changed: 24 ins; 0 del; 13 mod
Patch: https://git.openjdk.org/jdk/pull/12708.diff
Fetch: git fetch https://git.openjdk.org/jdk pull/12708/head:pull/12708
PR: https://git.openjdk.org/jdk/pull/12708
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Mon, 27 Feb 2023 23:11:27 GMT, Leonid Mesnik wrote: >> The solution proposed by Stefan K >> >> The startProcess() might wait forever for the expected line if the process >> exits (failed to start). It makes sense to just fail earlier in such cases. >> >> The fix also move >> 'output = new OutputAnalyzer(this.process);' >> in method xrun() to be able to try to print them in waitFor is >> failed/interrupted. > > Leonid Mesnik has updated the pull request incrementally with two additional > commits since the last revision: > > - latch != updated > - message improved We are seeing large numbers of failures after this change was integrated, so it will likely need to be backed out. - PR: https://git.openjdk.org/jdk/pull/12751
Re: 8281301: Map.of() calls with null argument should throw more contextual information.
Sorry, it appears that you have fallen into a thicket of issues. [*]
I'm usually fairly skeptical of these kinds of changes, as they often have more
overhead than is apparent. Java requires all arguments to be /evaluated/ before the
method is invoked. Evaluating a lambda includes loading and running the lambda
metafactory, spinning bytecodes, and loading another class into the JVM. This is
done even if the lambda is never /invoked/.
The unmodifiable collections are used a lot at startup time, so this means this
extra work will be done on critical code paths even before main() is called. Also,
most of this is run in interpretive mode, so we can't rely on the benefit of JIT
compilation.
But there are other reasons to avoid using lambdas in critical startup code like
this. Just for grins I applied your patch at ran it, and the result was this:
$ java -version
Error occurred during initialization of VM
java.lang.ExceptionInInitializerError
at
java.lang.invoke.MethodHandleNatives.findMethodHandleType(java.base/MethodHandleNatives.java:411)
at
java.util.ImmutableCollections$MapN.(java.base/ImmutableCollections.java:1186)
at java.util.Map.of(java.base/Map.java:1584)
at
jdk.internal.reflect.Reflection.(java.base/Reflection.java:55)
at
java.security.AccessController.doPrivileged(java.base/AccessController.java:319)
at
java.lang.reflect.AccessibleObject.(java.base/AccessibleObject.java:524)
Caused by: java.lang.NullPointerException
at
java.lang.invoke.MethodType$ConcurrentWeakInternSet.(java.base/MethodType.java:1406)
at java.lang.invoke.MethodType.(java.base/MethodType.java:231)
at
java.lang.invoke.MethodHandleNatives.findMethodHandleType(java.base/MethodHandleNatives.java:411)
at
java.util.ImmutableCollections$MapN.(java.base/ImmutableCollections.java:1186)
at java.util.Map.of(java.base/Map.java:1584)
at
jdk.internal.reflect.Reflection.(java.base/Reflection.java:55)
at
java.security.AccessController.doPrivileged(java.base/AccessController.java:319)
at
java.lang.reflect.AccessibleObject.(java.base/AccessibleObject.java:524)
Essentially the JVM crashed even before it got to print out the version message. I
don't know exactly what happened, but this kind of error often occurs if lambdas are
used "too early" during initialization. The reason is that lambdas use a bunch of
runtime infrastructure in java.lang.invoke that's written in Java. This Java code in
turn uses a variety of collections. If collections code were to use lambdas that are
evaluated at startup time, this would cause problems like initialization
circularities, references to uninitialized data, or other inexplicable errors.
That's what's happening here.
We could try to work around this by using explicit classes instead of lambdas for
the messageSupplier, but this would still mean more classes loaded and initialized
and bytecodes executed at startup time, which I'm fairly uncomfortable with given
the reasons above.
s'marks
[*] Fortunately, it seems unlikely that you will be eaten by a grue. :-)
On 2/28/23 1:14 PM, Sage Vaillancourt wrote:
Hi there. It's my first time posting to the development list, so
please forgive any formatting errors.
My proposed change is pretty small. Basically, a pattern I see with
some frequency is calling Objects.requireNonNull(object, "objectName")
before (or within) Map.of(), because otherwise there's not much of a
way to tell _which_ parameter caused the exception. Even just an index
and key/value hint would make tracking down these types of errors
easier without that bit of boilerplate.
I'd expect that any performance impact to be negligible, already being
off the happy path, here.
Diff as I'm imagining it, but very open to suggestions:
diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java
b/src/java.base/share/classes/java/util/ImmutableCollections.java
index 3de7e1d5eae..814c1a9ec1b 100644
--- a/src/java.base/share/classes/java/util/ImmutableCollections.java
+++ b/src/java.base/share/classes/java/util/ImmutableCollections.java
@@ -1103,8 +1103,8 @@ class ImmutableCollections {
private final V v0;
Map1(K k0, V v0) {
-this.k0 = Objects.requireNonNull(k0);
-this.v0 = Objects.requireNonNull(v0);
+this.k0 = Objects.requireNonNull(k0, () -> "Key in map
entry was null");
+this.v0 = Objects.requireNonNull(v0, () -> "Value in map
entry was null");
}
@Override
@@ -1183,9 +1183,9 @@ class ImmutableCollections {
for (int i = 0; i < input.length; i += 2) {
@SuppressWarnings("unchecked")
-K k = Objects.requireNonNull((K)input[i]);
+K k = Objects.requireNonNull((K)input[i], () ->
"Key for map entry " + (i / 2) + " was null");
@SuppressWarnings("unchecked")
-V v = Objects.req
Re: RFR: 8303266: Prefer ArrayList to LinkedList in JImageTask
On Wed, 1 Mar 2023 01:32:39 GMT, Roger Riggs wrote: > For this PR, go ahead and integrate, since we've already spent the budget for > reviewing it. I'm ok with this, as Roger said, since we've all already looked at it. I'm not really interested in seeing 234 followup changes removing LinkedList from all over the JDK though. - PR: https://git.openjdk.org/jdk/pull/12760
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:08:29 GMT, Eirik Bjorsnos wrote:
>> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set
>> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark
>> serves as an example of how vectorization can be useful also in the area of
>> text processing. It takes advantage of the fact that ASCII and Latin-1 were
>> designed to optimize case-twiddling operations.
>>
>> The code came about during the work on #12632, where vectorization was
>> deemed out of scope.
>>
>> Benchmark results:
>>
>>
>> Benchmark (size) Mode Cnt Score Error
>> Units
>> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493
>> ns/op
>
> Eirik Bjorsnos has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Uppercase Thorn is 0xDE
> - Update 'a' to 'A' and 'z' to 'Z' in comments
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 61:
> 59: len = a.length;
> 60: }
> 61: @Benchmark
Style: Insert one blank line between line 60-61?
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 124:
> 122: }
> 123:
> 124: public boolean scalarEqualsIgnoreCase(byte[] a, byte[] b, int len) {
Style: remove one more space between "`public boolean`" please?
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 147:
> 145: return false; // Low ASCII
> 146: }
> 147: return ( U <= 'Z' // In range A-Z
Style: remove one more space between `( U` please?
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303405: fix @returnss typo in ReflectionFactory
On Wed, 1 Mar 2023 00:24:31 GMT, Justin Lu wrote: > The following typo: `@returnss` was reported on line _482 of Reflection > Factory.java_. > > In addition to fixing that, I have replaced multiple instances of `@returns` > with `@return` in the same file. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12794
Re: RFR: 8303266: Prefer ArrayList to LinkedList in JImageTask
On Mon, 27 Feb 2023 11:33:38 GMT, Andrey Turbanov wrote: > `LinkedList` is used as a field > `jdk.tools.jimage.JImageTask.OptionsValues#jimages` > It's created, filled (with `add`) and then iterated. No removes from the head > or something like this. `ArrayList` should be preferred as more efficient and > widely used (more chances for JIT) collection. I should have expounded on the rationale for making only changes that are worth the time of the author and the reviewers. Stuart filled more of the reasons I was thinking in my terse comment. For this PR, go ahead and integrate, since we've already spent the budget for reviewing it. In choosing how you spend your time, make your efforts and the efforts of the reviewers count for something. - PR: https://git.openjdk.org/jdk/pull/12760
Re: RFR: 8303405: fix @returnss typo in ReflectionFactory
On Wed, 1 Mar 2023 00:24:31 GMT, Justin Lu wrote: > The following typo: `@returnss` was reported on line _482 of Reflection > Factory.java_. > > In addition to fixing that, I have replaced multiple instances of `@returns` > with `@return` in the same file. Marked as reviewed by martin (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12794
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of > benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves > as an example of how vectorization can be useful also in the area of text > processing. It takes advantage of the fact that ASCII and Latin-1 were > designed to optimize case-twiddling operations. > > The code came about during the work on #12632, where vectorization was deemed > out of scope. > > Benchmark results: > > > Benchmark (size) Mode Cnt Score Error > Units > EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 > ns/op > EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 > ns/op > EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 > ns/op > EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 > ns/op > EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 > ns/op > EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 > ns/op Eirik Bjorsnos has updated the pull request incrementally with two additional commits since the last revision: - Uppercase Thorn is 0xDE - Update 'a' to 'A' and 'z' to 'Z' in comments - Changes: - all: https://git.openjdk.org/jdk/pull/12790/files - new: https://git.openjdk.org/jdk/pull/12790/files/d8c0c2ed..6bc04db0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=01-02 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.org/jdk/pull/12790.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12790/head:pull/12790 PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303405: fix @returnss typo in ReflectionFactory
On Wed, 1 Mar 2023 00:24:31 GMT, Justin Lu wrote: > The following typo: `@returnss` was reported on line _482 of Reflection > Factory.java_. > > In addition to fixing that, I have replaced multiple instances of `@returns` > with `@return` in the same file. The change is okay. This is an internal class and no javadoc is generated for it. - Marked as reviewed by mchung (Reviewer). PR: https://git.openjdk.org/jdk/pull/12794
Re: RFR: 8303405: fix @returnss typo in ReflectionFactory
On Wed, 1 Mar 2023 00:24:31 GMT, Justin Lu wrote: > The following typo: `@returnss` was reported on line _482 of Reflection > Factory.java_. > > In addition to fixing that, I have replaced multiple instances of `@returns` > with `@return` in the same file. Marked as reviewed by jpai (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12794
Integrated: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed.
On Fri, 24 Feb 2023 22:15:18 GMT, Leonid Mesnik wrote: > The solution proposed by Stefan K > > The startProcess() might wait forever for the expected line if the process > exits (failed to start). It makes sense to just fail earlier in such cases. > > The fix also move > 'output = new OutputAnalyzer(this.process);' > in method xrun() to be able to try to print them in waitFor is > failed/interrupted. This pull request has now been integrated. Changeset: 1fdaf252 Author:Leonid Mesnik URL: https://git.openjdk.org/jdk/commit/1fdaf252b6375773072e665fd5d4cfb509e98f4c Stats: 29 lines in 2 files changed: 16 ins; 3 del; 10 mod 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. Reviewed-by: dholmes, rriggs - PR: https://git.openjdk.org/jdk/pull/12751
Re: RFR: 8303266: Prefer ArrayList to LinkedList in JImageTask
On Mon, 27 Feb 2023 11:33:38 GMT, Andrey Turbanov wrote: > `LinkedList` is used as a field > `jdk.tools.jimage.JImageTask.OptionsValues#jimages` > It's created, filled (with `add`) and then iterated. No removes from the head > or something like this. `ArrayList` should be preferred as more efficient and > widely used (more chances for JIT) collection. I guess I should weigh in because I'm mentioned here by tweet. The main point of "stop using LinkedList" (from the tweet) is in new code. If you're writing new code and get bogged down thinking, "Hm, should I use a LinkedList or ArrayList here?" then you should probably just use ArrayList and move on. There are few use cases where LinkedList is preferable to ArrayList, and most people won't ever see them. That doesn't justify replacing existing usages of ArrayList with LinkedList. The proximate question to ask here is, "Does this change improve the JDK?" I don't see any such justification here. Functionally the replacement is intended to be the same, so this doesn't fix any actual bug. The performance is supposedly better, but it's unclear whether this **actually** matters in this case. However, there are risks and costs associated with such changes, including the possibility of unforeseen behavioral changes that result in test failures, and the cost of reviewer time. The only benefit mentioned is that people look at the JDK and see LinkedList being used so they think it's OK to use LinkedList in their own code. This is quite tenuous. If you want people to stop using LinkedList, it will be much more effective to get to say "use ArrayList instead of LinkedList" instead of trying to remove uses of it from the JDK. - PR: https://git.openjdk.org/jdk/pull/12760
Integrated: 8282319: java.util.Locale method to stream available Locales
On Fri, 27 Jan 2023 21:48:26 GMT, Justin Lu wrote: > This PR adds a new method to java.util.Locale which returns Stream > > The contents of the Stream are composed of the same underlying elements as > Locale.getAvailableLocales(). > > This method allows streaming of the Locale array without creating a defensive > copy. This pull request has now been integrated. Changeset: 7e47d51e Author:Justin Lu Committer: Naoto Sato URL: https://git.openjdk.org/jdk/commit/7e47d51e10eef7b3bead636d20ff392e7b1dd185 Stats: 116 lines in 3 files changed: 113 ins; 1 del; 2 mod 8282319: java.util.Locale method to stream available Locales Reviewed-by: stsypanov, naoto, lancea, rriggs - PR: https://git.openjdk.org/jdk/pull/12269
Re: RFR: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal [v2]
On Tue, 28 Feb 2023 17:10:51 GMT, Brian Burkhalter wrote:
>> src/jdk.unsupported/share/classes/com/sun/nio/file/SensitivityWatchEventModifier.java
>> line 40:
>>
>>> 38: * {@code WatchService} is used only on macOS and likely to be removed
>>> 39: * in a future release when a version based on the native file event
>>> 40: * notification facility becomes available.
>>
>> I agree it's time to deprecate this extension but I think the reasoning will
>> need a few rounds to get right. As background, JDK-8285956 changed the
>> default sensitivity from medium to high in JDK 19 so the need to bump the
>> sensitivity level (and thus reducing the polling interval) has mostly gone
>> away. So maybe we should thinking about changing PollingWatchService it to
>> ignore the these modifiers (like it is done with the native
>> implementations). If we did that then it would be easy to word the
>> deprecation text as it could just say that the modifier originally provided
>> a hint to polling based WatchService implementations but is no longer used.
>
> Should the suggested change to PollingWatchService be addressed in the
> context of this PR or separately?
JDK-8303413 was created to track this change (#12795).
-
PR: https://git.openjdk.org/jdk/pull/12746
RFR: 8303405: fix @returnss typo in ReflectionFactory
The following typo: `@returnss` was reported on line _482 of Reflection Factory.java_. In addition to fixing that, I have replaced multiple instances of `@returns` with `@return` in the same file. - Commit messages: - @return tag fix for methods Changes: https://git.openjdk.org/jdk/pull/12794/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12794&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303405 Stats: 6 lines in 1 file changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12794.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12794/head:pull/12794 PR: https://git.openjdk.org/jdk/pull/12794
Re: RFR: 8298939: Refactor open/test/jdk/javax/rmi/ssl/SSLSocketParametersTest.sh to jtreg java test [v5]
On Mon, 27 Feb 2023 14:04:42 GMT, Matthew Donovan wrote: >> Removed SSLSocketParametersTest.sh script (which just called a Java file) >> and configured the java code to run directly with jtreg > > Matthew Donovan has updated the pull request incrementally with one > additional commit since the last revision: > > cleaned up exception handling Marked as reviewed by smarks (Reviewer). OK, I think this is pretty good as it stands. It achieves the original goal of converting the shell script test to a regular Java jtreg test; the shell script is removed entirely, and the Java code is cleaned up and is 20% shorter. Hooray! So I think this can go in as it stands. Optionally, if you're game to continue working on this, there are some additional cleanups that can be done. This is now digging into the Java code that was already in the test before you got involved. Specifically: 1) The `HelloImpl` and `HelloClient` classes aren't adding much value and can pretty much be deleted if the object is exported with something like: Hello stub = (Hello) UnicastRemoteObject.exportObject(new HelloImpl(), port, csf, ssf); or similar. 2) The `ClientFactory` and `ServerFactory` classes aren't adding much value. They override the create methods and print a message, but otherwise don't do anything beyond what's done in their respective superclasses. 3) The exception handling is now fairly simple, but one still needs to read through the code to figure out what is actually being tested. The `expectedException` boolean also makes things a little harder to read since it can invert the logic. I observe that the various test frameworks (such as Test-NG or JUnit) have APIs for this such as `assertThrows` or `expectThrows` which return the caught exception, allowing additional assertions to be made over it, and failing the test if the expected exception type isn't thrown. I think this would improve the test cases where an exception is expected, but it might not be worth the effort of converting to one of the frameworks. It's up to you (and your project lead) as to how to proceed. You could continue here, or in another PR, or just consider this done and move onto the next thing. - PR: https://git.openjdk.org/jdk/pull/11910
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:08:29 GMT, Eirik Bjorsnos wrote:
>> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set
>> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark
>> serves as an example of how vectorization can be useful also in the area of
>> text processing. It takes advantage of the fact that ASCII and Latin-1 were
>> designed to optimize case-twiddling operations.
>>
>> The code came about during the work on #12632, where vectorization was
>> deemed out of scope.
>>
>> Benchmark results:
>>
>>
>> Benchmark (size) Mode Cnt Score Error
>> Units
>> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890
>> ns/op
>> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735
>> ns/op
>> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493
>> ns/op
>
> Eirik Bjorsnos has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Uppercase Thorn is 0xDE
> - Update 'a' to 'A' and 'z' to 'Z' in comments
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 67:
> 65: public void scalar(Blackhole blackhole) {
> 66: blackhole.consume(scalarEqualsIgnoreCase(a, b, len));
> 67: }
If you like there is no need to explicitly use a black hole, instead declare
the benchmark method to return `boolean` and return the result of the call. JMH
will do the right thing. I find that a little more concise and easier to read.
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:08:29 GMT, Eirik Bjorsnos wrote: >> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set >> of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark >> serves as an example of how vectorization can be useful also in the area of >> text processing. It takes advantage of the fact that ASCII and Latin-1 were >> designed to optimize case-twiddling operations. >> >> The code came about during the work on #12632, where vectorization was >> deemed out of scope. >> >> Benchmark results: >> >> >> Benchmark (size) Mode Cnt Score Error >> Units >> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 >> ns/op >> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 >> ns/op >> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 >> ns/op > > Eirik Bjorsnos has updated the pull request incrementally with two additional > commits since the last revision: > > - Uppercase Thorn is 0xDE > - Update 'a' to 'A' and 'z' to 'Z' in comments Looks good to me. - Marked as reviewed by sviswanathan (Reviewer). PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v3]
On Tue, 28 Feb 2023 23:11:35 GMT, Sandhya Viswanathan wrote: >> Eirik Bjorsnos has updated the pull request incrementally with two >> additional commits since the last revision: >> >> - Uppercase Thorn is 0xDE >> - Update 'a' to 'A' and 'z' to 'Z' in comments > > Looks good to me. Thanks, @sviswa7! Would you sponsor this? - PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v2]
On Tue, 28 Feb 2023 22:19:46 GMT, Sandhya Viswanathan
wrote:
>> Eirik Bjorsnos has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Use GE, LE, NE operations instead of the lt,not combinations. Use
>> uppercase in vectorized code to match the scalar version.
>
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 85:
>
>> 83:
>> 84: // ASCII and Latin-1 were designed to optimize
>> case-twiddling operations
>> 85: ByteVector lowerA = va.or((byte) 0x20);
>
> Just curious, here you use lower whereas in scalar code upper is being used.
> Any reasons?
I can't remember the exact reason. Perhaps I thought I could get simpler range
checking of the Latin-1 code points because they are almost at the end of the
Latin-1 range.
I changed the vectorized version to use uppercase for better alignment with the
scalar version.
> test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
> line 88:
>
>> 86:
>> 87: // Determine which bytes represent ASCII or Latin-1 letters:
>> 88: VectorMask asciiLetter = lowerA.lt((byte)
>> '{').and(lowerA.lt((byte) 0x60).not());
>
> We do have GT/GE/NE etc comparison operators supported in Vector API, which
> you can use here and other places in this benchmark. e.g.
> You could do lowerA.compare(GE, (byte)0x60) instead of using lt() followed
> not(). BTW did you mean to use GT here?
>
> You will need to do the following import:
> import static jdk.incubator.vector.VectorOperators.*;
Thanks, that's a lot more readable! This checks that bytes are in the range =>
(byte) 'A', <= (byte) 'A', so I think this "not less than" was a poor man's
compare(GE, (byte) 'A') here.
I've updated the code to use LE, GE, NE where appropriate
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark [v2]
> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of > benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves > as an example of how vectorization can be useful also in the area of text > processing. It takes advantage of the fact that ASCII and Latin-1 were > designed to optimize case-twiddling operations. > > The code came about during the work on #12632, where vectorization was deemed > out of scope. > > Benchmark results: > > > Benchmark (size) Mode Cnt Score Error > Units > EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 > ns/op > EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 > ns/op > EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 > ns/op > EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 > ns/op > EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 > ns/op > EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 > ns/op Eirik Bjorsnos has updated the pull request incrementally with one additional commit since the last revision: Use GE, LE, NE operations instead of the lt,not combinations. Use uppercase in vectorized code to match the scalar version. - Changes: - all: https://git.openjdk.org/jdk/pull/12790/files - new: https://git.openjdk.org/jdk/pull/12790/files/c01a464d..d8c0c2ed Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=00-01 Stats: 18 lines in 1 file changed: 7 ins; 4 del; 7 mod Patch: https://git.openjdk.org/jdk/pull/12790.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12790/head:pull/12790 PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8297587: Upgrade JLine to 3.22.0
On Fri, 17 Feb 2023 10:20:46 GMT, Jan Lahoda wrote:
> This is a proposal to upgrade the JLine inside JDK to 3.22.0. It was done by:
> -for shared/classes, taking a diff between JLine 3.20.0 and the existing JDK
> version, copying the JLine 3.22.0 into the JDK, repackaing, re-appling the
> patch (including trailing space removal, UTF-8 characters replacement,
> addition of inputStreamWrapper), and adjusting TerminalProvider
> -for Windows, mostly copying the JLine 3.22.0 code into the JDK, and
> adjusting based on old changes
some nit comments for your consideration, approved
src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/impl/LineReaderImpl.java
line 5086:
> 5084: if (groupName) {
> 5085: Comparator groupComparator = getGroupComparator();
> 5086: Map> sorted;
Candidate and String both implement Comparable, probably that could be used to
provide a more specific common supertype
src/jdk.internal.le/share/classes/jdk/internal/org/jline/utils/NonBlocking.java
line 147:
> 145: this.input = input;
> 146: this.decoder = decoder;
> 147: this.bytes = ByteBuffer.allocate(2048);
wow, big difference in allocation size
-
Marked as reviewed by vromero (Reviewer).
PR: https://git.openjdk.org/jdk/pull/12614
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v4]
On Tue, 28 Feb 2023 13:29:49 GMT, Jim Laskey wrote:
>> Add the ability to repeatedly append char and CharSequence data to
>> StringBuilder/StringBuffer.
>
> Jim Laskey has updated the pull request incrementally with two additional
> commits since the last revision:
>
> - Remove @throws IndexOutOfBoundsException
> - Change error report to use "is negative"
Changes requested by rriggs (Reviewer).
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1835:
> 1833: * @return a reference to this object.
> 1834: *
> 1835: * @since 21
@since should be last / after \throws; here and in the other method added.
src/java.base/share/classes/java/lang/StringBuffer.java line 713:
> 711: /**
> 712: * @since 21
> 713: * @throws IllegalArgumentException {@inheritDoc}
@throws comes before @since here and in StringBuilder.
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark
On Tue, 28 Feb 2023 15:59:26 GMT, Eirik Bjorsnos wrote:
> This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of
> benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves
> as an example of how vectorization can be useful also in the area of text
> processing. It takes advantage of the fact that ASCII and Latin-1 were
> designed to optimize case-twiddling operations.
>
> The code came about during the work on #12632, where vectorization was deemed
> out of scope.
>
> Benchmark results:
>
>
> Benchmark (size) Mode Cnt Score Error
> Units
> EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718
> ns/op
> EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258
> ns/op
> EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767
> ns/op
> EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890
> ns/op
> EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540
> ns/op
> EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232
> ns/op
> EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097
> ns/op
> EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135
> ns/op
> EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735
> ns/op
> EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493
> ns/op
Thanks a lot for this micro benchmark.
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 85:
> 83:
> 84: // ASCII and Latin-1 were designed to optimize case-twiddling
> operations
> 85: ByteVector lowerA = va.or((byte) 0x20);
Just curious, here you use lower whereas in scalar code upper is being used.
Any reasons?
test/micro/org/openjdk/bench/jdk/incubator/vector/EqualsIgnoreCaseBenchmark.java
line 88:
> 86:
> 87: // Determine which bytes represent ASCII or Latin-1 letters:
> 88: VectorMask asciiLetter = lowerA.lt((byte)
> '{').and(lowerA.lt((byte) 0x60).not());
We do have GT/GE/NE etc comparison operators supported in Vector API, which you
can use here and other places in this benchmark. e.g.
You could do lowerA.compare(GE, (byte)0x60) instead of using lt() followed
not(). BTW did you mean to use GT here?
You will need to do the following import:
import static jdk.incubator.vector.VectorOperators.*;
-
PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v3]
On Tue, 28 Feb 2023 19:45:02 GMT, Stuart Marks wrote: >> Raffaello Giulietti has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) > > In concept, having APIs that search a string subrange is a fine idea. > Unfortunately the exception handling policy is an issue. We need to think > through this carefully. > > Currently, almost everything that takes some kind of String index or subrange > will throw IndexOutOfBoundsException if the arguments are ill-specified in > some fashion. There are a few notable outliers though: > > indexOf(int ch, int fromIndex) > indexOf(String str, int fromIndex) > lastIndexOf(int ch, int fromIndex) > lastIndexOf(String str, int fromIndex) > regionMatches(boolean ignoreCase, int toffset, String other, int ooffset, > int len) > regionMatches(int toffset, String other, int ooffset, int len) > > They don't throw any exceptions for ill-defined values; instead, they return > -1 or false which is indistinguishable from "not found". These APIs date all > the way back to 1.0. Note that the JDK 1.0 String wasn't internally > consistent. For example, other 1.0-era methods like `substring` throw > StringIndexOutOfBoundsException. > > The prevailing API style since that time has been to check arguments and > throw the appropriate exceptions, instead of masking ill-defined values by > returning "not found". This tends to reveal errors earlier instead of > covering them up. Compared to the existing `indexOf` methods (which specify a > single starting index), the new `indexOf` method specifies a subrange; this > allows a new class of "end < start" errors. It seems strange not to throw any > exception for this additional class of errors. > > In understand the desire to be consistent with the existing methods. Adding a > new non-throwing method seems like a mistake though. It's perpetuating an old > API design mistake for the sake of consistency, while also being inconsistent > with current API design style. > > I also don't think it's necessary to have both throwing and non-throwing > methods. > > I'd suggest returning to the original `indexOf(ch, from, to)` proposal, but > instead having it check its subrange arguments and throwing > IndexOutOfBoundsException. I don't think the exception handling inconsistency > with the existing methods is that bad. If people really, really object to it, > then maybe it would be necessary to choose a new name for the new method (and > not introduce two versions). But choosing a good name is hard, and it > introduces issues such as discoverability and questions about how the new > thing differs from the existing methods, so I'm skeptical that choosing a > different name would be any better. > > Another possible mitigation is to add API notes to highlight the unusual > behavior of the old non-throwing methods. Some of these old methods don't > mention their handling of illegal index values at all. (This could be done as > part of a separate PR.) @stuart-marks I agree. My insistence on preserving old (but bad) behavior was well intended, but I'm now convinced that the new 3 params `indexOf()` better throws on illegal indices. I'll thus add a check in its implementation, adapt the spec, remove the additional `checkedIndexOf()` method, and add API notes to the existing methods to highlight their unusual behavior w.r.t. illegal indices (assuming that adding these notes can be done in the same CSR issue). - PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v4]
On Tue, 28 Feb 2023 13:29:49 GMT, Jim Laskey wrote: >> Add the ability to repeatedly append char and CharSequence data to >> StringBuilder/StringBuffer. > > Jim Laskey has updated the pull request incrementally with two additional > commits since the last revision: > > - Remove @throws IndexOutOfBoundsException > - Change error report to use "is negative" Marked as reviewed by tvaleev (Committer). - PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8298619: java/io/File/GetXSpace.java is failing [v3]
On Wed, 22 Feb 2023 18:26:49 GMT, Brian Burkhalter wrote: >> There has definitely been a problem with quotas on Windows. I set up quotas >> on actual Windows 11 hardware and replicated the same failure. I am not sure >> how much lack of system quiescence is to blame, but there would be no harm >> in building in some robustness for lack of quiescence while we're at it. > > Spawning `df` and `diskFree` processes have been replaced with native calls. > For a reason as yet undetermined, the Windows function > `GetDiskSpaceInformationW` fails to load on Windows Server 2016 so it is > loaded dynamically and, if not found, a workaround is used. Testing in the CI against the latest commit 114857c1fbc6ea6ad0ee9ccce10cb886e8c62469 has passed more than 500 iterations on Windows with no failures. - PR: https://git.openjdk.org/jdk/pull/12397
Re: JEP-198 - Lets start talking about JSON
> From: "Brian Goetz"
> To: "Ethan McCue" , "core-libs-dev"
>
> Sent: Tuesday, February 28, 2023 8:48:00 PM
> Subject: Re: JEP-198 - Lets start talking about JSON
> As you can probably imagine, I've been thinking about these topics for quite a
> while, ever since we started working on records and pattern matching. It
> sounds
> like a lot of your thoughts have followed a similar arc to ours.
> I'll share with you some of our thoughts, but I can't be engaging in a
> detailed
> back-and-forth right now -- we have too many other things going on, and this
> isn't yet on the front burner. I think there's a right time for this work, and
> we're not quite there yet, but we'll get there soon enough and we'll pick up
> the ball again then.
> To the existential question: yes, there should be a simpler, built-in way to
> parse JSON. And, as you observe, the railroad diagram in the JSON spec is a
> graphical description of an algebraic data type. One of the great simplifying
> effects of having algebraic data types (records + sealed classes) in the
> language is that many data modeling problems collapse down to the point where
> considerably less creativity is required of an API. Here's the JSON API one
> can
> write after literally only 30 seconds of thought:
>> sealed interface JsonValue {
>> record JsonString (String s)implements JsonValue { }
>> record JsonNumber (double d)implements JsonValue { }
>> record JsonNull ()implements JsonValue { }
>> record JsonBoolean ( boolean b)implements JsonValue { }
>> record JsonArray (List< JsonValue > values)implements JsonValue { }
>> record JsonObject (Map pairs)implements JsonValue { }
>> }
> It matches the JSON spec almost literally, and you can use pattern matching to
> parse a document. (OK, there's some tiny bit of creativity here in that
> True/False have been collapsed to a single JsonBoolean type, but you get my
> point.)
> But, we're not quite ready to put this API into the JDK, because the language
> isn't *quite* there yet. Records give you nice pattern matching, but they come
> at a cost; they're very specific and have rigid ideas about initialization,
> which ripples into a number of constraints on an implementation (i.e., much
> harder to parse lazily.) So we're waiting until we have deconstruction
> patterns
> (next up on the patterns parade) so that the records above can be interfaces
> and still support pattern matching (and more flexibility in implementation,
> including using value classes when they arrive.) It's not a long hop, though.
> I agree with your assessment of streaming models; for documents too large to
> fit
> into memory, we'll let someone else provide a specialized solution. Streaming
> and fully-materialized-tree are not the only two options; there are plenty of
> points in the middle.
> As to API idioms, these can be layered. The lazy-tree model outlined above can
> be a foundation for data binding, dynamic mapping to records, jsonpath, etc.
> But once you've made the streaming-vs-materialized choice in favor of
> materialized, it's hard to imagine not having something like the above at the
> base of the tower.
> The question you raise about error handling is one that infuses pattern
> matching
> in general. Pattern matching allows us to collapse what would be a thousand
> questions -- "does key X exist? is it mapped to a number? is the number in the
> range of byte?" -- each with their own failure-handling path, into a single
> question. That's great for reliable and readable code, but it does make errors
> more opaque, because it is more like the red "check engine" light on your
> dashboard. (Something like JSONPath could generate better error messages since
> you've given it a declarative description of an assumed structural invariant.)
> But, imperative code that has to treat each structural assumption as a
> possible
> control-flow point is a disaster; we've seen too much code like this already.
> The ecosystem is big enough that there will be lots of people with strong
> opinions that "X is the only sensible way to do it" (we've already seen
> X=databinding on this thread), but the reality is that there are multiple
> overlapping audiences here, and we have to be clear which audiences we are
> prioritizing. We can have that debate when the time is right.
> So, we'll get there, but we're waiting for one or two more bits of language
> evolution to give us the substrate for the API that feels right.
> Hope this helps,
> -Brian
You can "simulate" deconstructors by using when + instanceof,
Let say we an interface with a deconstructor that can deconstruct the instance
of that interface as a tuple of points
interface Point {
record $(int x, int y) {}
$ deconstructor();
}
If there is an implementation, the deconstructor is just an implementation of
an instance method "deconstructor"
class PointImpl implements Point {
private int x;
private int y;
public PointImpl(int x, int y) {
this.x = x;
this.y =
8281301: Map.of() calls with null argument should throw more contextual information.
Hi there. It's my first time posting to the development list, so
please forgive any formatting errors.
My proposed change is pretty small. Basically, a pattern I see with
some frequency is calling Objects.requireNonNull(object, "objectName")
before (or within) Map.of(), because otherwise there's not much of a
way to tell _which_ parameter caused the exception. Even just an index
and key/value hint would make tracking down these types of errors
easier without that bit of boilerplate.
I'd expect that any performance impact to be negligible, already being
off the happy path, here.
Diff as I'm imagining it, but very open to suggestions:
diff --git a/src/java.base/share/classes/java/util/ImmutableCollections.java
b/src/java.base/share/classes/java/util/ImmutableCollections.java
index 3de7e1d5eae..814c1a9ec1b 100644
--- a/src/java.base/share/classes/java/util/ImmutableCollections.java
+++ b/src/java.base/share/classes/java/util/ImmutableCollections.java
@@ -1103,8 +1103,8 @@ class ImmutableCollections {
private final V v0;
Map1(K k0, V v0) {
-this.k0 = Objects.requireNonNull(k0);
-this.v0 = Objects.requireNonNull(v0);
+this.k0 = Objects.requireNonNull(k0, () -> "Key in map
entry was null");
+this.v0 = Objects.requireNonNull(v0, () -> "Value in map
entry was null");
}
@Override
@@ -1183,9 +1183,9 @@ class ImmutableCollections {
for (int i = 0; i < input.length; i += 2) {
@SuppressWarnings("unchecked")
-K k = Objects.requireNonNull((K)input[i]);
+K k = Objects.requireNonNull((K)input[i], () ->
"Key for map entry " + (i / 2) + " was null");
@SuppressWarnings("unchecked")
-V v = Objects.requireNonNull((V)input[i+1]);
+V v = Objects.requireNonNull((V)input[i+1], () ->
"Value for map entry " + (i / 2) + " was null");
int idx = probe(k);
if (idx >= 0) {
throw new IllegalArgumentException("duplicate key: " + k);
--
This email message may contain privileged or confidential information, and
is for the use of intended recipients only. Do not share with or forward to
additional parties except as necessary to conduct the business for which
this email (and attachments) was clearly intended. If you have received
this message in error, please immediately advise the sender by reply email
and then delete this message.
Re: RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark
On Tue, 28 Feb 2023 15:59:26 GMT, Eirik Bjorsnos wrote: > This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of > benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves > as an example of how vectorization can be useful also in the area of text > processing. It takes advantage of the fact that ASCII and Latin-1 were > designed to optimize case-twiddling operations. > > The code came about during the work on #12632, where vectorization was deemed > out of scope. > > Benchmark results: > > > Benchmark (size) Mode Cnt Score Error > Units > EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 > ns/op > EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 > ns/op > EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 > ns/op > EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 > ns/op > EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 > ns/op > EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 > ns/op > EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 > ns/op I am not an expert in Vector API but in terms of JMH structure it looks good. - Marked as reviewed by ecaspole (Committer). PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8303040: linux PPC64le: Implementation of Foreign Function & Memory API (Preview) [v3]
On Thu, 23 Feb 2023 06:18:49 GMT, Martin Doerr wrote:
>> Implementation of "Foreign Function & Memory API" for linux on Power (Little
>> Endian) according to "Power Architecture 64-Bit ELF V2 ABI Specification".
>>
>> This PR does not include code for VaList support because it's supposed to
>> get removed by [JDK-8299736](https://bugs.openjdk.org/browse/JDK-8299736).
>> I've kept the related tests disabled for this platform and throw an
>> exception instead. Note that the ABI doesn't precisely specify variable
>> argument lists. Instead, it refers to `` (2.2.4 Variable Argument
>> Lists).
>>
>> Big Endian support is implemented to some extend, but not complete. E.g.
>> structs with size not divisible by 8 are not passed correctly (see
>> `useABIv2` in CallArranger.java). Big Endian is excluded by selecting
>> `ARCH.equals("ppc64le")` (CABI.java) only.
>>
>> There's another limitation: This PR only accepts structures with size
>> divisible by 4. (An `IllegalArgumentException` gets thrown otherwise.) I
>> think arbitrary sizes are not usable on other platforms, either, because
>> `SharedUtils.primitiveCarrierForSize` only accepts powers of 2.
>>
>> The ABI has some tricky corner cases related to HFA (Homogeneous Float
>> Aggregate). The same argument may need to get passed in both, a FP reg and a
>> GP reg or stack slot (see "no partial DW rule"). This cases are not covered
>> by the existing tests.
>>
>> I had to make changes to shared code and code for other platforms:
>> 1. Pass type information when creating `VMStorage` objects from `VMReg`.
>> This is needed for the following reasons:
>> - PPC64 ABI requires integer types to get extended to 64 bit (also see
>> CCallingConventionRequiresIntsAsLongs in existing hotspot code). We need to
>> know the type or at least the bit width for that.
>> - Floating point load / store instructions need the correct width to select
>> between the correct IEEE 754 formats. The register representation in single
>> FP registers is always IEEE 754 double precision on PPC64.
>> - Big Endian also needs usage of the precise size. Storing 8 Bytes and
>> loading 4 Bytes yields different values than on Little Endian!
>> 2. It happens that a `NativeMemorySegmentImpl` is used as a raw pointer
>> (with byteSize() == 0) while running TestUpcallScope. Hence, existing size
>> checks don't work (see MemorySegment.java). As a workaround, I'm just
>> skipping the check in this particular case. Please check if this makes sense
>> or if there's a better fix (possibly as separate RFE).
>
> Martin Doerr has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Remove size restriction for structs. Add TODO for Big Endian.
This looks good overall I think, though I'll stick to my previous suggestion to
try and move more logic into Java.
Also, I recommend adding a test in `java/foreign/callarranger` similar to the
tests already found there. They call the CallGenerator directly and then check
the binding recipe. This could prove useful if we need to refactor shared code
for whatever reason, since those tests run on (almost) every platform, so can
be used to do some basic sanity checking.
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 133:
> 131: Register callerSP= R2, // C/C++ uses R2 as TOC, but we can
> reuse it here
> 132:tmp = R11_scratch1, // same as shuffle_reg
> 133:call_target_address = R12_scratch2; // same as _abi._scratch2
> (ABIv2 requires this reg!)
Do I understand correctly that the ABI requires the register to be used for the
call to be `R12`? How does that make a difference? I guess in some cases the
callee might want to know the address through which it is called? (so it looks
at `R12`)
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 154:
> 152: // (abi_reg_args is abi_minframe plus space for 8 argument register
> spill slots)
> 153: assert(_abi._shadow_space_bytes == frame::abi_minframe_size, "expected
> space according to ABI");
> 154: int allocated_frame_size = frame::abi_minframe_size +
> MAX2(_input_registers.length(), 8) * BytesPerWord;
This is hard-coding an assumption about the ABI that's being called. Ok for now.
If it needs to be addressed in the future, it could be done by adding another
field to `ABIDescriptor` like `min_stack_arg_bytes`, or something like that
(which is set to zero for other ABIs). It seems to be different from
`shadow_space` since it's also used by the caller to put stack arguments.
src/hotspot/cpu/ppc/downcallLinker_ppc.cpp line 343:
> 341:
> 342: __ flush();
> 343: // Disassembler::decode((u_char*)start, (u_char*)__ pc(), tty);
Leftover commented code?
(note that the stub can also be disassembled with
`-Xlog:foreign+downcall=trace` now)
src/hotspot/cpu/ppc/foreignGlobals_ppc.cpp line 229:
> 227:
> 228: void ArgumentShuffle::pd_generate(MacroAssembler* masm, VMStorage tmp,
>
Re: RFR: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal [v2]
On Tue, 28 Feb 2023 11:09:33 GMT, Alan Bateman wrote: >> Brian Burkhalter has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8303175: Move @deprecated above @since; tweak verbiage > > src/jdk.unsupported/share/classes/com/sun/nio/file/SensitivityWatchEventModifier.java > line 45: > >> 43: */ >> 44: >> 45: @Deprecated(since="7", forRemoval = true) > > I assume this should be "21" rather than "7" _Mea culpa_. Fixed in f00174e5f70295dff2aa2abd16559dbc81696d2d. - PR: https://git.openjdk.org/jdk/pull/12746
Re: RFR: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal [v3]
> Deprecate `SensitivityWatchEventModifier` for now instead of directly > removing it as proposed in #12626. Brian Burkhalter has updated the pull request incrementally with one additional commit since the last revision: 8303175: Change since value of @Deprecated annotation from 7 to 21 - Changes: - all: https://git.openjdk.org/jdk/pull/12746/files - new: https://git.openjdk.org/jdk/pull/12746/files/b3ea4f71..f00174e5 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12746&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12746&range=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12746.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12746/head:pull/12746 PR: https://git.openjdk.org/jdk/pull/12746
RFR: 8303401: Add a Vector API equalsIgnoreCase micro benchmark
This PR suggests we add a vectorized equalsIgnoreCase benchmark to the set of benchmarks in `org.openjdk.bench.jdk.incubator.vector`. This benchmark serves as an example of how vectorization can be useful also in the area of text processing. It takes advantage of the fact that ASCII and Latin-1 were designed to optimize case-twiddling operations. The code came about during the work on #12632, where vectorization was deemed out of scope. Benchmark results: Benchmark (size) Mode Cnt Score Error Units EqualsIgnoreCaseBenchmark.scalar 16 avgt 1520.671 ± 0.718 ns/op EqualsIgnoreCaseBenchmark.scalar 32 avgt 1546.155 ± 3.258 ns/op EqualsIgnoreCaseBenchmark.scalar 64 avgt 1568.248 ± 1.767 ns/op EqualsIgnoreCaseBenchmark.scalar 128 avgt 15 148.948 ± 0.890 ns/op EqualsIgnoreCaseBenchmark.scalar1024 avgt 15 1090.708 ± 7.540 ns/op EqualsIgnoreCaseBenchmark.vectorized 16 avgt 1521.872 ± 0.232 ns/op EqualsIgnoreCaseBenchmark.vectorized 32 avgt 1511.378 ± 0.097 ns/op EqualsIgnoreCaseBenchmark.vectorized 64 avgt 1513.703 ± 0.135 ns/op EqualsIgnoreCaseBenchmark.vectorized 128 avgt 1521.632 ± 0.735 ns/op EqualsIgnoreCaseBenchmark.vectorized1024 avgt 15 105.509 ± 7.493 ns/op - Commit messages: - Remove references to 'the oldest ASCII trick in the book' - Merge branch 'master' into vectorized-equalsignorecase - Fix "using applying" - Benchmark for exploring a vectorized latin1 equalsIgnoreCase Changes: https://git.openjdk.org/jdk/pull/12790/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12790&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303401 Stats: 152 lines in 1 file changed: 152 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12790.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12790/head:pull/12790 PR: https://git.openjdk.org/jdk/pull/12790
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v3]
On Tue, 28 Feb 2023 13:31:56 GMT, Raffaello Giulietti wrote: >> Add an `indexOf()` variant allowing to specify both a lower and an upper >> bound on the search. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) In concept, having APIs that search a string subrange is a fine idea. Unfortunately the exception handling policy is an issue. We need to think through this carefully. Currently, almost everything that takes some kind of String index or subrange will throw IndexOutOfBoundsException if the arguments are ill-specified in some fashion. There are a few notable outliers though: indexOf(int ch, int fromIndex) indexOf(String str, int fromIndex) lastIndexOf(int ch, int fromIndex) lastIndexOf(String str, int fromIndex) regionMatches(boolean ignoreCase, int toffset, String other, int ooffset, int len) regionMatches(int toffset, String other, int ooffset, int len) They don't throw any exceptions for ill-defined values; instead, they return -1 or false which is indistinguishable from "not found". These APIs date all the way back to 1.0. Note that the JDK 1.0 String wasn't internally consistent. For example, other 1.0-era methods like `substring` throw StringIndexOutOfBoundsException. The prevailing API style since that time has been to check arguments and throw the appropriate exceptions, instead of masking ill-defined values by returning "not found". This tends to reveal errors earlier instead of covering them up. Compared to the existing `indexOf` methods (which specify a single starting index), the new `indexOf` method specifies a subrange; this allows a new class of "end < start" errors. It seems strange not to throw any exception for this additional class of errors. In understand the desire to be consistent with the existing methods. Adding a new non-throwing method seems like a mistake though. It's perpetuating an old API design mistake for the sake of consistency, while also being inconsistent with current API design style. I also don't think it's necessary to have both throwing and non-throwing methods. I'd suggest returning to the original `indexOf(ch, from, to)` proposal, but instead having it check its subrange arguments and throwing IndexOutOfBoundsException. I don't think the exception handling inconsistency with the existing methods is that bad. If people really, really object to it, then maybe it would be necessary to choose a new name for the new method (and not introduce two versions). But choosing a good name is hard, and it introduces issues such as discoverability and questions about how the new thing differs from the existing methods, so I'm skeptical that choosing a different name would be any better. Another possible mitigation is to add API notes to highlight the unusual behavior of the old non-throwing methods. Some of these old methods don't mention their handling of illegal index values at all. (This could be done as part of a separate PR.) - PR: https://git.openjdk.org/jdk/pull/12600
Re: JEP-198 - Lets start talking about JSON
As you can probably imagine, I've been thinking about these topics for
quite a while, ever since we started working on records and pattern
matching. It sounds like a lot of your thoughts have followed a similar
arc to ours.
I'll share with you some of our thoughts, but I can't be engaging in a
detailed back-and-forth right now -- we have too many other things going
on, and this isn't yet on the front burner. I think there's a right
time for this work, and we're not quite there yet, but we'll get there
soon enough and we'll pick up the ball again then.
To the existential question: yes, there should be a simpler, built-in
way to parse JSON. And, as you observe, the railroad diagram in the
JSON spec is a graphical description of an algebraic data type. One of
the great simplifying effects of having algebraic data types (records +
sealed classes) in the language is that many data modeling problems
collapse down to the point where considerably less creativity is
required of an API. Here's the JSON API one can write after literally
only 30 seconds of thought:
sealed interface JsonValue{
record JsonString(String s)implements JsonValue{ }
record JsonNumber(double d)implements JsonValue{ }
record JsonNull()implements JsonValue{ }
record JsonBoolean(booleanb)implements JsonValue{ }
record JsonArray(List values)implements JsonValue{ }
record JsonObject(Map pairs)implements JsonValue{ }
}
It matches the JSON spec almost literally, and you can use pattern
matching to parse a document. (OK, there's some tiny bit of creativity
here in that True/False have been collapsed to a single JsonBoolean
type, but you get my point.)
But, we're not quite ready to put this API into the JDK, because the
language isn't *quite* there yet. Records give you nice pattern
matching, but they come at a cost; they're very specific and have rigid
ideas about initialization, which ripples into a number of constraints
on an implementation (i.e., much harder to parse lazily.) So we're
waiting until we have deconstruction patterns (next up on the patterns
parade) so that the records above can be interfaces and still support
pattern matching (and more flexibility in implementation, including
using value classes when they arrive.) It's not a long hop, though.
I agree with your assessment of streaming models; for documents too
large to fit into memory, we'll let someone else provide a specialized
solution. Streaming and fully-materialized-tree are not the only two
options; there are plenty of points in the middle.
As to API idioms, these can be layered. The lazy-tree model outlined
above can be a foundation for data binding, dynamic mapping to records,
jsonpath, etc. But once you've made the streaming-vs-materialized
choice in favor of materialized, it's hard to imagine not having
something like the above at the base of the tower.
The question you raise about error handling is one that infuses pattern
matching in general. Pattern matching allows us to collapse what would
be a thousand questions -- "does key X exist? is it mapped to a
number? is the number in the range of byte?" -- each with their own
failure-handling path, into a single question. That's great for
reliable and readable code, but it does make errors more opaque, because
it is more like the red "check engine" light on your dashboard.
(Something like JSONPath could generate better error messages since
you've given it a declarative description of an assumed structural
invariant.) But, imperative code that has to treat each structural
assumption as a possible control-flow point is a disaster; we've seen
too much code like this already.
The ecosystem is big enough that there will be lots of people with
strong opinions that "X is the only sensible way to do it" (we've
already seen X=databinding on this thread), but the reality is that
there are multiple overlapping audiences here, and we have to be clear
which audiences we are prioritizing. We can have that debate when the
time is right.
So, we'll get there, but we're waiting for one or two more bits of
language evolution to give us the substrate for the API that feels right.
Hope this helps,
-Brian
On 12/15/2022 3:30 PM, Ethan McCue wrote:
I'm writing this to drive some forward motion and to nerd-snipe those
who know better than I do into putting their thoughts into words.
There are three ways to process JSON[1]
- Streaming (Push or Pull)
- Traversing a Tree (Realized or Lazy)
- Declarative Databind (N ways)
Of these, JEP-198 explicitly ruled out providing "JAXB style type safe
data binding."
No justification is given, but if I had to insert my own: mapping the
Json model to/from the Java/JVM object model is a cursed combo of
- Huge possible design space
- Unpalatably large surface for backwards compatibility
- Serialization! Boo![2]
So for an artifact like the JDK, it probably doesn't make sense to
include. That tracks.
It won't
Re: RFR: 8303133: Update ProcessTools.startProcess(...) to exit early if process exit before linePredicate is printed. [v5]
On Mon, 27 Feb 2023 23:11:27 GMT, Leonid Mesnik wrote: >> The solution proposed by Stefan K >> >> The startProcess() might wait forever for the expected line if the process >> exits (failed to start). It makes sense to just fail earlier in such cases. >> >> The fix also move >> 'output = new OutputAnalyzer(this.process);' >> in method xrun() to be able to try to print them in waitFor is >> failed/interrupted. > > Leonid Mesnik has updated the pull request incrementally with two additional > commits since the last revision: > > - latch != updated > - message improved Marked as reviewed by rriggs (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12751
Integrated: 8303227: JniObjWithEnv should be NullablePointer compliant
On Mon, 27 Feb 2023 08:19:53 GMT, Julian Waters wrote: > JniObjWithEnv is a struct that is commonly managed by std::unique_ptr. > Although it can support managing objects that are not raw pointers, any such > objects have to be > [NullablePointers](https://en.cppreference.com/w/cpp/named_req/NullablePointer). > In the past this has [broken the build when compiler upgrades were carried > out](https://bugs.openjdk.org/browse/JDK-8244220), so we should add in the > final requirements to make the struct a true NullablePointer once and for > all, to prevent similar issues from happening in the future This pull request has now been integrated. Changeset: 50dc041e Author:Julian Waters URL: https://git.openjdk.org/jdk/commit/50dc041ee69eb62454bbf5c47a9545c6183d43d1 Stats: 12 lines in 1 file changed: 12 ins; 0 del; 0 mod 8303227: JniObjWithEnv should be NullablePointer compliant Reviewed-by: asemenyuk - PR: https://git.openjdk.org/jdk/pull/12758
Integrated: JDK-8302040: Port fdlibm sqrt to Java
On Thu, 23 Feb 2023 23:28:11 GMT, Joe Darcy wrote: > The wheel of FDLIBM porting turns a notch and sqrt comes into play. > > While the sqrt operation usually has a hardware implementation that is > intrinsified, for completeness a software implementation should be available > as well. This pull request has now been integrated. Changeset: 61e88675 Author:Joe Darcy URL: https://git.openjdk.org/jdk/commit/61e8867591f02f568776eed7211c4a83c56b8170 Stats: 875 lines in 8 files changed: 869 ins; 0 del; 6 mod 8302040: Port fdlibm sqrt to Java Reviewed-by: bpb, thartmann, aturbanov - PR: https://git.openjdk.org/jdk/pull/12736
Re: RFR: 8303227: JniObjWithEnv should be NullablePointer compliant
On Mon, 27 Feb 2023 08:19:53 GMT, Julian Waters wrote: > JniObjWithEnv is a struct that is commonly managed by std::unique_ptr. > Although it can support managing objects that are not raw pointers, any such > objects have to be > [NullablePointers](https://en.cppreference.com/w/cpp/named_req/NullablePointer). > In the past this has [broken the build when compiler upgrades were carried > out](https://bugs.openjdk.org/browse/JDK-8244220), so we should add in the > final requirements to make the struct a true NullablePointer once and for > all, to prevent similar issues from happening in the future Marked as reviewed by asemenyuk (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12758
Re: RFR: 8303232: java.util.Date.parse(String) and java.util.Date(String) don't declare thrown IllegalArgumentException
On Tue, 28 Feb 2023 00:07:05 GMT, Justin Lu wrote: > The method `java.util.Date.parse(String)` and as a result, constructor > `java.util.Date(String)` throw an `IllegalArgumentException`. This exception > is not properly referenced in the javadoc, and this PR simply adds the throws > javadoc tag to make it apparent. Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.org/jdk/pull/12779
Re: JEP-198 - Lets start talking about JSON
Link to the proxy which I forgot to include
https://gist.github.com/bowbahdoe/eb29d172351162408eab5e4ee9d84fec
On Tue, Feb 28, 2023 at 12:16 PM Ethan McCue wrote:
> As an update to my character arc, I documented and wrote up an explanation
> for the prototype library I was working on.[1]
>
> And I've gotten a good deal of feedback on reddit[2] and in private.
>
> I think its relevant to the conversation here in the sense of
>
> - There are more of rzwitserloot's objections to read on the general
> concept JSON as a built in.[3]
> - There are a lot of well reasoned objections to the manner in which I am
> interpreting a JSON tree, as well
> as objections to the usage of a tree as the core. JEP 198's current
> writeup (which I know is subject to a rewrite/retraction)
> presumes that an immutable tree would be the core data structure.
> - The peanut gallery might be interested in a "base" to implement whatever
> their take on an API should be.
>
> For that last category, I have a method-handle proxy written up for those
> who want to try the "push parser into a pull parser"
> transformation I alluded to in my first email of this thread.
>
> [1]: https://mccue.dev/pages/2-26-23-json
> [2]:
> https://www.reddit.com/r/java/comments/11cyoh1/please_try_my_json_library/
> [3]: Including one that reddit took down, but can be seen through reveddit
> https://www.reveddit.com/y/rzwitserloot/?after=t1_jacpsj6&limit=1&sort=new&show=t1_jaa3x0q&removal_status=all
>
> On Fri, Dec 16, 2022 at 6:23 PM Ethan McCue wrote:
>
>> Sidenote about "Project Galahad" - I know Graal uses json for a few
>> things including a reflection-config.json. Food for thought.
>>
>> > the java.util.log experiment shows that trying to ‘core-librarize’
>> needs that the community at large already fulfills with third party deps
>> isn’t a good move,
>>
>> I, personally, do not have much historical context for java.util.log.
>> What feels distinct about providing a JSON api is that
>> logging is an implicitly global thing. If a JSON api doesn't fill all
>> ecosystem niches, multiple can be used alongside
>> each other.
>>
>> > The root issue with JSON is that you just can’t tell how to interpret
>> any given JSON token
>>
>> The point where this could be an issue is numbers. Once something is
>> identified as a number we can
>>
>> 1. Parse it immediately. Using a long and falling back to a BigInteger.
>> For decimals its harder to know
>> whether to use a double or BigDecimal internally. In the library I've
>> been copy pasting from to build
>> a prototype that last one is an explicit option and it defaults to
>> doubles for the whole parse.
>> 2. Store the string and parse it upon request. We can still model it as a
>> Json.Number, but the
>> work of interpreting is deferred.
>>
>> But in general, making a tree of json values doesn't particularly affect
>> our ability to interpret it
>> in a certain way. That interpretation is just positional. That's just as
>> true as when making assertions
>> in the form of class structure and field types as it is when making
>> assertions in the form of code.[2]
>>
>> record Thing(Instant a) {}
>>
>> // vs.
>>
>> Decoder.field(json, "a", a -> Instant.ofEpochSecond(Decoder.long_(a)))
>>
>> If anything, using a named type as a lookup key for a deserialization
>> function is the less obvious
>> way to do this.
>>
>> > I’m not sure how to square this circle
>> > I don’t like the idea of shipping a non-data-binding JSON API in the
>> core libs.
>>
>> I think the way to cube this rhombus is to find ways to like the idea of
>> a non-data-binding JSON API. ¯\_(ツ)_/¯
>>
>> My personal journey with that is reaching its terminus here I think.
>>
>> Look on the bright side though - there are legit upsides to explicit tree
>> plucking!
>>
>> Yeah, the friction per field is slightly higher, but the relative
>> friction of custom types, or multiple construction methods for a
>> particular type, or maintaining compatibility with
>> legacy representations, or even just handling a top level list of things
>> - its much lower.
>>
>> And all that complexity - that an instant is made by looking for a long
>> or that it is parsed from a string in a
>> particular format - it lives in Java code you can see, touch, feel and
>> taste.
>>
>> I know "nobody does this"[2] but it's not that bad, actually.
>>
>> [1]: I do apologize for the code sketches consistently being "what I
>> think an interaction with a tree api should look like."
>> That is what I have been thinking about for a while so it's hard to
>> resist.
>> [2]: https://youtu.be/dOgfWXw9VrI?t=1225
>>
>> On Thu, Dec 15, 2022 at 6:34 PM Ethan McCue wrote:
>>
>>> > are pure JSON parsers really the go-to for most people?
>>>
>>> Depends on what you mean by JSON parsers and it depends on what you mean
>>> by people.
>>>
>>> To the best of my knowledge, both python and Javascript do not include
>>> streaming, databinding, or path navigation capabilities in their json
>>
Re: JEP-198 - Lets start talking about JSON
As an update to my character arc, I documented and wrote up an explanation
for the prototype library I was working on.[1]
And I've gotten a good deal of feedback on reddit[2] and in private.
I think its relevant to the conversation here in the sense of
- There are more of rzwitserloot's objections to read on the general
concept JSON as a built in.[3]
- There are a lot of well reasoned objections to the manner in which I am
interpreting a JSON tree, as well
as objections to the usage of a tree as the core. JEP 198's current writeup
(which I know is subject to a rewrite/retraction)
presumes that an immutable tree would be the core data structure.
- The peanut gallery might be interested in a "base" to implement whatever
their take on an API should be.
For that last category, I have a method-handle proxy written up for those
who want to try the "push parser into a pull parser"
transformation I alluded to in my first email of this thread.
[1]: https://mccue.dev/pages/2-26-23-json
[2]:
https://www.reddit.com/r/java/comments/11cyoh1/please_try_my_json_library/
[3]: Including one that reddit took down, but can be seen through reveddit
https://www.reveddit.com/y/rzwitserloot/?after=t1_jacpsj6&limit=1&sort=new&show=t1_jaa3x0q&removal_status=all
On Fri, Dec 16, 2022 at 6:23 PM Ethan McCue wrote:
> Sidenote about "Project Galahad" - I know Graal uses json for a few things
> including a reflection-config.json. Food for thought.
>
> > the java.util.log experiment shows that trying to ‘core-librarize’ needs
> that the community at large already fulfills with third party deps isn’t a
> good move,
>
> I, personally, do not have much historical context for java.util.log. What
> feels distinct about providing a JSON api is that
> logging is an implicitly global thing. If a JSON api doesn't fill all
> ecosystem niches, multiple can be used alongside
> each other.
>
> > The root issue with JSON is that you just can’t tell how to interpret
> any given JSON token
>
> The point where this could be an issue is numbers. Once something is
> identified as a number we can
>
> 1. Parse it immediately. Using a long and falling back to a BigInteger.
> For decimals its harder to know
> whether to use a double or BigDecimal internally. In the library I've been
> copy pasting from to build
> a prototype that last one is an explicit option and it defaults to doubles
> for the whole parse.
> 2. Store the string and parse it upon request. We can still model it as a
> Json.Number, but the
> work of interpreting is deferred.
>
> But in general, making a tree of json values doesn't particularly affect
> our ability to interpret it
> in a certain way. That interpretation is just positional. That's just as
> true as when making assertions
> in the form of class structure and field types as it is when making
> assertions in the form of code.[2]
>
> record Thing(Instant a) {}
>
> // vs.
>
> Decoder.field(json, "a", a -> Instant.ofEpochSecond(Decoder.long_(a)))
>
> If anything, using a named type as a lookup key for a deserialization
> function is the less obvious
> way to do this.
>
> > I’m not sure how to square this circle
> > I don’t like the idea of shipping a non-data-binding JSON API in the
> core libs.
>
> I think the way to cube this rhombus is to find ways to like the idea of a
> non-data-binding JSON API. ¯\_(ツ)_/¯
>
> My personal journey with that is reaching its terminus here I think.
>
> Look on the bright side though - there are legit upsides to explicit tree
> plucking!
>
> Yeah, the friction per field is slightly higher, but the relative
> friction of custom types, or multiple construction methods for a
> particular type, or maintaining compatibility with
> legacy representations, or even just handling a top level list of things -
> its much lower.
>
> And all that complexity - that an instant is made by looking for a long or
> that it is parsed from a string in a
> particular format - it lives in Java code you can see, touch, feel and
> taste.
>
> I know "nobody does this"[2] but it's not that bad, actually.
>
> [1]: I do apologize for the code sketches consistently being "what I think
> an interaction with a tree api should look like."
> That is what I have been thinking about for a while so it's hard to resist.
> [2]: https://youtu.be/dOgfWXw9VrI?t=1225
>
> On Thu, Dec 15, 2022 at 6:34 PM Ethan McCue wrote:
>
>> > are pure JSON parsers really the go-to for most people?
>>
>> Depends on what you mean by JSON parsers and it depends on what you mean
>> by people.
>>
>> To the best of my knowledge, both python and Javascript do not include
>> streaming, databinding, or path navigation capabilities in their json
>> parsers.
>>
>>
>> On Thu, Dec 15, 2022 at 6:26 PM Ethan McCue wrote:
>>
>>> > The 95%+ use case for working with JSON for your average java coder is
>>> best done with data binding.
>>>
>>> To be brave yet controversial: I'm not sure this is neccesarily true.
>>>
>>> I will elaborate and respond to the
Re: RFR: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal [v2]
On Tue, 28 Feb 2023 11:24:59 GMT, Alan Bateman wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8303175: Move @deprecated above @since; tweak verbiage
>
> src/jdk.unsupported/share/classes/com/sun/nio/file/SensitivityWatchEventModifier.java
> line 40:
>
>> 38: * {@code WatchService} is used only on macOS and likely to be removed
>> 39: * in a future release when a version based on the native file event
>> 40: * notification facility becomes available.
>
> I agree it's time to deprecate this extension but I think the reasoning will
> need a few rounds to get right. As background, JDK-8285956 changed the
> default sensitivity from medium to high in JDK 19 so the need to bump the
> sensitivity level (and thus reducing the polling interval) has mostly gone
> away. So maybe we should thinking about changing PollingWatchService it to
> ignore the these modifiers (like it is done with the native implementations).
> If we did that then it would be easy to word the deprecation text as it could
> just say that the modifier originally provided a hint to polling based
> WatchService implementations but is no longer used.
Should the suggested change to PollingWatchService be addressed in the context
of this PR or separately?
-
PR: https://git.openjdk.org/jdk/pull/12746
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 14:47:52 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/InstructionData.java
> line 47:
>
>> 45: * InstructionData
>> 46: */
>> 47: public class InstructionData {
>
> As CodeImpl seems to be the only client of this, I wonder if we could move
> the static cache in there?
yes, will fix it
> src/java.base/share/classes/jdk/internal/classfile/impl/LabelImpl.java line
> 65:
>
>> 63:
>> 64: public int getContextInfo() {
>> 65: return contextInfo;
>
> This seems to be the BCI - should we change names to reflect that?
> (`contextInfo` seems very vague)
yes, will fix it
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 14:41:16 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/ConcreteEntry.java
> line 58:
>
>> 56: import jdk.internal.classfile.jdktypes.PackageDesc;
>> 57:
>> 58: public abstract sealed class ConcreteEntry {
>
> Why the name `concrete` ? Am I missing something (e.g. existence of
> "non-concrete" pool entries?)
XyzEntryImpl naming convention would be better, I'll adjust it.
> src/java.base/share/classes/jdk/internal/classfile/impl/DirectCodeBuilder.java
> line 658:
>
>> 656: mruParent = parent;
>> 657: mruParentTable = table;
>> 658: return mruParentTable[lab.getContextInfo()] - 1;
>
> Am I correct that this code can misbehave e.g. if `computeIfAbsent` ends up
> creating a brand new `table` array - in which case, all array elements are
> set to `0` - meaning we end up returning `-1`. Is that what we want?
Yes, -1 indicates the label has not been resolved yet.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v5]
On Tue, 28 Feb 2023 11:09:37 GMT, Viktor Klang wrote: >> I noticed when looking at the code that there was no real need to use a CHM >> to perform the tracking of activation in an ordered fashion on >> ForEachOrderedTask, but instead a VarHandle can be used, reducing >> allocations and indirection. > > Viktor Klang has refreshed the contents of this pull request, and previous > commits have been removed. The incremental views will show differences > compared to the previous content of the PR. The pull request contains one new > commit since the last revision: > > Updating copyright header of ForEachOps.java and removing unnecessary > suppression of an unchecked cast. src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > 511: // of right subtree (if any, which can be this task's right > sibling) > 512: // > 513: var leftDescendant = (ForEachOrderedTask T>)NEXT.getAndSet(this, (ForEachOrderedTask)null); The reference cast on the argument is not required. `VarHandle`'s by default have `MethodHandle` invoke semantics (but without the throwing Throwable): https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#invoke VarHandle's have been engineered so such reference casts on the arguments can be optimized away. This makes them much easier to use than MethodHandles. - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 13:43:32 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java
> line 54:
>
>> 52: */
>> 53: public class BytecodeHelpers {
>> 54: //public static Map constantsToOpcodes = new
>> HashMap<>(16);
>
> Should this be removed?
yes, fixed, thanks.
> src/java.base/share/classes/jdk/internal/classfile/impl/BytecodeHelpers.java
> line 56:
>
>> 54: //public static Map constantsToOpcodes = new
>> HashMap<>(16);
>> 55:
>> 56: public BytecodeHelpers() {
>
> Should this also be removed (same as default constructor) ?
I set the constructor to private as there are no instances created.
> src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line 30:
>
>> 28: * An open-chain multimap used to map nonzero hashes to indexes (of
>> either CP
>> 29: * elements or BSM entries). Code transformed from public domain
>> implementation
>> 30: *
>> (http://java-performance.info/implementing-world-fastest-java-int-to-int-hash-map/).
>
> Could not open this link - seems to redirect to main page
Direct link does not work, I've fixed it to point to the home page.
> src/java.base/share/classes/jdk/internal/classfile/impl/EntryMap.java line
> 192:
>
>> 190: return (int)s;
>> 191: }
>> 192: }
>
> Watch for newlines
fixed, thanks.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v28]
> This is root pull request with Classfile API implementation, tests and > benchmarks initial drop into JDK. > > Following pull requests consolidating JDK class files parsing, generating, > and transforming ([JDK-8294957](https://bugs.openjdk.org/browse/JDK-8294957)) > will chain to this one. > > Classfile API development is tracked at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-branch > > Development branch of consolidated JDK class files parsing, generating, and > transforming is at: > https://github.com/openjdk/jdk-sandbox/tree/classfile-api-dev-branch > > Classfile API [JEP](https://bugs.openjdk.org/browse/JDK-8280389) and [online > API > documentation](https://htmlpreview.github.io/?https://raw.githubusercontent.com/openjdk/jdk-sandbox/classfile-api-javadoc-branch/doc/classfile-api/javadoc/java.base/jdk/internal/classfile/package-summary.html) > is also available. > > Please take you time to review this non-trivial JDK addition. > > Thank you, > Adam Adam Sotona has updated the pull request incrementally with two additional commits since the last revision: - BytecodeHelpers fix - javadoc and long lines fixes - Changes: - all: https://git.openjdk.org/jdk/pull/10982/files - new: https://git.openjdk.org/jdk/pull/10982/files/7002d719..58c9d2c0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=27 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10982&range=26-27 Stats: 35 lines in 4 files changed: 15 ins; 1 del; 19 mod Patch: https://git.openjdk.org/jdk/pull/10982.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10982/head:pull/10982 PR: https://git.openjdk.org/jdk/pull/10982
Integrated: 8303350: Fix mistyped {@code}
On Tue, 28 Feb 2023 13:31:06 GMT, Pavel Rappo wrote:
> Please review this trivial fix.
This pull request has now been integrated.
Changeset: dc5ea6ae
Author:Pavel Rappo
URL:
https://git.openjdk.org/jdk/commit/dc5ea6aeb500d531b4ba49c8e95bf97744cc6c33
Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod
8303350: Fix mistyped {@code}
Reviewed-by: jpai
-
PR: https://git.openjdk.org/jdk/pull/12784
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Tue, 28 Feb 2023 14:33:58 GMT, Adam Sotona wrote:
>> src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
>> line 38:
>>
>>> 36: private final List> attributes = new ArrayList<>();
>>> 37:
>>> 38: public AttributeHolder() {
>>
>> default constructor
>
> yes, it is a default constructor, do you suggest to remove it?
Yes, it's inside the implementation, it's not like it's used as a placeholder
for javadoc or anything?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Tue, 28 Feb 2023 09:05:44 GMT, John Hendrikx wrote:
>> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1903:
>>
>>> 1901: throw new OutOfMemoryError("Required length exceeds
>>> implementation limit");
>>> 1902: }
>>> 1903: int total = count * length;
>>
>> We may avoid division if we use the long type:
>>
>>
>> long totalLong = ((long) count) * length;
>> if (totalLong > Integer.MAX_VALUE - offset) {
>> throw new OutOfMemoryError("Required length exceeds implementation
>> limit");
>> }
>> int total = (int) totalLong;
>>
>>
>> Should be faster.
>
> I'm a bit surprised this (and the original code) is throwing
> `OutOfMemoryError` when running into the max array size. It is not truly an
> out of memory error, but being an `Error`, it would evade standard catch
> `Exception` blocks. I would think this is more of an `IllegalStateException`
> or perhaps something array specific.
>
> `OutOfMemoryError` is documented pretty specifically that an object
> allocation failed after exhaustive GC, but no allocation or GC happened:
>
>>Thrown when the Java Virtual Machine cannot allocate an object because it is
>>out of memory, and no more memory could be made available by the garbage
>>collector.
>
> ... which is not happening here at all. This leads me to believe the error is
> not used correctly here.
>
> Other reasons I find it surprising: `StringBuilder` is not documented to
> throw this anywhere, it seems to just leak through from various methods
> implemented by `AbstractStringBuilder`.
OOME is used for a number of not-strictly out of memory conditions, for example
off-heap allocation and allocations that can't succeed because they exceed
implementation limits.
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8303350: Fix mistyped {@code}
On Tue, 28 Feb 2023 13:31:06 GMT, Pavel Rappo wrote: > Please review this trivial fix. The changes look fine to me. - Marked as reviewed by jpai (Reviewer). PR: https://git.openjdk.org/jdk/pull/12784
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 13:47:04 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/impl/AttributeHolder.java
> line 38:
>
>> 36: private final List> attributes = new ArrayList<>();
>> 37:
>> 38: public AttributeHolder() {
>
> default constructor
yes, it is a default constructor, do you suggest to remove it?
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 13:40:49 GMT, Maurizio Cimadamore
wrote:
>> Adam Sotona has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> added 4-byte Unicode text to Utf8EntryTest
>
> src/java.base/share/classes/jdk/internal/classfile/constantpool/ConstantPoolBuilder.java
> line 75:
>
>> 73: static ConstantPoolBuilder of(ClassModel classModel) {
>> 74: ClassReader reader = (ClassReader) classModel.constantPool();
>> 75: return reader.optionValue(Classfile.Option.Key.CP_SHARING)
>
> Question: the fact that, when transforming, the new constant pool is the same
> as the old one, plus some "appended" entries at the end, is a pretty crucial
> property to ensure that existing code that is not transformed (e.g. unknown
> attributes) remains legal after transformation. But it seems that, if the
> `CP_SHARING` option is not passed, we build a brand new constant pool, in
> which case there's no guarantee that old indices will still point to the same
> position. Doesn't that lead to issue with unknown attributes?
>
> I guess my question is: shouldn't the semantics properties/guarantees of a
> classfile transform be specified regardless of the options being passed in?
> E.g. it's ok if using different options ends up with different performance
> model, but I'm a bit worried if options can affect correctness of the
> transform?
If you propose `constantPoolSharing(false)` should automatically imply
`processUnknownAttributes(false)` - it would avoid potential invalid CP entries
in the unknown attributes and we may consider that dependency between the two
options.
However on the other side there might be unknown attributes completely
independent of CP and for such the transformation is safe. Strong options
implication would force users to create custom attribute mappers for such
attributes.
Current options independence gives user power to configure transformations
flexibly, however on the other side it allows to produce classes with invalid
CP entries in unknown attributes.
-
PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8294982: Implementation of Classfile API [v20]
On Thu, 16 Feb 2023 12:55:06 GMT, Maurizio Cimadamore wrote: >> Adam Sotona has updated the pull request incrementally with one additional >> commit since the last revision: >> >> added 4-byte Unicode text to Utf8EntryTest > > src/java.base/share/classes/jdk/internal/classfile/impl/SplitConstantPool.java > line 217: > >> 215: } >> 216: }; >> 217: // Doing a full scan here yields fall-off-the-cliff >> performance results, > > Understanding checkpoint: the parent is a class reader, which holds some > class bytes. The class reader is set up in a way that constant pool entries > are read "on-demand" - that is, if I ask the class reader "give me CP entry > 42", the classreader will only build that entry (assuming 42 is a valid > index) and give me back that entry. Also, the class reader will save the read > constant in an internal array, to avoid re-reading the classfile bytes > multiple times. > > So, what this code here does is: when we create an entry map, we populate the > map with the entries from the parent reader - but we only add entries that > were already looked up (e.g. we do not bother reading _all_ entries, and > adding all of them to the entry map). > > But, when `findEntry` is called, if we see that we can't find the entry, and > we know that there might be constant pool entries from the parent reader > still unread, we force a full scan of the parent reader pool and try again. > > Correct? Right, it is a multi-stage inflation, where partial-only inflation improves performance of frequent use cases significantly and full scan is a fall-back. - PR: https://git.openjdk.org/jdk/pull/10982
Re: RFR: 8303198: System and Runtime.exit() resilience to logging errors
On Tue, 28 Feb 2023 02:45:36 GMT, David Holmes wrote: > But does that logging include the thread identity? If multiple threads can > race to exit and all log, then the developer/user needs to know which logging > came from which thread. That's really up to the Logger and its configuration. If j.u.logging is used then formatters can be configured to put the thread ID into the log records. With 3rd party logging libraries there seems to be several choices, like %t for the thread name. The main usage for this logging is to be able to find code in tests, plugins, etc. that is calling System.exit and causing the test runner or container to exit. So I think it's less about "which thread" and more about "which code". - PR: https://git.openjdk.org/jdk/pull/12770
Re: RFR: JDK-8302040: Port fdlibm sqrt to Java [v5]
On Mon, 27 Feb 2023 18:26:51 GMT, Joe Darcy wrote: >> The wheel of FDLIBM porting turns a notch and sqrt comes into play. >> >> While the sqrt operation usually has a hardware implementation that is >> intrinsified, for completeness a software implementation should be available >> as well. > > Joe Darcy 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 six additional commits since > the last revision: > > - Enable intrinsic for StrictMath.sqrt; thanks to Tobias Hartmann for > assistance with the HotSpot updates. > - Merge branch 'master' into JDK-8302040 > - Implement review feedback. > - Add one one unsigned shift comment. > - Respond to review feedback. > - JDK-8302040: Port fdlibm sqrt to Java Marked as reviewed by aturbanov (Committer). - PR: https://git.openjdk.org/jdk/pull/12736
RFR: 8303350: Fix mistyped {@code}
Please review this trivial fix. - Commit messages: - Initial commit Changes: https://git.openjdk.org/jdk/pull/12784/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12784&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8303350 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.org/jdk/pull/12784.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12784/head:pull/12784 PR: https://git.openjdk.org/jdk/pull/12784
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v3]
On Tue, 28 Feb 2023 13:31:56 GMT, Raffaello Giulietti wrote: >> Add an `indexOf()` variant allowing to specify both a lower and an upper >> bound on the search. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [CSR](https://bugs.openjdk.org/browse/JDK-8302680) updated accordingly. - PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v2]
On Fri, 24 Feb 2023 17:45:23 GMT, Raffaello Giulietti wrote: >> Add an `indexOf()` variant allowing to specify both a lower and an upper >> bound on the search. > > Raffaello Giulietti has updated the pull request incrementally with one > additional commit since the last revision: > > 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) Added a `checkedIndexOf(int ch, int fromIndex, int toIndex)` variant that throws if `0 <= fromIndex <= toIndex <= this.length()` is not met. Otherwise it behaves like `indexOf(int ch, int fromIndex, int toIndex)`. - PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) [v3]
> Add an `indexOf()` variant allowing to specify both a lower and an upper > bound on the search. Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 8302590: Add String.indexOf(int ch, int fromIndex, int toIndex) - Changes: - all: https://git.openjdk.org/jdk/pull/12600/files - new: https://git.openjdk.org/jdk/pull/12600/files/fdb39c47..d627e558 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12600&range=02 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12600&range=01-02 Stats: 72 lines in 2 files changed: 72 ins; 0 del; 0 mod Patch: https://git.openjdk.org/jdk/pull/12600.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12600/head:pull/12600 PR: https://git.openjdk.org/jdk/pull/12600
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Tue, 28 Feb 2023 10:54:14 GMT, Alan Bateman wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Optimize for empty CharSequence
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1837:
>
>> 1835: * @since 21
>> 1836: * @throws IllegalArgumentException if {@code count} is less than
>> zero
>> 1837: * @throws IndexOutOfBoundsException if the result overflows the
>> buffer
>
> IOOBE is for cases when an index is out of range so maybe that should be
> looked at again.
See above. Removed.
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1872:
>
>> 1870: * {@code CharSequence} length.
>> 1871: *
>> 1872: * @param cs a {@code CharSequence}
>
> append(CharSequence, int, int) has "If s is null, then this method appends
> characters as if the s parameter was a sequence containing the four
> characters "null".". It looks like the proposal is to allow cs be null, in
> which case you'll need similar javadoc. In passing, append(CharSequence) is
> inheriting the method description from Appendable so it has the wrong
> parameter name.
Changing
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v4]
> Add the ability to repeatedly append char and CharSequence data to > StringBuilder/StringBuffer. Jim Laskey has updated the pull request incrementally with two additional commits since the last revision: - Remove @throws IndexOutOfBoundsException - Change error report to use "is negative" - Changes: - all: https://git.openjdk.org/jdk/pull/12728/files - new: https://git.openjdk.org/jdk/pull/12728/files/01ffb19b..9a157ce7 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12728&range=03 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12728&range=02-03 Stats: 13 lines in 3 files changed: 3 ins; 6 del; 4 mod Patch: https://git.openjdk.org/jdk/pull/12728.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12728/head:pull/12728 PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Tue, 28 Feb 2023 07:50:18 GMT, Tagir F. Valeev wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Optimize for empty CharSequence
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1879:
>
>> 1877: * @since 21
>> 1878: * @throws IllegalArgumentException if {@code count} is less than
>> zero
>> 1879: * @throws IndexOutOfBoundsException if the result overflows the
>> buffer
>
> Can `IndexOutOfBoundsException` be really thrown? We don't have any index
> here. From the implementation I see that only `OutOfMemoryError` and
> `IllegalArgumentException` can be thrown.
>
> Also, I see a small descrepancy between messages in this method and
> `String.repeat`. The latter has:
>
>
> * @throws IllegalArgumentException if the {@code count} is
> * negative.
> *
> * @since 11
> */
> public String repeat(int count) {
> if (count < 0) {
> throw new IllegalArgumentException("count is negative: " + count);
> }
>
>
> Probably it's better to reuse the same wording ("is negative" instead of "is
> less than zero") for consistency?
You are correct about `IndexOutOfBoundsException`. Earlier versions used
methods that threw `IndexOutOfBoundsException`. Also, changing to "is negative"
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8303227: JniObjWithEnv should be NullablePointer compliant
On Mon, 27 Feb 2023 08:19:53 GMT, Julian Waters wrote: > JniObjWithEnv is a struct that is commonly managed by std::unique_ptr. > Although it can support managing objects that are not raw pointers, any such > objects have to be > [NullablePointers](https://en.cppreference.com/w/cpp/named_req/NullablePointer). > In the past this has [broken the build when compiler upgrades were carried > out](https://bugs.openjdk.org/browse/JDK-8244220), so we should add in the > final requirements to make the struct a true NullablePointer once and for > all, to prevent similar issues from happening in the future @alexeysemenyukoracle Sorry for the ping, just wanted to avoid the PR getting buried since I don't know any other jpackage reviewers - PR: https://git.openjdk.org/jdk/pull/12758
Re: RFR: 8303175: (fs) Deprecate com.sun.nio.file.SensitivityWatchEventModifier for removal [v2]
On Fri, 24 Feb 2023 19:04:33 GMT, Brian Burkhalter wrote:
>> Deprecate `SensitivityWatchEventModifier` for now instead of directly
>> removing it as proposed in #12626.
>
> Brian Burkhalter has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8303175: Move @deprecated above @since; tweak verbiage
src/jdk.unsupported/share/classes/com/sun/nio/file/SensitivityWatchEventModifier.java
line 40:
> 38: * {@code WatchService} is used only on macOS and likely to be removed
> 39: * in a future release when a version based on the native file event
> 40: * notification facility becomes available.
I agree it's time to deprecate this extension but I think the reasoning will
need a few rounds to get right. As background, JDK-8285956 changed the default
sensitivity from medium to high in JDK 19 so the need to bump the event (and
thus reducing the polling interval) has mostly gone away. So maybe we should
thinking about changing PollingWatchService it to ignore the these modifiers
(like it is done with the native implementations). If we did that then it would
be easy to word the deprecation text as it could just say that the modifier
originally provided a hint to polling based WatchService implementations but is
no longer used.
src/jdk.unsupported/share/classes/com/sun/nio/file/SensitivityWatchEventModifier.java
line 45:
> 43: */
> 44:
> 45: @Deprecated(since="7", forRemoval = true)
I assume this should be "21" rather than "7"
-
PR: https://git.openjdk.org/jdk/pull/12746
Re: RFR: JDK-8297605 DelayQueue javadoc is confusing
On Mon, 27 Feb 2023 20:09:12 GMT, Martin Buchholz wrote:
>> Here's my attempt:
>>
>>
>> * An unbounded {@linkplain BlockingQueue blocking queue} of {@link Delayed}
>> * elements, in which an element generally becomes eligible for removal when
>> its
>> * delay has expired.
>> *
>> * An element is considered expired when its {@code
>> * getDelay(TimeUnit.NANOSECONDS)} method would return a value less than or
>> * equal to zero.
>> *
>> * An element is considered the head of the queue if it is the
>> * element with the earliest expiration time, whether in the past or the
>> future,
>> * if there is such an element.
>> *
>> * An element is considered the expired head of the queue if it
>> is
>> * the expired element with the earliest expiration time in the
>> * past, if there is such an element.
>> * The expired head, when present, is also the head.
>> *
>> * While this class implements the {@code BlockingQueue} interface, it
>> * intentionally violates the general contract of {@code BlockingQueue}, in
>> that
>> * the following methods disregard the presence of unexpired elements and
>> only
>> * ever remove the expired head:
>> *
>> *
>> * {@link #poll()}
>> * {@link #poll(long,TimeUnit)}
>> * {@link #take()}
>> * {@link #remove()}
>> *
>> *
>> * All other methods operate on both expired and unexpired elements. For
>> * example, the {@code size} method returns the count of all elements.
>> Method
>> * {@link peek()} may return the non-null head even when {@code
>> * take()} would block waiting for that element to expire.
>> *
>> * This queue does not permit null elements.
>
>> @Martin-Buchholz Martin, how would you like to proceed with your proposed
>> wording, would you prefer a suggested edit to this PR, do a separate PR, or
>> otherwise? /cc @AlanBateman (any recommendation, Alan? thinking )
>
> Talked me into it - I will dust off my github/skara skillz and make a new PR.
>
>
> I wonder if there's now a way to override javadoc for remove() without
> creating a new method body.
@Martin-Buchholz I guess `remove` could be overridden and just delegate to
`super`?
-
PR: https://git.openjdk.org/jdk/pull/12729
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v5]
> I noticed when looking at the code that there was no real need to use a CHM > to perform the tracking of activation in an ordered fashion on > ForEachOrderedTask, but instead a VarHandle can be used, reducing allocations > and indirection. Viktor Klang has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR. The pull request contains one new commit since the last revision: Updating copyright header of ForEachOps.java and removing unnecessary suppression of an unchecked cast. - Changes: - all: https://git.openjdk.org/jdk/pull/12320/files - new: https://git.openjdk.org/jdk/pull/12320/files/b4b241e2..a5c6b0d0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=12320&range=03-04 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/12320.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12320/head:pull/12320 PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: 8303232: java.util.Date.parse(String) and java.util.Date(String) don't declare thrown IllegalArgumentException
On Tue, 28 Feb 2023 00:07:05 GMT, Justin Lu wrote: > The method `java.util.Date.parse(String)` and as a result, constructor > `java.util.Date(String)` throw an `IllegalArgumentException`. This exception > is not properly referenced in the javadoc, and this PR simply adds the throws > javadoc tag to make it apparent. Thanks for getting the CSR submitted Justin (and it is already approved). - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.org/jdk/pull/12779
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Mon, 27 Feb 2023 13:30:47 GMT, Jim Laskey wrote:
>> Add the ability to repeatedly append char and CharSequence data to
>> StringBuilder/StringBuffer.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Optimize for empty CharSequence
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1837:
> 1835: * @since 21
> 1836: * @throws IllegalArgumentException if {@code count} is less than
> zero
> 1837: * @throws IndexOutOfBoundsException if the result overflows the
> buffer
IOOBE is for cases when an index is out of range so maybe that should be looked
at again.
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v4]
On Tue, 28 Feb 2023 09:04:21 GMT, ExE Boss wrote: >> Viktor Klang has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains three commits: >> >> - Updating copyright header of ForEachOps.java and removing unnecessary >> suppression of an unchecked cast. >> - Write the initial value of the next reference without using the VarHandle >> - JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask > > src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > >> 511: // of right subtree (if any, which can be this task's right >> sibling) >> 512: // >> 513: var leftDescendant = (ForEachOrderedTask> T>)NEXT.getAndSet(this, null); > > Casting the `null` is required for the resolved method descriptor to be > `(ForEachOrderedTask, ForEachOrderedTask)ForEachOrderedTask` instead of > `(ForEachOrderedTask, Object)ForEachOrderedTask`, which prevents unnecessary > type conversion `LambdaForm`s from being introduced and allows > [`VarHandle::withInvokeExactBehavior`] to be used: > Suggestion: > > var leftDescendant = (ForEachOrderedTask) > NEXT.getAndSet(this, (ForEachOrderedTask) null); > > > [`VarHandle::withInvokeExactBehavior`]: > https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#withInvokeExactBehavior() @ExE-Boss Ah, sorry, it was meant to be there. :) - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Mon, 27 Feb 2023 13:30:47 GMT, Jim Laskey wrote:
>> Add the ability to repeatedly append char and CharSequence data to
>> StringBuilder/StringBuffer.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Optimize for empty CharSequence
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1872:
> 1870: * {@code CharSequence} length.
> 1871: *
> 1872: * @param cs a {@code CharSequence}
append(CharSequence, int, int) has "If s is null, then this method appends
characters as if the s parameter was a sequence containing the four characters
"null".". It looks like the proposal is to allow cs be null, in which case
you'll need similar javadoc. In passing, append(CharSequence) is inheriting the
method description from Appendable so it has the wrong parameter name.
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8303266: Prefer ArrayList to LinkedList in JImageTask
On Mon, 27 Feb 2023 22:59:42 GMT, Roger Riggs wrote: >> `LinkedList` is used as a field >> `jdk.tools.jimage.JImageTask.OptionsValues#jimages` >> It's created, filled (with `add`) and then iterated. No removes from the >> head or something like this. `ArrayList` should be preferred as more >> efficient and widely used (more chances for JIT) collection. > > IMHO, there is not enough value in this change to continue the review. It > should be closed without integrating. @RogerRiggs I believe it's [common](https://twitter.com/stuartmarks/status/1241522641707532289) [knowledge](https://mobile.twitter.com/kuksenk0/status/1299412730672304128) that LinkedList is not a best collection nowadays. OpenJDK source code is a reference implementation, and people look at its code. And when people see that OpenJDK guys use LinkedList (As I can see it's still have 234 usages in OpenJDK), it's confusing and creates questions. I believe cleaning OpenJDK repository from outdated usages could speed up LinkedList obsoleteness. - PR: https://git.openjdk.org/jdk/pull/12760
Re: RFR: 8302872: Speed up StringLatin1.regionMatchesCI_UTF16
On Sat, 18 Feb 2023 18:22:49 GMT, Eirik Bjorsnos wrote: > This PR continues the efforts from #12632 to speed up case-insensitive string > matching. > > We now tackle case-insensitive comparison of mixed-coder strings, implemented > in `StringLatin1.regionMatchesCI_UTF16` > > Key insights: > > - If the UTF16 code point is also in latin1 range, we can leverage > improvements from 12632 directly by calling > `CharacterDataLatin1.equalsIgnoreCase` > - There are exactly 7 non-latin1 Unicode code points which case fold into the > latin1 range. We can special-case our comparison of these code points by > adding the method `CharacterDataLatin1.latin1CaseFold`. > - To avoid checking of `a == b` twice, this check is lifted out of > `CharacterDataLatin1.equalsIgnoreCase` and the two callers are updated to > check that `a != b` before calling the method. > > For completeness, the RegionMatches test is updated to also compare Turkic > dotted/dotless 'i's against the uppercase ASCII 'I', not just the lowercase > one. Not stricktly related to the purpose of this PR, but it did help catch > a regression introduced in an earlier iteration of the PR. > > To guard against regressions caused by future changes to the set of Unicode > code points folding into latin1, a new test is added to `EqualsIgnoreCase` > which identifies all such code points and verifies they are compared correcty. > > Performance is tested for matching and mismatching cases of selected code > point pairs picked from the ASCII letter, ASCII number, latin1 letter and > non-latin Unicode letter ranges. Results in the first comment below. Benchmark results: Baseline: Benchmark (codePoints) (size) Mode Cnt Score Error Units RegionMatchesIC.Mixed.regionMatchesIC ascii-match1024 avgt 15 1497.391 ± 22.350 ns/op RegionMatchesIC.Mixed.regionMatchesIC ascii-mismatch1024 avgt 15 5.346 ± 0.165 ns/op RegionMatchesIC.Mixed.regionMatchesIC number-match1024 avgt 15 364.034 ± 5.561 ns/op RegionMatchesIC.Mixed.regionMatchesIC number-mismatch1024 avgt 15 4.036 ± 0.171 ns/op RegionMatchesIC.Mixed.regionMatchesIC lat1-match1024 avgt 15 2674.043 ± 174.669 ns/op RegionMatchesIC.Mixed.regionMatchesIClat1-mismatch1024 avgt 15 6.493 ± 0.230 ns/op RegionMatchesIC.Mixed.regionMatchesIC utf16-match1024 avgt 15 12630.314 ± 212.472 ns/op RegionMatchesIC.Mixed.regionMatchesIC utf16-mismatch1024 avgt 15 14.796 ± 0.359 ns/op PR: Benchmark (codePoints) (size) Mode Cnt ScoreError Units RegionMatchesIC.Mixed.regionMatchesIC ascii-match1024 avgt 15 1449.499 ± 14.350 ns/op RegionMatchesIC.Mixed.regionMatchesIC ascii-mismatch1024 avgt 15 3.450 ± 0.082 ns/op RegionMatchesIC.Mixed.regionMatchesIC number-match1024 avgt 15 362.582 ± 2.963 ns/op RegionMatchesIC.Mixed.regionMatchesIC number-mismatch1024 avgt 15 3.259 ± 0.021 ns/op RegionMatchesIC.Mixed.regionMatchesIC lat1-match1024 avgt 15 1625.513 ± 14.305 ns/op RegionMatchesIC.Mixed.regionMatchesIClat1-mismatch1024 avgt 15 3.858 ± 0.027 ns/op RegionMatchesIC.Mixed.regionMatchesIC utf16-match1024 avgt 15 1422.722 ± 85.581 ns/op RegionMatchesIC.Mixed.regionMatchesIC utf16-mismatch1024 avgt 15 3.756 ± 0.089 ns/op - PR: https://git.openjdk.org/jdk/pull/12637
RFR: 8302872: Speed up StringLatin1.regionMatchesCI_UTF16
This PR continues the efforts from #12632 to speed up case-insensitive string matching. We now tackle case-insensitive comparison of mixed-coder strings, implemented in `StringLatin1.regionMatchesCI_UTF16` Key insights: - If the UTF16 code point is also in latin1 range, we can leverage improvements from 12632 directly by calling `CharacterDataLatin1.equalsIgnoreCase` - There are exactly 7 non-latin1 Unicode code points which case fold into the latin1 range. We can special-case our comparison of these code points by adding the method `CharacterDataLatin1.latin1CaseFold`. - To avoid checking of `a == b` twice, this check is lifted out of `CharacterDataLatin1.equalsIgnoreCase` and the two callers are updated to check that `a != b` before calling the method. For completeness, the RegionMatches test is updated to also compare Turkic dotted/dotless 'i's against the uppercase ASCII 'I', not just the lowercase one. Not stricktly related to the purpose of this PR, but it did help catch a regression introduced in an earlier iteration of the PR. To guard against regressions caused by future changes to the set of Unicode code points folding into latin1, a new test is added to `EqualsIgnoreCase` which identifies all such code points and verifies they are compared correcty. Performance is tested for matching and mismatching cases of selected code point pairs picked from the ASCII letter, ASCII number, latin1 letter and non-latin Unicode letter ranges. Results in the first comment below. - Commit messages: - Inline local variable - latin1CaseFold was moved to CharacterDataLatin1 - Move latin1CaseFold to CharacterDataLatin1 - Improve latin1CaseFold javadocs - Simplify comments - Prefer fast matching by comparing for equality before checking latin1 range - Improve Javadocs of latin1CaseFold - Be consistent in comments - CharacterData.latin1LowerCase was renamed to latin1CaseFold - Hoist equality check out of CharacterDataLatin1.equalsIgnoreCase - ... and 13 more: https://git.openjdk.org/jdk/compare/f2b03f9a...92755920 Changes: https://git.openjdk.org/jdk/pull/12637/files Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=12637&range=00 Issue: https://bugs.openjdk.org/browse/JDK-8302872 Stats: 169 lines in 5 files changed: 155 ins; 2 del; 12 mod Patch: https://git.openjdk.org/jdk/pull/12637.diff Fetch: git fetch https://git.openjdk.org/jdk pull/12637/head:pull/12637 PR: https://git.openjdk.org/jdk/pull/12637
Re: RFR: JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask [v4]
On Mon, 27 Feb 2023 09:14:56 GMT, Viktor Klang wrote: >> I noticed when looking at the code that there was no real need to use a CHM >> to perform the tracking of activation in an ordered fashion on >> ForEachOrderedTask, but instead a VarHandle can be used, reducing >> allocations and indirection. > > Viktor Klang has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains three commits: > > - Updating copyright header of ForEachOps.java and removing unnecessary > suppression of an unchecked cast. > - Write the initial value of the next reference without using the VarHandle > - JDK-8302666: Replace CHM with VarHandle in ForeachOrderedTask src/java.base/share/classes/java/util/stream/ForEachOps.java line 513: > 511: // of right subtree (if any, which can be this task's right > sibling) > 512: // > 513: var leftDescendant = (ForEachOrderedTask T>)NEXT.getAndSet(this, null); Casting the `null` is required for the resolved method descriptor to be `(ForEachOrderedTask, ForEachOrderedTask)ForEachOrderedTask` instead of `(ForEachOrderedTask, Object)ForEachOrderedTask`, which prevents unnecessary type conversion `LambdaForm`s from being introduced and allows [`VarHandle::withInvokeExactBehavior`] to be used: Suggestion: var leftDescendant = (ForEachOrderedTask) NEXT.getAndSet(this, (ForEachOrderedTask) null); [`VarHandle::withInvokeExactBehavior`]: https://docs.oracle.com/en/java/javase/19/docs/api/java.base/java/lang/invoke/VarHandle.html#withInvokeExactBehavior() - PR: https://git.openjdk.org/jdk/pull/12320
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Tue, 28 Feb 2023 07:43:13 GMT, Tagir F. Valeev wrote:
>> Jim Laskey has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Optimize for empty CharSequence
>
> src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1903:
>
>> 1901: throw new OutOfMemoryError("Required length exceeds
>> implementation limit");
>> 1902: }
>> 1903: int total = count * length;
>
> We may avoid division if we use the long type:
>
>
> long totalLong = ((long) count) * length;
> if (totalLong > Integer.MAX_VALUE - offset) {
> throw new OutOfMemoryError("Required length exceeds implementation
> limit");
> }
> int total = (int) totalLong;
>
>
> Should be faster.
I'm a bit surprised this (and the original code) is throwing `OutOfMemoryError`
when running into the max array size. It is not truly an out of memory error,
but being an `Error`, it would evade standard catch `Exception` blocks. I
would think this is more of an `IllegalStateException` or perhaps something
array specific.
`OutOfMemoryError` is documented pretty specifically that an object allocation
failed after exhaustive GC, but no allocation or GC happened:
>Thrown when the Java Virtual Machine cannot allocate an object because it is
>out of memory, and no more memory could be made available by the garbage
>collector.
... which is not happening here at all. This leads me to believe the error is
not used correctly here.
Other reasons I find it surprising: `StringBuilder` is not documented to throw
this anywhere, it seems to just leak through from various methods implemented
by `AbstractStringBuilder`.
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: JDK-8302323 Add repeat methods to StringBuilder/StringBuffer [v3]
On Mon, 27 Feb 2023 13:30:47 GMT, Jim Laskey wrote:
>> Add the ability to repeatedly append char and CharSequence data to
>> StringBuilder/StringBuffer.
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Optimize for empty CharSequence
src/java.base/share/classes/java/lang/AbstractStringBuilder.java line 1879:
> 1877: * @since 21
> 1878: * @throws IllegalArgumentException if {@code count} is less than
> zero
> 1879: * @throws IndexOutOfBoundsException if the result overflows the
> buffer
Can `IndexOutOfBoundsException` be really thrown? We don't have any index here.
From the implementation I see that only `OutOfMemoryError` and
`IllegalArgumentException` can be thrown.
Also, I see a small descrepancy between messages in this method and
`String.repeat`. The latter has:
* @throws IllegalArgumentException if the {@code count} is
* negative.
*
* @since 11
*/
public String repeat(int count) {
if (count < 0) {
throw new IllegalArgumentException("count is negative: " + count);
}
Probably it's better to reuse the same wording ("is negative" instead of "is
less than zero") for consistency?
-
PR: https://git.openjdk.org/jdk/pull/12728
Re: RFR: 8303232: java.util.Date.parse(String) and java.util.Date(String) don't declare thrown IllegalArgumentException
On Tue, 28 Feb 2023 00:07:05 GMT, Justin Lu wrote: > The method `java.util.Date.parse(String)` and as a result, constructor > `java.util.Date(String)` throw an `IllegalArgumentException`. This exception > is not properly referenced in the javadoc, and this PR simply adds the throws > javadoc tag to make it apparent. The changes adds a new testable assertion to the Date(String) constructor so it will need a CSR. For the parse method then it's mostly a doc update as the method description already documents that it throws IAE, it's just didn't declare with @throws for some reason. - PR: https://git.openjdk.org/jdk/pull/12779
