Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-08-02 Thread Сергей Цыпанов
On Tue, 2 Aug 2022 17:48:07 GMT, Peter Levart wrote: >> Let's wait a bit > > @stsypanov Do you need a sponsor or are you waiting for some other reviewer? @plevart I don't think we need any more review for the change is simple - PR: https://git.openjdk.org/jdk/pull/9143

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-08-02 Thread Peter Levart
On Tue, 2 Aug 2022 16:49:07 GMT, Сергей Цыпанов wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8288327: Inline privateGetParameters() > > Let's wait a bit @stsypanov Do you need a sponsor or are you waiting

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-08-02 Thread Сергей Цыпанов
On Thu, 30 Jun 2022 12:08:19 GMT, Сергей Цыпанов wrote: >> If there are two threads calling `Executable.hasRealParameterData()` under >> race and the first one writes into volatile `Executable.parameters` field >> (doing _releasing store_) and the second thread reads non-null value from >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-05 Thread Peter Levart
On Thu, 30 Jun 2022 12:08:19 GMT, Сергей Цыпанов wrote: >> If there are two threads calling `Executable.hasRealParameterData()` under >> race and the first one writes into volatile `Executable.parameters` field >> (doing _releasing store_) and the second thread reads non-null value from >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-05 Thread Peter Levart
On Mon, 4 Jul 2022 10:06:12 GMT, Сергей Цыпанов wrote: >> A more realistic use case would be something in the lines of checking >> whether the method is an expected one: >> >> >> @Benchmark >> public boolean isFoo() { >> return matches(method, "foo", int.class, String.class); >> }

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-04 Thread Сергей Цыпанов
On Mon, 4 Jul 2022 06:31:15 GMT, Peter Levart wrote: >> ...neither is obtaining a cloned array and passing its reference to JMH's >> black hole our usecase... Still, it seems that even part of that has some >> advantage. I would keep the @stable annotation then. > > A more realistic use case

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-04 Thread Peter Levart
On Mon, 4 Jul 2022 06:19:00 GMT, Peter Levart wrote: >> hmm, is the faster getParameters (without explicit index access) a result of >> the annotation? getParameter0 shows the documented effect but isn't quite >> our use case here. > > ...neither is obtaining a cloned array and passing its

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-04 Thread Peter Levart
On Sun, 3 Jul 2022 20:12:13 GMT, liach wrote: >> With `static` and without `@Stable` the benchmark yields >> >> BenchmarkMode Cnt Score Error Units >> AccessParamsBenchmark.getParameter0 avgt 10 1,212 ± 0,083 ns/op >> AccessParamsBenchmark.getParameters

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-03 Thread liach
On Fri, 1 Jul 2022 20:39:49 GMT, Сергей Цыпанов wrote: >> @Stable is only effective if the path leading to @Stable value can be >> constant-folded by JIT. In above test, you have an instance field Method >> method. This can not be constant-folded, so neither can @stable fiels in the >> Field

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Сергей Цыпанов
On Fri, 1 Jul 2022 18:32:18 GMT, Peter Levart wrote: >> @plevart I've checked it with and without `@stable`, it's the same: >> >> with >> BenchmarkMode Cnt Score Error Units >> AccessParamsBenchmark.getParameter0 avgt 50 6,196 ± 0,142 ns/op >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Fri, 1 Jul 2022 12:26:04 GMT, Сергей Цыпанов wrote: >> ... I can only see the array being cloned and not accessed directly. I don't >> belive cloning a @stable array is any different in JIT-ed code as cloning >> normal "mutable" array unless JIT "sees" through it and scalarizes the >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Сергей Цыпанов
On Fri, 1 Jul 2022 10:48:32 GMT, Peter Levart wrote: >> But, ... is any code path accessing the elements of the @Stable array by >> constant indexes? Only in that case would the annotation have any effect on >> the JIT-ed code. Otherwise it's just a waste of space. > > ... I can only see the

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Fri, 1 Jul 2022 10:38:53 GMT, Peter Levart wrote: >> Right, in that case, it should remain. > > But, ... is any code path accessing the elements of the @Stable array by > constant indexes? Only in that case would the annotation have any effect on > the JIT-ed code. Otherwise it's just a

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Fri, 1 Jul 2022 10:35:11 GMT, Peter Levart wrote: >> @plevart so should I remove it or keep as is? > > Right, in that case, it should remain. But, ... is any code path accessing the elements of the @Stable array by constant indexes? Only in that case would the annotation have any effect on

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Thu, 30 Jun 2022 12:08:19 GMT, Сергей Цыпанов wrote: >> If there are two threads calling `Executable.hasRealParameterData()` under >> race and the first one writes into volatile `Executable.parameters` field >> (doing _releasing store_) and the second thread reads non-null value from >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Fri, 1 Jul 2022 07:27:59 GMT, Сергей Цыпанов wrote: >> src/java.base/share/classes/java/lang/reflect/Executable.java line 457: >> >>> 455: private transient @Stable ParameterData parameterData; >>> 456: >>> 457: record ParameterData(@Stable Parameter[] parameters, boolean >>>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Сергей Цыпанов
On Fri, 1 Jul 2022 06:12:11 GMT, Peter Levart wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8288327: Inline privateGetParameters() > > src/java.base/share/classes/java/lang/reflect/Executable.java line 457: >

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread ExE Boss
On Fri, 1 Jul 2022 06:12:11 GMT, Peter Levart wrote: >> Сергей Цыпанов has updated the pull request incrementally with one >> additional commit since the last revision: >> >> 8288327: Inline privateGetParameters() > > src/java.base/share/classes/java/lang/reflect/Executable.java line 457: >

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-07-01 Thread Peter Levart
On Thu, 30 Jun 2022 12:08:19 GMT, Сергей Цыпанов wrote: >> If there are two threads calling `Executable.hasRealParameterData()` under >> race and the first one writes into volatile `Executable.parameters` field >> (doing _releasing store_) and the second thread reads non-null value from >>

Re: RFR: 8288327: Executable.hasRealParameterData should not be volatile [v7]

2022-06-30 Thread Сергей Цыпанов
> If there are two threads calling `Executable.hasRealParameterData()` under > race and the first one writes into volatile `Executable.parameters` field > (doing _releasing store_) and the second thread reads non-null value from the > same field (doing acquiring read) then it must read exactly