On Sun, 8 Nov 2020 05:07:07 GMT, Hui Shi <h...@openjdk.org> 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
