Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Sat, 14 Nov 2020 07:55:35 GMT, Hui Shi wrote: >> Notice that the `/test` command is currently un-available (some >> implementation concerns are still under consideration). As an alternative, >> GitHub actions can be used to do basic automatic testing when pushes occur. >> >> However, pushes to the master branch are ignored. The tests didn't run for >> this PR since it was created from the `master` branch instead of a feature >> branch (this is ill-advised >> https://github.com/openjdk/jdk/pull/1070#issuecomment-722108502). >> >> @huishi-hs My advice for now would be to push the changes for this PR to a >> separate new branch in your fork, for which the GitHub actions workflow >> should run, and then simply include a link to the workflow here (if you want >> to share the results). > > @JornVernee How can "ready" label be added to this PR? This PR has been > reviewed. Would you help on this? @huishi-hs See the "Integration blocker" section in the PR body; you have a JBS issue and PR title mismatch. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Fri, 13 Nov 2020 16:02:42 GMT, Jorn Vernee wrote: >> Thanks all! May this test-request get approved? > > Notice that the `/test` command is currently un-available (some > implementation concerns are still under consideration). As an alternative, > GitHub actions can be used to do basic automatic testing when pushes occur. > > However, pushes to the master branch are ignored. The tests didn't run for > this PR since it was created from the `master` branch instead of a feature > branch (this is ill-advised > https://github.com/openjdk/jdk/pull/1070#issuecomment-722108502). > > @huishi-hs My advice for now would be to push the changes for this PR to a > separate new branch in your fork, for which the GitHub actions workflow > should run, and then simply include a link to the workflow here (if you want > to share the results). @JornVernee How can "ready" label be added to this PR? This PR has been reviewed. Would you help on this? - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Fri, 13 Nov 2020 16:02:42 GMT, Jorn Vernee wrote: >> Thanks all! May this test-request get approved? > > Notice that the `/test` command is currently un-available (some > implementation concerns are still under consideration). As an alternative, > GitHub actions can be used to do basic automatic testing when pushes occur. > > However, pushes to the master branch are ignored. The tests didn't run for > this PR since it was created from the `master` branch instead of a feature > branch (this is ill-advised > https://github.com/openjdk/jdk/pull/1070#issuecomment-722108502). > > @huishi-hs My advice for now would be to push the changes for this PR to a > separate new branch in your fork, for which the GitHub actions workflow > should run, and then simply include a link to the workflow here (if you want > to share the results). according to instructions, test is performed with another PR and passed https://github.com/openjdk/jdk/pull/1213 - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Fri, 13 Nov 2020 14:01:28 GMT, Hui Shi wrote: >> Thanks for the update, latest version looks good. > > Thanks all! May this test-request get approved? Notice that the `/test` command is currently un-available (some implementation concerns are still under consideration). As an alternative, GitHub actions can be used to do basic automatic testing when pushes occur. However, pushes to the master branch are ignored. The tests didn't run for this PR since it was created from the `master` branch instead of a feature branch (this is ill-advised https://github.com/openjdk/jdk/pull/1070#issuecomment-722108502). @huishi-hs My advice for now would be to push the changes for this PR to a separate new branch in your fork, for which the GitHub actions workflow should run, and then simply include a link to the workflow here (if you want to share the results). - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Fri, 13 Nov 2020 08:33:30 GMT, Alan Bateman wrote: >> Hui Shi 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: >> >> 8255883: Avoid multiple GeneratedAccessor for same >> NativeMethod/ConstructorAccessorImpl object > > Thanks for the update, latest version looks good. Thanks all! May this test-request get approved? - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
On Fri, 13 Nov 2020 03:50:09 GMT, Hui Shi wrote: >> …AccessorImpl object >> >> We met real problem when using protobuf with option optimized for code size, >> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 >> >> Optimize solution is adding a new boolean field to detect concurrent method >> accessor generation in same NativeMethodAccessorImpl object, only one thread >> is allowed to generate accessor, other threads still invoke in jni way until >> parent's delegator is updated from NativeMethodAccessorImpl to generated >> accessor. >> >> In common case, extra overhead is an atomic operation, compared with method >> accessor generate, this cost is trivial. > > Hui Shi 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: > > 8255883: Avoid multiple GeneratedAccessor for same > NativeMethod/ConstructorAccessorImpl object Thanks for the update, latest version looks good. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
On Thu, 12 Nov 2020 12:23:36 GMT, Alan Bateman wrote: >> @AlanBateman >> >>> What is the reason for using an int? I remember there was a suggestion for >>> three states but two states seems okay so curious why it was changed from >>> boolean to int. >> >> shipilev suggested not to use sub-word CAS, so change compareAndSetBoolean >> to compareAndSetInt. Change field "generated" from boolean to int doesn't >> increase object size (no extra byte/boolean to fold into same word). >> >>> The restoring to 0 in the event of failure should probably be a >>> volatile-write. Might be clearer to declare it as a volatile. >> >> "generated" field is read once and CAS once in this method. It doesn't >> likely cached and no visibiliy order required. > > okay, although I think it would be clearer for maintainers to declare it as > volatile because it is set with CAS. @AlanBateman volatile attribute is added. As in common case, it only read and CAS at most once. And I see no regression on my testcase. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v6]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi 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: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/ec1531b2..85c8b96a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=04-05 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
On Thu, 12 Nov 2020 04:23:37 GMT, Hui Shi wrote: >> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java >> line 44: >> >>> 42: private DelegatingConstructorAccessorImpl parent; >>> 43: private int numInvocations; >>> 44: private int generated; >> >> What is the reason for using an int? I remember there was a suggestion for >> three states but two states seems okay so curious why it was changed from >> boolean to int. >> The restoring to 0 in the event of failure should probably be a >> volatile-write. Might be clearer to declare it as a volatile. > > @AlanBateman > >> What is the reason for using an int? I remember there was a suggestion for >> three states but two states seems okay so curious why it was changed from >> boolean to int. > > shipilev suggested not to use sub-word CAS, so change compareAndSetBoolean to > compareAndSetInt. Change field "generated" from boolean to int doesn't > increase object size (no extra byte/boolean to fold into same word). > >> The restoring to 0 in the event of failure should probably be a >> volatile-write. Might be clearer to declare it as a volatile. > > "generated" field is read once and CAS once in this method. It doesn't > likely cached and no visibiliy order required. okay, although I think it would be clearer for maintainers to declare it as volatile because it is set with CAS. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
On Wed, 11 Nov 2020 19:12:08 GMT, Alan Bateman wrote: >> Hui Shi 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. > > src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java > line 44: > >> 42: private DelegatingConstructorAccessorImpl parent; >> 43: private int numInvocations; >> 44: private int generated; > > What is the reason for using an int? I remember there was a suggestion for > three states but two states seems okay so curious why it was changed from > boolean to int. > The restoring to 0 in the event of failure should probably be a > volatile-write. Might be clearer to declare it as a volatile. @AlanBateman > What is the reason for using an int? I remember there was a suggestion for > three states but two states seems okay so curious why it was changed from > boolean to int. shipilev suggested not to use sub-word CAS, so change compareAndSetBoolean to compareAndSetInt. Change field "generated" from boolean to int doesn't increase object size (no extra byte/boolean to fold into same word). > The restoring to 0 in the event of failure should probably be a > volatile-write. Might be clearer to declare it as a volatile. "generated" field is read once and CAS once in this method. It doesn't likely cached and no visibiliy order required. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
On Wed, 11 Nov 2020 05:38:08 GMT, Hui Shi wrote: >> …AccessorImpl object >> >> We met real problem when using protobuf with option optimized for code size, >> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 >> >> Optimize solution is adding a new boolean field to detect concurrent method >> accessor generation in same NativeMethodAccessorImpl object, only one thread >> is allowed to generate accessor, other threads still invoke in jni way until >> parent's delegator is updated from NativeMethodAccessorImpl to generated >> accessor. >> >> In common case, extra overhead is an atomic operation, compared with method >> accessor generate, this cost is trivial. > > Hui Shi 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. src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java line 44: > 42: private DelegatingConstructorAccessorImpl parent; > 43: private int numInvocations; > 44: private int generated; What is the reason for using an int? I remember there was a suggestion for three states but two states seems okay so curious why it was changed from boolean to int. The restoring to 0 in the event of failure should probably be a volatile-write. Might be clearer to declare it as a volatile. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
On Wed, 11 Nov 2020 05:38:08 GMT, Hui Shi wrote: >> …AccessorImpl object >> >> We met real problem when using protobuf with option optimized for code size, >> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 >> >> Optimize solution is adding a new boolean field to detect concurrent method >> accessor generation in same NativeMethodAccessorImpl object, only one thread >> is allowed to generate accessor, other threads still invoke in jni way until >> parent's delegator is updated from NativeMethodAccessorImpl to generated >> accessor. >> >> In common case, extra overhead is an atomic operation, compared with method >> accessor generate, this cost is trivial. > > Hui Shi 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. Looks okay (as far as low-level code goes) to me. Others need to ack too. - Marked as reviewed by shade (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Tue, 10 Nov 2020 18:36:22 GMT, Aleksey Shipilev wrote: >> Hui Shi 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: >> >> 8255883: Avoid multiple GeneratedAccessor for same >> NativeMethod/ConstructorAccessorImpl object > > Minor nits here. @shipilev all comments are updated, please help review again! - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Tue, 10 Nov 2020 18:28:11 GMT, Aleksey Shipilev wrote:
>> Hui Shi 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:
>>
>> 8255883: Avoid multiple GeneratedAccessor for same
>> NativeMethod/ConstructorAccessorImpl object
>
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 37:
>
>> 35:
>> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
>> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
>
> `static final` field identifiers should be capitalized. Suggestion: `private
> static final Unsafe U = Unsafe.getUnsafe()`.
fixed
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 38:
>
>> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
>> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
>> 38: private static final long accessorGeneratedaOffset
>
> Typo "Generated*a*Offset". But also this is `static final`, so it should be
> e.g. `GENERATED_OFFSET`. The field can be just `generated` to match the name
> of this offset.
field name udpated
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 61:
>
>> 59: && !c.getDeclaringClass().isHidden()
>> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
>> 61: && accessorGenerated == false
>
> Should be `!accessorGenerated`.
change to "generated == 0", as generated is int type now
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 62:
>
>> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
>> 61: && accessorGenerated == false
>> 62: && unsafe.compareAndSetBoolean(this,
>> accessorGeneratedaOffset, false, true)) {
>
> `compareAndSetBoolean` gets us into the awkward territory of sub-word CASes.
> (Click through to `Unsafe.compareAndExchangeByte` to see what I am talking
> about). It is probably not relevant here, though. Still, might be as good to
> use `int` field for generated flag.
use int type now
> src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
> line 72:
>
>> 70: parent.setDelegate(acc);
>> 71: } catch (Throwable t) {
>> 72: // in case Thowable happens in generateMethod
>
> Typo: `Thowable`.
updated
-
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v5]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi 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: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/abf34872..ec1531b2 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=03-04 Stats: 16 lines in 2 files changed: 0 ins; 2 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v4]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi 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: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/7a1b274a..abf34872 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v3]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi 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: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/8f65047e..7a1b274a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=01-02 Stats: 18 lines in 2 files changed: 0 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Sun, 8 Nov 2020 05:07:07 GMT, Hui Shi wrote:
>> …AccessorImpl object
>>
>> We met real problem when using protobuf with option optimized for code size,
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>>
>> Optimize solution is adding a new boolean field to detect concurrent method
>> accessor generation in same NativeMethodAccessorImpl object, only one thread
>> is allowed to generate accessor, other threads still invoke in jni way until
>> parent's delegator is updated from NativeMethodAccessorImpl to generated
>> accessor.
>>
>> In common case, extra overhead is an atomic operation, compared with method
>> accessor generate, this cost is trivial.
>
> Hui Shi 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:
>
> 8255883: Avoid multiple GeneratedAccessor for same
> NativeMethod/ConstructorAccessorImpl object
Minor nits here.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 37:
> 35:
> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
`static final` field identifiers should be capitalized. Suggestion: `private
static final Unsafe U = Unsafe.getUnsafe()`.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 38:
> 36: class NativeConstructorAccessorImpl extends ConstructorAccessorImpl {
> 37: private static final Unsafe unsafe = Unsafe.getUnsafe();
> 38: private static final long accessorGeneratedaOffset
Typo "Generated*a*Offset". But also this is `static final`, so it should be
e.g. `GENERATED_OFFSET`. The field can be just `generated` to match the name of
this offset.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 61:
> 59: && !c.getDeclaringClass().isHidden()
> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
> 61: && accessorGenerated == false
Should be `!accessorGenerated`.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 62:
> 60: && !ReflectUtil.isVMAnonymousClass(c.getDeclaringClass())
> 61: && accessorGenerated == false
> 62: && unsafe.compareAndSetBoolean(this,
> accessorGeneratedaOffset, false, true)) {
`compareAndSetBoolean` gets us into the awkward territory of sub-word CASes.
(Click through to `Unsafe.compareAndExchangeByte` to see what I am talking
about). It is probably not relevant here, though. Still, might be as good to
use `int` field for generated flag.
src/java.base/share/classes/jdk/internal/reflect/NativeConstructorAccessorImpl.java
line 72:
> 70: parent.setDelegate(acc);
> 71: } catch (Throwable t) {
> 72: // in case Thowable happens in generateMethod
Typo: `Thowable`.
-
Changes requested by shade (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Fri, 6 Nov 2020 08:58:13 GMT, Joel Borggrén-Franck wrote: > Are there any benchmarks to compare this accessor with the previous version > in the presumably common case where there is no or very little contention? > Edit to clarify: it is stated as "trivial" is this also measured somewhere? @jbf in presumably common case, one extra CAS is added when invocation reach threshold. MethodInvoke in jdk repo micro benchmark test shows no regression with this patch. **Before patch** Benchmark Mode Cnt Score Error Units MethodInvoke.invokeWithSixObjectParams avgt 25 5.443 ? 0.008 ns/op MethodInvoke.invokeWithSixPrimitiveParams avgt 25 10.195 ? 0.020 ns/op MethodInvoke.invokeWithoutParams avgt 25 3.919 ? 0.011 ns/op Benchmark Mode Cnt Score Error Units MethodInvoke.invokeWithSixObjectParams avgt 25 5.440 ? 0.012 ns/op MethodInvoke.invokeWithSixPrimitiveParams avgt 25 10.197 ? 0.028 ns/op MethodInvoke.invokeWithoutParams avgt 25 3.923 ? 0.010 ns/op **After patch** Benchmark Mode Cnt Score Error Units MethodInvoke.invokeWithSixObjectParams avgt 25 5.438 ? 0.005 ns/op MethodInvoke.invokeWithSixPrimitiveParams avgt 25 10.190 ? 0.038 ns/op MethodInvoke.invokeWithoutParams avgt 25 3.930 ? 0.010 ns/op Finished running test 'micro:java.lang.reflect.MethodInvoke' Benchmark Mode Cnt Score Error Units MethodInvoke.invokeWithSixObjectParams avgt 25 5.442 ? 0.009 ns/op MethodInvoke.invokeWithSixPrimitiveParams avgt 25 10.205 ? 0.034 ns/op MethodInvoke.invokeWithoutParams avgt 25 3.916 ? 0.005 ns/op - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
On Thu, 5 Nov 2020 14:59:56 GMT, Alan Bateman wrote: >> I do wonder if it makes sense to handle triple-state `int` here: "not yet >> generated", "generated", "in error"? So that we don't try to generate the >> accessor over and over again when it is in error? > > If we are changing NativeMethodAccessorImpl.invoke then we should probably do > NativeConstructorAccessorImpl.newInstance at the same time. Also wondering if > we should, while in the area, add "return acc.invoke(obj, args)" after > setting the delegate so that it invokes the newly generated accessor. > > Are there resource or other cases that you have observed where generateMethod > fails and then succeeds in a subsequent call? > > @cl4es Do you know of any startup tests that might be sensitive to the eager > creating of a VarHandle? > > I agree with @shipilev to test before the CAS. @AlanBateman @shipilev commit is updated, please help reivew it again 1. add same change in NativeConstructorAccessorImpl 2. eager creating of a VarHandle in both NativeConstructorAccessorImpl and NativeMethodAccessorImpl will cause error. [http://cr.openjdk.java.net/~hshi/8255883/cyclic_clinit_dependency.txt](url). Use unsafe API here. 3. add test before compareAndSet and remove "succ" variable. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod… [v2]
> …AccessorImpl object > > We met real problem when using protobuf with option optimized for code size, > detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 > > Optimize solution is adding a new boolean field to detect concurrent method > accessor generation in same NativeMethodAccessorImpl object, only one thread > is allowed to generate accessor, other threads still invoke in jni way until > parent's delegator is updated from NativeMethodAccessorImpl to generated > accessor. > > In common case, extra overhead is an atomic operation, compared with method > accessor generate, this cost is trivial. Hui Shi 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: 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/94390613..8f65047e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=1070=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1070=00-01 Stats: 43 lines in 2 files changed: 15 ins; 9 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Fri, 6 Nov 2020 00:24:14 GMT, Hui Shi wrote: >> If we are changing NativeMethodAccessorImpl.invoke then we should probably >> do NativeConstructorAccessorImpl.newInstance at the same time. Also >> wondering if we should, while in the area, add "return acc.invoke(obj, >> args)" after setting the delegate so that it invokes the newly generated >> accessor. >> >> Are there resource or other cases that you have observed where >> generateMethod fails and then succeeds in a subsequent call? >> >> @cl4es Do you know of any startup tests that might be sensitive to the eager >> creating of a VarHandle? >> >> I agree with @shipilev to test before the CAS. > >> If we are changing NativeMethodAccessorImpl.invoke then we should probably >> do NativeConstructorAccessorImpl.newInstance at the same time. > > Yes, NativeConstructorAccessorImpl should also apply this change. > >> Also wondering if we should, while in the area, add "return acc.invoke(obj, >> args)" after setting the delegate so that it invokes the newly generated >> accessor. >> > > Agree, I see no harm to invoke generated method when it is aviable. > Should leave this to another patch? > >> Are there resource or other cases that you have observed where >> generateMethod fails and then succeeds in a subsequent call? >> > I have not seen exception/error happen in generate method yet. But in case > it fails in some ways, try - catch - reset is added to make sure behavior is > same before/after with this change. > >> @cl4es Do you know of any startup tests that might be sensitive to the eager >> creating of a VarHandle? >> >> I agree with @shipilev to test before the CAS. > > @AlanBateman > Thanks for you comments! Are there any benchmarks to compare this accessor with the previous version in the presumably common case where there is no or very little contention? Edit to clarify: it is stated as "trivial" is this also measured somewhere? - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 14:59:56 GMT, Alan Bateman wrote: > If we are changing NativeMethodAccessorImpl.invoke then we should probably do > NativeConstructorAccessorImpl.newInstance at the same time. Yes, NativeConstructorAccessorImpl should also apply this change. > Also wondering if we should, while in the area, add "return acc.invoke(obj, > args)" after setting the delegate so that it invokes the newly generated > accessor. > Agree, I see no harm to invoke generated method when it is aviable. Should leave this to another patch? > Are there resource or other cases that you have observed where generateMethod > fails and then succeeds in a subsequent call? > I have not seen exception/error happen in generate method yet. But in case it fails in some ways, try - catch - reset is added to make sure behavior is same before/after with this change. > @cl4es Do you know of any startup tests that might be sensitive to the eager > creating of a VarHandle? > > I agree with @shipilev to test before the CAS. @AlanBateman Thanks for you comments! - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On 5/11/2020 7:09 pm, Aleksey Shipilev wrote:
On Thu, 5 Nov 2020 02:52:05 GMT, Hui Shi wrote:
…AccessorImpl object
We met real problem when using protobuf with option optimized for code size,
detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
Optimize solution is adding a new boolean field to detect concurrent method
accessor generation in same NativeMethodAccessorImpl object, only one thread is
allowed to generate accessor, other threads still invoke in jni way until
parent's delegator is updated from NativeMethodAccessorImpl to generated
accessor.
In common case, extra overhead is an atomic operation, compared with method
accessor generate, this cost is trivial.
I do wonder if it makes sense to handle triple-state `int` here: "not yet generated",
"generated", "in error"? So that we don't try to generate the accessor over and over
again when it is in error?
You have no idea why generation failed so don't know whether a retry
will succeed or not. Current code will retry so it would be a change in
behaviour to declare it permanently "in error".
Cheers,
David
src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
line 80:
78: succ = true;
79: } finally {
80: if (succ == false) {
Why `succ` variable, if you can just `catch (Throwable e)` and restore the
`accessorGenerated`?
src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
line 66:
64: && !method.getDeclaringClass().isHidden()
65: &&
!ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {
As the micro-optimization, checking that `accessor_generated` is `false` before
attempting a (potentially contended) CAS might fit better. (See
https://en.wikipedia.org/wiki/Test_and_test-and-set).
-
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 14:59:56 GMT, Alan Bateman wrote: >> I do wonder if it makes sense to handle triple-state `int` here: "not yet >> generated", "generated", "in error"? So that we don't try to generate the >> accessor over and over again when it is in error? > > If we are changing NativeMethodAccessorImpl.invoke then we should probably do > NativeConstructorAccessorImpl.newInstance at the same time. Also wondering if > we should, while in the area, add "return acc.invoke(obj, args)" after > setting the delegate so that it invokes the newly generated accessor. > > Are there resource or other cases that you have observed where generateMethod > fails and then succeeds in a subsequent call? > > @cl4es Do you know of any startup tests that might be sensitive to the eager > creating of a VarHandle? > > I agree with @shipilev to test before the CAS. @AlanBateman I had a look and it seems unlikely the eager creation in this `` will matter. I can find `NativeMethodAccessorImpl` in the startup profiles only for netty and larger apps, which often already initialize some `VarHandle` or are large enough that the initialization overhead will be lost in the noise. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 09:07:13 GMT, Aleksey Shipilev wrote: >> …AccessorImpl object >> >> We met real problem when using protobuf with option optimized for code size, >> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 >> >> Optimize solution is adding a new boolean field to detect concurrent method >> accessor generation in same NativeMethodAccessorImpl object, only one thread >> is allowed to generate accessor, other threads still invoke in jni way until >> parent's delegator is updated from NativeMethodAccessorImpl to generated >> accessor. >> >> In common case, extra overhead is an atomic operation, compared with method >> accessor generate, this cost is trivial. > > I do wonder if it makes sense to handle triple-state `int` here: "not yet > generated", "generated", "in error"? So that we don't try to generate the > accessor over and over again when it is in error? If we are changing NativeMethodAccessorImpl.invoke then we should probably do NativeConstructorAccessorImpl.newInstance at the same time. Also wondering if we should, while in the area, add "return acc.invoke(obj, args)" after setting the delegate so that it invokes the newly generated accessor. Are there resource or other cases that you have observed where generateMethod fails and then succeeds in a subsequent call? @cl4es Do you know of any startup tests that might be sensitive to the eager creating of a VarHandle? I agree with @shipilev to test before the CAS. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 09:05:55 GMT, Aleksey Shipilev wrote:
>> …AccessorImpl object
>>
>> We met real problem when using protobuf with option optimized for code size,
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>>
>> Optimize solution is adding a new boolean field to detect concurrent method
>> accessor generation in same NativeMethodAccessorImpl object, only one thread
>> is allowed to generate accessor, other threads still invoke in jni way until
>> parent's delegator is updated from NativeMethodAccessorImpl to generated
>> accessor.
>>
>> In common case, extra overhead is an atomic operation, compared with method
>> accessor generate, this cost is trivial.
>
> src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
> line 66:
>
>> 64: && !method.getDeclaringClass().isHidden()
>> 65: &&
>> !ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
>> 66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {
>
> As the micro-optimization, checking that `accessor_generated` is `false`
> before attempting a (potentially contended) CAS might fit better. (See
> https://en.wikipedia.org/wiki/Test_and_test-and-set).
Agree this could benefit performance. Grep jdk codes (grep -R compareAndSet
*), not found codes with test and test-and-set pattern.
compareAndSet is intrinsic and maybe it's better let runtime/compiler generate
Test_and_test-and-set code sequence to solve all compareAndSet operations.
-
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 09:02:55 GMT, Aleksey Shipilev wrote:
>> …AccessorImpl object
>>
>> We met real problem when using protobuf with option optimized for code size,
>> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>>
>> Optimize solution is adding a new boolean field to detect concurrent method
>> accessor generation in same NativeMethodAccessorImpl object, only one thread
>> is allowed to generate accessor, other threads still invoke in jni way until
>> parent's delegator is updated from NativeMethodAccessorImpl to generated
>> accessor.
>>
>> In common case, extra overhead is an atomic operation, compared with method
>> accessor generate, this cost is trivial.
>
> src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
> line 80:
>
>> 78: succ = true;
>> 79: } finally {
>> 80: if (succ == false) {
>
> Why `succ` variable, if you can just `catch (Throwable e)` and restore the
> `accessorGenerated`?
Good suggest! Will update.
-
PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
On Thu, 5 Nov 2020 02:52:05 GMT, Hui Shi wrote:
> …AccessorImpl object
>
> We met real problem when using protobuf with option optimized for code size,
> detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883
>
> Optimize solution is adding a new boolean field to detect concurrent method
> accessor generation in same NativeMethodAccessorImpl object, only one thread
> is allowed to generate accessor, other threads still invoke in jni way until
> parent's delegator is updated from NativeMethodAccessorImpl to generated
> accessor.
>
> In common case, extra overhead is an atomic operation, compared with method
> accessor generate, this cost is trivial.
I do wonder if it makes sense to handle triple-state `int` here: "not yet
generated", "generated", "in error"? So that we don't try to generate the
accessor over and over again when it is in error?
src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
line 80:
> 78: succ = true;
> 79: } finally {
> 80: if (succ == false) {
Why `succ` variable, if you can just `catch (Throwable e)` and restore the
`accessorGenerated`?
src/java.base/share/classes/jdk/internal/reflect/NativeMethodAccessorImpl.java
line 66:
> 64: && !method.getDeclaringClass().isHidden()
> 65: &&
> !ReflectUtil.isVMAnonymousClass(method.getDeclaringClass())
> 66: && ACCESSOR_GENERATED.compareAndSet(this, false, true)) {
As the micro-optimization, checking that `accessor_generated` is `false` before
attempting a (potentially contended) CAS might fit better. (See
https://en.wikipedia.org/wiki/Test_and_test-and-set).
-
PR: https://git.openjdk.java.net/jdk/pull/1070
RFR: 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethod…
…AccessorImpl object We met real problem when using protobuf with option optimized for code size, detail in JBS https://bugs.openjdk.java.net/browse/JDK-8255883 Optimize solution is adding a new boolean field to detect concurrent method accessor generation in same NativeMethodAccessorImpl object, only one thread is allowed to generate accessor, other threads still invoke in jni way until parent's delegator is updated from NativeMethodAccessorImpl to generated accessor. In common case, extra overhead is an atomic operation, compared with method accessor generate, this cost is trivial. - Commit messages: - 8255883: Avoid multiple GeneratedMethodAccessor for same NativeMethodAccessorImpl object Changes: https://git.openjdk.java.net/jdk/pull/1070/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=1070=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255883 Stats: 34 lines in 1 file changed: 24 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/1070.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1070/head:pull/1070 PR: https://git.openjdk.java.net/jdk/pull/1070
