Re: RFR: 8266610: Method RandomAccessFile#length() returns 0 for block devices on linux. [v2]
On Fri, 7 May 2021 12:17:11 GMT, Vyom Tewari wrote: >> RandomAccessFile.length() method for block device return always 0 > > Vyom Tewari has updated the pull request incrementally with one additional > commit since the last revision: > > fixed assigning -1 to uint64_t Could required os = linux added for test/jdk/java/nio/channels/FileChannel/BlockDeviceSize.java? As it is decribed only run as linux. - PR: https://git.openjdk.java.net/jdk/pull/3914
Integrated: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads
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. This pull request has now been integrated. Changeset: 8eeb36f1 Author:Hui Shi Committer: Alan Bateman URL: https://git.openjdk.java.net/jdk/commit/8eeb36f1 Stats: 46 lines in 2 files changed: 28 ins; 0 del; 18 mod 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads Reviewed-by: shade, alanb - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads [v6]
On Sun, 15 Nov 2020 07:25:30 GMT, Alan Bateman wrote: >> After review PR/commit title is changed to align with JBS title to add ready >> label > > Can you sync up your branch (the bot is saying that it's 140 commits behind > the main line)? I can sponsor once it has been tested with an up to date > branch. @AlanBateman Thanks for your sponsoring! Commit is updated. - PR: https://git.openjdk.java.net/jdk/pull/1070
Re: RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads [v8]
> …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 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 one additional commit since the last revision: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/98278050..e7b6d88a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=06-07 Stats: 110505 lines in 1385 files changed: 59994 ins; 39577 del; 10934 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
Withdrawn: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads
On Fri, 13 Nov 2020 23:37:46 GMT, Hui Shi wrote: > …ructorAccessorImpl object > > duplicate with https://github.com/openjdk/jdk/pull/1070, try trigger test > with new PR This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/1213
Re: RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads
On Sat, 14 Nov 2020 07:40:07 GMT, Hui Shi wrote: >> @huishi-hs Testing should start automatically, and will appear here when you >> push to any branch in your fork that is not `master`: >> https://github.com/huishi-hs/jdk/actions (unless you disabled this in the >> repo settings). >> >> As I said, the `/test` command does not work at this time. > > @JornVernee Thanks! I didn't enable > https://github.com/huishi-hs/jdk/actions. Now it works. Close this. PR is only for presubmit testing. - PR: https://git.openjdk.java.net/jdk/pull/1213
Re: RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads [v7]
> …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 duplicated GeneratedMethodAccessor when reflect method invoked from different threads - Changes: - all: https://git.openjdk.java.net/jdk/pull/1070/files - new: https://git.openjdk.java.net/jdk/pull/1070/files/85c8b96a..98278050 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=05-06 Stats: 0 lines in 0 files changed: 0 ins; 0 del; 0 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 duplicated GeneratedMethodAccessor when reflect method invoked from different threads [v6]
On Sat, 14 Nov 2020 12:11:52 GMT, Jorn Vernee wrote: >> @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. After review PR/commit title is changed to align with JBS title to add ready label - 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 duplicated GeneratedMethodAccessor when reflect method invoked from different threads
On Sat, 14 Nov 2020 00:28:58 GMT, Jorn Vernee wrote: >> @JornVernee >> Could you please help approve and start tier1 test? >> This is same PR with https://github.com/openjdk/jdk/pull/1070 > > @huishi-hs Testing should start automatically, and will appear here when you > push to any branch in your fork that is not `master`: > https://github.com/huishi-hs/jdk/actions (unless you disabled this in the > repo settings). > > As I said, the `/test` command does not work at this time. @JornVernee Thanks! I didn't enable https://github.com/huishi-hs/jdk/actions. Now it works. - PR: https://git.openjdk.java.net/jdk/pull/1213
Re: RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads
On Fri, 13 Nov 2020 23:37:46 GMT, Hui Shi wrote: > …ructorAccessorImpl object > > duplicate with https://github.com/openjdk/jdk/pull/1070, try trigger test > with new PR @JornVernee Could you please help approve and start tier1 test? This is same PR with https://github.com/openjdk/jdk/pull/1070 - PR: https://git.openjdk.java.net/jdk/pull/1213
RFR: 8255883: Avoid duplicated GeneratedMethodAccessor when reflect method invoked from different threads
…ructorAccessorImpl object duplicate with https://github.com/openjdk/jdk/pull/1070, try trigger test with new PR - Commit messages: - 8255883: Avoid multiple GeneratedAccessor for same NativeMethod/ConstructorAccessorImpl object Changes: https://git.openjdk.java.net/jdk/pull/1213/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1213&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8255883 Stats: 46 lines in 2 files changed: 28 ins; 0 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/1213.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1213/head:pull/1213 PR: https://git.openjdk.java.net/jdk/pull/1213
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… [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&pr=1070&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=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 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… [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&pr=1070&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=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&pr=1070&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=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&pr=1070&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=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 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&pr=1070&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1070&range=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 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 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
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&pr=1070&range=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
