Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy wrote:
>> This is an early review of changes to better model JVM access flags, that is
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>>
>> Language level modifiers and JVM level access flags are closely related, but
>> distinct. There are concepts that overlap in the two domains (public,
>> private, etc.), others that only have a language-level modifier (sealed),
>> and still others that only have an access flag (synthetic).
>>
>> The existing java.lang.reflect.Modifier class is inadequate to model these
>> subtleties. For example, the bit positions used by access flags on different
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for
>> methods. Just having a raw integer does not provide sufficient context to
>> decode the corresponding language-level string. Methods like
>> Modifier.methodModifiers() were introduced to cope with this situation.
>>
>> With additional modifiers and flags on the horizon with projects like
>> Valhalla, addressing the existent modeling deficiency now ahead of time is
>> reasonable before further strain is introduced.
>>
>> This PR in its current form is meant to give the overall shape of the API.
>> It is missing implementations to map from, say, method modifiers to access
>> flags, taking into account overlaps in bit positions.
>>
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in
>> once the API is further along.
>
> 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/74cd2687...05cf2d8b
src/java.base/share/classes/java/lang/reflect/Member.java line 96:
> 94: */
> 95: public default Set accessFlags() {
> 96: return Set.of();
Is is not better to throw a NoSuchMethodError instead of Set.of() if there is
no implementation.
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v4]
On Tue, 15 Feb 2022 21:05:08 GMT, Joe Darcy wrote: >> Note that the presence or absence of `ACC_SUPER` has no effect since **Java >> 8**, which always treats it as set regardless of the actual contents of the >> binary class file. > > For completeness, I think including SUPER is reasonable, even though has been > a no-op for some time. (Some time in the future, there could be a class file > version aware additions to this enum class.) Well spotted omission. `ACC_SUPER`may be reconed as `ACC_IDENTITY`by Valhalla, so i'm not sure it's a good idea to add ACC_SUPER. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 00:35:24 GMT, Joe Darcy wrote:
>> This is an early review of changes to better model JVM access flags, that is
>> "modifiers" like public, protected, etc. but explicitly at a VM level.
>>
>> Language level modifiers and JVM level access flags are closely related, but
>> distinct. There are concepts that overlap in the two domains (public,
>> private, etc.), others that only have a language-level modifier (sealed),
>> and still others that only have an access flag (synthetic).
>>
>> The existing java.lang.reflect.Modifier class is inadequate to model these
>> subtleties. For example, the bit positions used by access flags on different
>> kinds of elements overlap (such as "volatile" for fields and "bridge" for
>> methods. Just having a raw integer does not provide sufficient context to
>> decode the corresponding language-level string. Methods like
>> Modifier.methodModifiers() were introduced to cope with this situation.
>>
>> With additional modifiers and flags on the horizon with projects like
>> Valhalla, addressing the existent modeling deficiency now ahead of time is
>> reasonable before further strain is introduced.
>>
>> This PR in its current form is meant to give the overall shape of the API.
>> It is missing implementations to map from, say, method modifiers to access
>> flags, taking into account overlaps in bit positions.
>>
>> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in
>> once the API is further along.
>
> 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 32 additional commits since
> the last revision:
>
> - Target JDK 20 rather than 19.
> - Merge branch 'master' into JDK-8266670
> - Add mask values to constants' javadoc.
> - Implement review feedback from mlchung.
> - Fix type in @throws tag.
> - Merge branch 'master' into JDK-8266670
> - Respond to review feedback.
> - Merge branch 'master' into JDK-8266670
> - Make workding changes suggested in review feedback.
> - Merge branch 'master' into JDK-8266670
> - ... and 22 more:
> https://git.openjdk.java.net/jdk/compare/1545e825...05cf2d8b
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 130:
> 128: MANDATED(AccessFlag.MANDATED.mask());
> 129:
> 130: private int mask;
it should be declared final ?
src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 134:
> 132: this.mask = mask;
> 133: }
> 134: private int mask() {return mask;}
seem useless, `mask` can be accessed directly
-
PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8262889: Compiler implementation for Record Patterns [v7]
On Wed, 25 May 2022 04:20:35 GMT, Jan Lahoda wrote: >> 8262889: Compiler implementation for Record Patterns >> >> A first version of a patch that introduces record patterns into javac as a >> preview feature. For the specification, please see: >> http://cr.openjdk.java.net/~gbierman/jep427+405/jep427+405-20220426/specs/patterns-switch-record-patterns-jls.html >> >> There are two notable tricky parts: >> -in the parser, it was necessary to improve the `analyzePattern` method to >> handle nested/record patterns, while still keeping error recovery reasonable >> -in the `TransPatterns`, the desugaring of the record patterns is very >> straightforward - effectivelly the record patterns are desugared into >> guards/conditions. This will likely be improved in some future >> version/preview >> >> `MatchException` has been extended to cover additional cases related to >> record patterns. > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 34 commits: > > - Merge branch 'master' into patterns-record-deconstruction3 > - Post merge fix. > - Merge branch 'master' into patterns-record-deconstruction3 > - Fixing (non-)exhaustiveness for enum. > - Merge branch 'type-pattern-third' into patterns-record-deconstruction3 > - Merge branch 'master' into type-patterns-third > - Using Type.isRaw instead of checking the AST structure. > - Exhaustiveness should accept supertypes of the specified type. > - Renaming the features from deconstruction pattern to record pattern. > - Fixing guards after record patterns. > - ... and 24 more: > https://git.openjdk.java.net/jdk/compare/742644e2...f49d3f0a Yai ! champagne (at least virtual). - PR: https://git.openjdk.java.net/jdk/pull/8516
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v5]
On Wed, 18 May 2022 11:27:24 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 118 commits: > > - Merge branch 'JEP-19-ASM' of https://github.com/JornVernee/jdk into > JEP-19-ASM > - Merge branch 'pr/7959' into JEP-19-ASM > - Merge branch 'master' into JEP-19-VM-IMPL2 > - ifdef NOT_PRODUCT -> ifndef PRODUCT > - Missing ASSERT -> NOT_PRODUCT > - Cleanup UL usage > - Fix failure with SPEC disabled (accidentally dropped change) > - indentation > - fix space > - Merge branch 'master' into JEP-19-ASM > - ... and 108 more: > https://git.openjdk.java.net/jdk/compare/81e4bdbe...ce5c677a LGTM ! - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 11:57:01 GMT, Jorn Vernee wrote:
>> src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java
>> line 42:
>>
>>> 40:
>>> 41: private class Node {
>>> 42: private SoftReference ref;
>>
>> this code looks like a double check locking so ref should be volatile
>
> Thanks. I thought the `moniterenter` & `monitorexit` were enough here for
> visibility. I've read up on this and it looks like `volatile` is needed to
> correctly order the constructor and apply call (and contents) before the
> write to the `ref` field.
yes, otherwise another thread than the one inside the
`monitorenter`/`monitorexit` that is initializing the object can see the object
half initialized.
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Tue, 17 May 2022 08:16:32 GMT, Maurizio Cimadamore wrote: >> src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java >> line 98: >> >>> 96: private static final String CLASS_DATA_DESC = >>> methodType(Object.class, MethodHandles.Lookup.class, String.class, >>> Class.class).descriptorString(); >>> 97: private static final String RELEASE0_DESC = >>> methodType(void.class).descriptorString(); >>> 98: private static final String ACQUIRE0_DESC = >>> methodType(void.class).descriptorString(); >> >> calling methodType() is quite slow because of the cache of MethodType so >> maybe those initialization should be done explicitly in a static block to >> avoid to recompute methodType(long).descriptorString() several times > > What about using MethodTypeDesc/ClassDesc as building block? yes, it should be less expensive, the ClassDesc still need to be constructed from Class to allow refactoring. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> BootstrapMethodError -> ExceptionInInitializerError
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 943:
> 941: Z, B, S, C, I, J, F, D, L;
> 942:
> 943: static BasicType of(Class cls) {
This seems a duplication of ASM Type class for no good reason,
Type.getOpcode(ILOAD) or Type.getOpcode(IRETURN) gives you the proper variant
opcode
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee wrote: >> Hi, >> >> This PR brings over commits from the panama-foreign repo. These commits >> mostly pertain to the switch to ASM and away from MethodHandle combinators >> for the binding recipe specialization. But, there is one more commit which >> adds freeing of downcall stubs, since those changes were mostly Java as well. >> >> Thanks, >> Jorn >> >> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on >> Windows and Linux. > > Jorn Vernee has updated the pull request incrementally with one additional > commit since the last revision: > > BootstrapMethodError -> ExceptionInInitializerError src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java line 967: > 965: > 966: // unaligned constants > 967: public final static ValueLayout.OfBoolean JAVA_BOOLEAN_UNALIGNED = > JAVA_BOOLEAN; as far as i understand, those constants are accessed from the bytecode, they should be in their own class to cleanly separate the specializer part and the runtime part. - PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> BootstrapMethodError -> ExceptionInInitializerError
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 157:
> 155: }
> 156:
> 157: static MethodHandle doSpecialize(MethodHandle leafHandle,
> CallingSequence callingSequence, ABIDescriptor abi) {
I think thise method should be split in to, one version for the upcall, one for
the downcall, i do not see the point of merging the two codes.
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 816:
> 814: return;
> 815: }
> 816: if (con instanceof Integer) {
those can use the instanceof + type pattern
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8286669: Replace MethodHandle specialization with ASM in mainline [v3]
On Mon, 16 May 2022 17:40:41 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This PR brings over commits from the panama-foreign repo. These commits
>> mostly pertain to the switch to ASM and away from MethodHandle combinators
>> for the binding recipe specialization. But, there is one more commit which
>> adds freeing of downcall stubs, since those changes were mostly Java as well.
>>
>> Thanks,
>> Jorn
>>
>> Testing: `run-test-jdk_foreign` with `-Dgenerator.sample.factor=-1` on
>> Windows and Linux.
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> BootstrapMethodError -> ExceptionInInitializerError
src/java.base/share/classes/jdk/internal/foreign/abi/BindingSpecializer.java
line 98:
> 96: private static final String CLASS_DATA_DESC =
> methodType(Object.class, MethodHandles.Lookup.class, String.class,
> Class.class).descriptorString();
> 97: private static final String RELEASE0_DESC =
> methodType(void.class).descriptorString();
> 98: private static final String ACQUIRE0_DESC =
> methodType(void.class).descriptorString();
calling methodType() is quite slow because of the cache of MethodType so maybe
those initialization should be done explicitly in a static block to avoid to
recompute methodType(long).descriptorString() several times
src/java.base/share/classes/jdk/internal/foreign/abi/SoftReferenceCache.java
line 42:
> 40:
> 41: private class Node {
> 42: private SoftReference ref;
this code looks like a double check locking so ref should be volatile
-
PR: https://git.openjdk.java.net/jdk/pull/8685
Re: RFR: 8277090 : jsr166 refresh for jdk19 [v2]
On Mon, 2 May 2022 23:54:33 GMT, Doug Lea wrote:
>> src/java.base/share/classes/java/util/concurrent/FutureTask.java line 222:
>>
>>> 220: throw new IllegalStateException("Task has not
>>> completed");
>>> 221: }
>>> 222: }
>>
>> I think the code will be more readable if a switch expression and the arrow
>> syntax are used
>>
>>
>> return switch(state()) {
>> case SUCCESS -> {
>> @SuppressWarnings("unchecked")
>> V result = (V) outcome;
>> yield result;
>> }
>> case FAILED -> throw new IllegalStateException("Task completed with
>> exception");
>> ...
>> };
>
> Sorry, I don't understand how it would be more readable. I like new switch
> features because they extend the range of applicability of switch. But this
> (and the two others) are just plain vanilla enum-based switches that were
> handled well, and seem to have the same numbers of lines and structure either
> way?
It's more readable because it's now clear that the aim is to return the value
of the switch.
Also a switch expression has to be exhaustive, so there is no need for a
'default'.
-
PR: https://git.openjdk.java.net/jdk/pull/8490
Re: RFR: 8277090 : jsr166 refresh for jdk19
On Mon, 2 May 2022 10:23:01 GMT, Doug Lea wrote:
>> src/java.base/share/classes/java/util/concurrent/ExecutorService.java line
>> 138:
>>
>>> 136: * @author Doug Lea
>>> 137: */
>>> 138: public interface ExecutorService extends Executor, AutoCloseable {
>>
>> The class documentation should be expanded to explain that an
>> ExecutorService can be used with a try-with-resources
>
> Most AutoCloseables do not mention this in class-level javadocs. Is there a
> reason you think this one should?
close() is now equivalent to the method shutdownAndAwaitTermination() shown in
the javadoc so i believe , replacing it with a try-with-resources is better in
term of documenting how an executor service can be used
-
PR: https://git.openjdk.java.net/jdk/pull/8490
Re: RFR: 8277090 : jsr166 refresh for jdk19
On Sun, 1 May 2022 10:53:55 GMT, Doug Lea wrote:
> This is the jsr166 refresh for jdk19. See
> https://bugs.openjdk.java.net/browse/JDK-8285450 and
> https://bugs.openjdk.java.net/browse/JDK-8277090
src/java.base/share/classes/java/util/concurrent/ExecutorService.java line 138:
> 136: * @author Doug Lea
> 137: */
> 138: public interface ExecutorService extends Executor, AutoCloseable {
The class documentation should be expanded to explain that an ExecutorService
can be used with a try-with-resources
src/java.base/share/classes/java/util/concurrent/FutureTask.java line 222:
> 220: throw new IllegalStateException("Task has not
> completed");
> 221: }
> 222: }
I think the code will be more readable if a switch expression and the arrow
syntax are used
return switch(state()) {
case SUCCESS -> {
@SuppressWarnings("unchecked")
V result = (V) outcome;
yield result;
}
case FAILED -> throw new IllegalStateException("Task completed with
exception");
...
};
src/java.base/share/classes/java/util/concurrent/FutureTask.java line 237:
> 235: throw new IllegalStateException("Task has not
> completed");
> 236: }
> 237: }
Same here !
src/java.base/share/classes/java/util/concurrent/FutureTask.java line 247:
> 245: s = state;
> 246: }
> 247: switch (s) {
same here !
-
PR: https://git.openjdk.java.net/jdk/pull/8490
Re: RFR: 8282508: Updating ASM to 9.2 for JDK 19
On Mon, 28 Mar 2022 16:49:58 GMT, Vicente Romero wrote: > Please review this PR which is updating the ASM included in the JDK to ASM > 9.2. This version should be included in JDK19. There are basically two > commits here, one that was automatically generated and that mostly changes > file headers etc and another one, smaller, that make changes to the code to > adapt it to our needs, JDK version etc. In this second commit one can see > that two classes that were removed by the automatically generated change have > been copied back: > > jdk.internal.org.objectweb.asm.commons.RemappingMethodAdapter > jdk.internal.org.objectweb.asm.commons.RemappingAnnotationAdapter > > This is because package: `jdk.jfr.internal.instrument` needs them. > > TIA I suppose that you are raising commons.RemappingMethodAdapter and commons.RemappingAnnotationAdapter from the dead because you want to fix the code in jdk.jfr.internal.instrument later ? Otherwise it's a little dangerous because that code as started to drift, the new remapper has new code not supported by the old remapper. BTW, @deprecated on ASM source is equivalent to. @Deprecated + forRemoval in the JDK. The support of Java 19 is fine, the same code will be included in ASM soon. - PR: https://git.openjdk.java.net/jdk/pull/8000
Re: RFR: JDK-8283416: Update java.lang.invoke.MethodHandle to use sealed classes
On Tue, 22 Mar 2022 04:38:15 GMT, ExE Boss wrote:
> javac should be changed to allow fully qualified access to private inner
> classes in the permits clause of an enclosing class.
This is not how private works, private means accessible between the '{' and the
'}' of the class.
The permits clause is declared outside of the '{' and the '}' of the class,
thus a private member class is not visible from the permits clause.
-
PR: https://git.openjdk.java.net/jdk/pull/7881
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v10]
On Mon, 21 Mar 2022 05:17:31 GMT, ExE Boss wrote:
>> Jim Laskey has updated the pull request incrementally with three additional
>> commits since the last revision:
>>
>> - Typos
>> - Update Carrier.java
>> - Redo API to use list, bring Carrier.component back
>
> src/java.base/share/classes/java/lang/runtime/Carrier.java line 309:
>
>> 307: static {
>> 308: LOOKUP = MethodHandles.lookup();
>> 309: UNSAFE = Unsafe.getUnsafe();
>
> It might be better to use `java.lang.invoke.MethodHandleStatics.UNSAFE`, and
> probably also `java.lang.invoke.MethodHandles.Lookup.IMPL_LOOKUP`.
The package is java.lang.runtime not java.lang.invoke so both fields are not
accessible.
-
PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: 8283237: CallSite should be a sealed class
On Thu, 17 Mar 2022 07:32:40 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
>>
>>> 86: */
>>> 87: public
>>> 88: abstract sealed class CallSite permits ConstantCallSite,
>>> VolatileCallSite, MutableCallSite {
>>
>> Nitpicking with my JSR 292 hat,
>> given that the permits clause is reflected in the javadoc,
>> the order should be ConstantCS, MutableCS and VolatileCS,
>> it's both in the lexical order and in the "memory access" of setTarget()
>> order , from stronger access to weaker access.
>
> I agree that Constant, Mutable, Volatile order is better, ranked by the
> respective cost for `setTarget()` and (possibly) invocation, and earlier ones
> in the list are more preferable if conditions allow.
>
> However, in the current API documentation, the order is Constant, Mutable,
> and Volatile. Should I update that or leave it?
>
> /*
> *
> * If a mutable target is not required, an {@code invokedynamic}
> instruction
> * may be permanently bound by means of a {@linkplain ConstantCallSite
> constant call site}.
> * If a mutable target is required which has volatile variable semantics,
> * because updates to the target must be immediately and reliably witnessed
> by other threads,
> * a {@linkplain VolatileCallSite volatile call site} may be used.
> * Otherwise, if a mutable target is required,
> * a {@linkplain MutableCallSite mutable call site} may be used.
> *
> */
For me, this is unrelated, for this paragraph it's easier to explain the
semantics of MutableCallSite with an otherwise, i.e. it's mutable but you do
not want the cost of a volatile acces.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: 8283237: CallSite should be a sealed class
On Wed, 16 Mar 2022 13:09:30 GMT, liach wrote:
> Change `CallSite` to a sealed class, as `CallSite` is an abstract class which
> does not allow direct subclassing by users per its documentation. Since I
> don't have a JBS account, I posted the content for the CSR in a GitHub Gist
> at https://gist.github.com/150d5aa7f8b13a4deddf95969ad39d73 and wish someone
> can submit a CSR for me.
src/java.base/share/classes/java/lang/invoke/CallSite.java line 88:
> 86: */
> 87: public
> 88: abstract sealed class CallSite permits ConstantCallSite,
> VolatileCallSite, MutableCallSite {
Nitpicking with my JSR 292 hat,
given that the permits clause is reflected in the javadoc,
the order should be ConstantCS, MutableCS and VolatileCS,
it's both in the lexical order and in the "memory access" of setTarget() order
, from stronger access to weaker access.
-
PR: https://git.openjdk.java.net/jdk/pull/7840
Re: RFR: JDK-8281766: Update java.lang.reflect.Parameter to implement Member
On Tue, 15 Feb 2022 01:13:43 GMT, Joe Darcy wrote: > Retrofitting of Parameter to implement Member, an interface already > implemented by similar reflective modeling classes. > > Since Parameter is final, there is little compatibility impact from adding a > new method. > > Please also review the corresponding CSR: > > https://bugs.openjdk.java.net/browse/JDK-8281767 > > I'll update the copyright year before pushing; I judged this as trivial > enough to not require a regression test. I agree with Mandy and David, a Member is the thingy inside a class, so a parameter is no a member, it has no declaring class. - PR: https://git.openjdk.java.net/jdk/pull/7468
Re: RFR: JDK-8280168: Add Objects.toIdentityString [v7]
On Mon, 24 Jan 2022 21:31:37 GMT, Joe Darcy wrote: >> While it is strongly recommend to not use the default toString for a class, >> at times it is the least-bad alternative. When that alternative needs to be >> used, it would be helpful to have the implementation already available, such >> as in Objects.toDefaultString(). This method is analagous to >> System.identityHashCode. >> >> Please also review the CSR: https://bugs.openjdk.java.net/browse/JDK-8280184 > > 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 eight additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8280168 > - Merge branch 'master' into JDK-8280168 > - Appease jcheck. > - Add toIdentityString > - Respond to review feedback to augment test. > - Respond to review feedback. > - JDK-8280168 Add Objects.toDefaultString toIdentityString is a better name than toDefaultString. It's fine for me but given that "identity" has a slightly different meaning in the context of Valhalla that in System.identityHashCode(), it may be good to ask Brian about that name. - PR: https://git.openjdk.java.net/jdk/pull/7139
Re: RFR: 8261847: performace of java.lang.Record::toString should be improved
On Tue, 16 Nov 2021 13:03:35 GMT, Jim Laskey wrote: >> (I'm not reviewer.) >> >> I think `.toArray(Class[]::new)` should be better here. `.toList` seems >> unnecessary. > > Class[] types = Stream.of(getters) > .map(g -> g.type().returnType()) > .toArray(Class[]::new); or do not use a stream but a simple loop, codes in bootstrap methods should be low-level - PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v13]
On Thu, 14 Oct 2021 00:54:57 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleAccessorFactory.java
>> line 151:
>>
>>> 149: var setter = isReadOnly ? null :
>>> JLIA.unreflectField(field, true);
>>> 150: Class type = field.getType();
>>> 151: if (type == Boolean.TYPE) {
>>
>> dumb question: any reason why `boolean.class` (which is compiled to a LDC)
>> is not used?
>
> I only see `boolean.class` compiled to `getstatic Boolean.TYPE`. Is there a
> javac flag to compile it to a LDC?
The LDC bytecode instruction for a class takes a class name not a descriptor as
parameter, so there is no way to encode LDC Z. Valhalla may change that.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable [v4]
On Mon, 4 Oct 2021 15:49:43 GMT, Peter Levart wrote: >> This patch improves reflective access speed as shown by the included >> benchmarks: >> >> https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a >> >> ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection >> with Method Handle) perform better in some circumstances. > > Peter Levart has updated the pull request incrementally with one additional > commit since the last revision: > > More @stable fields in Constructor/Field to align them with Method fields I miss that, that very cool. - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8274299: Make Method/Constructor/Field accessors @Stable [v4]
On Mon, 4 Oct 2021 15:49:43 GMT, Peter Levart wrote: >> This patch improves reflective access speed as shown by the included >> benchmarks: >> >> https://jmh.morethan.io/?gists=902f4b43519c4f96c7abcd14cdc2d27d,ac490481e3001c710d75d6071c10b23a >> >> ... and is also a prerequisite to make JEP 416 (Reimplement Core Reflection >> with Method Handle) perform better in some circumstances. > > Peter Levart has updated the pull request incrementally with one additional > commit since the last revision: > > More @stable fields in Constructor/Field to align them with Method fields Can you surreptitiously add "java.lang" too (asking for a friend) - PR: https://git.openjdk.java.net/jdk/pull/5694
Re: RFR: 8272137: Make Iterable classes streamable
On Mon, 9 Aug 2021 12:28:23 GMT, CC007 wrote: > create Streamable and ParallelStreamable interface and use them in Collection > and Optional Hi Rick, I do not think that such interfaces are a good addition to the JDK, we do not want to promote the fact that you can create a Stream from an Iterable because the resulting Stream is not efficient (or is not as efficient as it could be) and we can already uses a Supplier if we want to to represent a factory of streams See my comment on the bug for more details - PR: https://git.openjdk.java.net/jdk/pull/5050
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310) and generally fixes the footprint regression, it may > increase MaxRSS slightly compared to the version before JDK-8266310, which is > shown in the below graphs. (updated) > >  > >  > > (update: added ZGC graphs) > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. Hi Sergey, thanks for the explanation, i don't think there is a need for more data with -Xint. Using anonymous classes instead of lambdas is good for me. - PR: https://git.openjdk.java.net/jdk/pull/4893
Re: RFR: 8270321: Startup regressions in 18-b5 caused by JDK-8266310
On Fri, 23 Jul 2021 18:03:31 GMT, Sergey Chernyshev wrote: > Dear colleagues, > > Please review the patch that replaces the lambdas with anonymous classes > which solves the startup time regression as shown below. > > I attached the Bytestacks flamegraphs for both original (regression) and > fixed versions. The flamegraphs clearly show the lambdas were causing the > performance issue. > > [bytestacks_flamegraphs.zip](https://github.com/openjdk/jdk/files/6870446/bytestacks_flamegraphs.zip) > > Although the proposed JDK-8270321 patch fixes the startup time (it might > appear even better than it was before the regression was introduced, i.e. > before JDK-8266310), it increases MaxRSS slightly compared to the version > before JDK-8266310, which is shown in the below graphs. > >  > >  > > I additionally include the heap objects histograms to show the change does > not increase the total live objects size significantly with only 1000 bytes > the total difference, namely 1116128 bytes in 25002 live objects after the > proposed fix JDK-8270321 compared to 1115128 bytes in 24990 objects in the > version with the original patch reverted (i.e. before JDK-8266310). > > [histograms.zip](https://github.com/openjdk/jdk/files/6870457/histograms.zip) > > The patch was tested w/hotspot/tier1/tier2 test groups. I don't understand your analysis, you are testing the startup time with -Xint which disable JITs, but there is no mention of -Xint in the bug report. It's obvious to me that there is a regression with -Xint given that the lambda creation is using invokedynamic which is only optimizable by JITs. I think you should first try to reproduce the issue with the correct flags and then follow the advice from Mandy on how to implement the fix. Using an anonymous class may introduce more allocation than using a lambda once the code is JITed. - PR: https://git.openjdk.java.net/jdk/pull/4893
Re: RFR: 6506405: Math.abs(float) is slow
On Wed, 7 Jul 2021 22:22:45 GMT, Joe Darcy wrote: > > > Your patch change the semantics, actually > > `Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE` > > with your patch > > `Math.abs(Integer.MIN_VALUE) == Integer.MAX_VALUE` > > Does it? The overriding of int arguments shouldn't be affected. You are right, it does not change abs(int) so it should be Ok. > > -Joe - PR: https://git.openjdk.java.net/jdk/pull/4711
Re: RFR: 6506405: Math.abs(float) is slow
On Wed, 7 Jul 2021 20:28:37 GMT, Brian Burkhalter wrote: > Please consider this change to make the `float` and `double` versions of > `java.lang.Math.abs()` branch-free. Your patch change the semantics, actually `Math.abs(Integer.MIN_VALUE) == Integer.MIN_VALUE` with your patch `Math.abs(Integer.MIN_VALUE) == Integer.MAX_VALUE` - PR: https://git.openjdk.java.net/jdk/pull/4711
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Fri, 18 Jun 2021 09:08:04 GMT, Jan Lahoda wrote:
>> src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:
>>
>>> 220: String invocationName,
>>> 221: MethodType invocationType,
>>> 222: Object... labels) throws
>>> Throwable {
>>
>> Is it not better to take a Class and a String... as separated parameters
>> instead of taking Object... and doing the conversion to a Class and an array
>> of String later in Java
>
> This is to represent cases like:
>
> E sel = null;
> switch (sel) {
> case A -> {}
> case E e && "B".equals(e.name()) -> {}
> case C -> {}
> case E e -> {}
> }
>
>
> The method needs to know which cases represent constants and which represent
> patterns (even though the primary type of all the patterns will be the enum
> type), so we cannot easily put the `Class` first (or elide it), and then a
> `String...`, unless we represent the patterns in the `String...` array
> somehow.
Ok got it,
At some point, we will have to represent patterns either as String and parse
them or as Condy of condy if we want a more structured recursive way.
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: [jdk17] RFR: 8268766: Desugaring of pattern matching enum switch should be improved [v2]
On Thu, 17 Jun 2021 18:33:56 GMT, Jan Lahoda wrote:
>> Currently, an enum switch with patterns is desugared in a very non-standard,
>> and potentially slow, way. It would be better to use the standard
>> `typeSwitch` bootstrap to classify the enum constants. The bootstrap needs
>> to accept enum constants as labels in order to allow this. A complication is
>> that if an enum constant is missing, that is not an incompatible change for
>> the switch, and the switch should simply work as if the case for the missing
>> constant didn't exist. So, the proposed solution is to have a new bootstrap
>> `enumConstant` that converts the enum constant name to the enum constant,
>> returning `null`, if the constant does not exist. It delegates to
>> `ConstantBootstraps.enumConstant` to do the actual conversion. And
>> `typeSwitch` accepts `null`s as padding.
>>
>> How does this look?
>
> Jan Lahoda has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Creating a new bootstrap method for (pattern matching) enum switches, as
> suggested.
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 222:
> 220: String invocationName,
> 221: MethodType invocationType,
> 222: Object... labels) throws Throwable
> {
Is it not better to take a Class and a String... as separated parameters
instead of taking Object... and doing the conversion to a Class and an array of
String later in Java
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 238:
> 236: MethodHandle target =
> 237: MethodHandles.insertArguments(DO_ENUM_SWITCH, 2,
> (Object) labels);
> 238: target = MethodHandles.explicitCastArguments(target,
> invocationType);
why explicitCast is used here instead of the classical cast asType() ?
-
PR: https://git.openjdk.java.net/jdk17/pull/81
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v13]
On Fri, 4 Jun 2021 20:20:26 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with two additional > commits since the last revision: > > - Applying review feedback. > - Tweaking javadoc. Dynamic constants are needed when de-structuring classes that are not record at top-level, to make the type that will carry the bindings, from invokedynamic to where they are accessed, opaque. So dynamic constants are not needed yet ! - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 11:01:02 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> packages to make use of the switch expressions? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8268124: small refactoring; fixed misplaced comment and added missing > lambda operator Looks good to me - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions [v3]
On Thu, 3 Jun 2021 10:57:16 GMT, Patrick Concannon wrote: > My mistake. I've replaced the colon now with the lambda operator. Drive by comment, in term of name, `->` is the arrow operator not the lambda operator. - lambda = parameters + arrow + code - arrow case = case + arrow + code The difference is important because a lambda is a function while an arrow case is a block. - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 17:12:21 GMT, Stuart Marks wrote: > A quick search reveals that Guava has a public subclass of > SimpleImmutableEntry: > https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html >There are possibly others. It doesn't seem worth the incompatibility to me, as >it would break stuff in order to have only a "cleaner" meaning for >"immutable." Also, there are other classes in the JDK that claim they are >immutable but which can be subclassed, e.g., BigInteger. I don't see a good >way of fixing this. Thanks, i've done some digging too, I foolishly hoped that nobody would subclass a class with `Immutable` in its name, oh boy i was wrong :) So documenting the existing behavior seems the only possible way. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/runtime/ObjectMethods.java line 366:
> 364: }
> 365: default -> throw new IllegalArgumentException(methodName);
> 366: };
I thinki it's simpler to have something like that
var handle = switch(methodName) {
...
};
return methodType != null ? new ConstantCallSite(handle) : handle;
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/MethodHandleProxies.java line 278:
> 276: private static boolean isObjectMethod(Method m) {
> 277: return switch (m.getName()) {
> 278: case "toString" -> (m.getReturnType() == String.class
the extra parenthesis are not needed
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon wrote: > Hi, > > Could someone please review my code for updating the code in the `java.lang` > packages to make use of the switch expressions? > > Kind regards, > Patrick src/java.base/share/classes/java/lang/invoke/MemberName.java line 331: > 329: assert(false) : this+" != > "+MethodHandleNatives.refKindName((byte)originalRefKind); > 330: yield true; > 331: } this code always yield true, better to check if the assert are enabled, do the switch in that case and always return true - PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8268124: Update java.lang to use switch expressions
On Wed, 2 Jun 2021 15:25:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.lang`
> packages to make use of the switch expressions?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/lang/invoke/InvokerBytecodeGenerator.java line
1663:
> 1661: case D_TYPE -> mv.visitInsn(Opcodes.DCONST_0);
> 1662: case L_TYPE -> mv.visitInsn(Opcodes.ACONST_NULL);
> 1663: default -> throw new InternalError("unknown type: " + type);
perhaps
mv.visitInsn(switch(type) { ...
-
PR: https://git.openjdk.java.net/jdk/pull/4312
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 00:39:25 GMT, Stuart Marks wrote: > I'm fixing this along with a couple intertwined issues. > > 1. Add Map.Entry::copyOf as an idempotent copy operation. > > 2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not > really immutable) but that subclasses can be modifiable. > > 3. Clarify some confusing, historical wording about Map.Entry instances being > obtainable only via iteration of a Map's entry-set view. This was never > really true, since anyone could implement the Map.Entry interface, and it > certainly was not true since JDK 1.6 when SimpleEntry and > SimpleImmutableEntry were added. LGTM, i wonder if we should not declare SimpleImmutableEntry final, while it's not a backward compatible change, it's may be better on the long term because SimpleImmutableEntry is already used as an immutable type, so instead of documenting the fact that SimpleImmutableEntry is not declared final thus SimpleImmutableEntry as a type does not guarantte shallow immutability, it may be better to fix the root cause. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v5]
On Wed, 26 May 2021 17:52:36 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 12 commits: > > - Post-merge fix - need to include jdk.internal.javac in the list of > packages used by jdk.compiler again, as we now (again) have a preview feature > in javac. > - Correcting LineNumberTable for rule switches. > - Merging master into JDK-8262891 > - Fixing various error-related bugs. > - Avoiding fall-through from the total case to a synthetic default but > changing total patterns to default. > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. > - Trailing whitespaces. > - Cleanup, reflecting review comments. > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/083416d3...fd748501 > > I've updated the code to produce better/more useful LineNumberTable for > > rule switches. > > @lahodaj I re-tested previous example and tested few others. Now everything > seems to be good with `LineNumberTable` entries +1 > [...] > Don't know about importance of this, and whether this was expected, but > decided to mention. Look likes a mistake for me, you only need a StackFrame in front of the call to invokedynamic if it's the target of a goto and in your example, there is no guard so no backward goto. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 16:14:56 GMT, Jan Lahoda wrote: > I'd like to note this is a preview feature - we can change the desugaring. At > the same time, I don't think this does not work with sub-patterns (those can > be easily desugared to guards, I think). Yes, but in that case the classcheck du to a sub-pattern matching will be either an instanceof or some other invokedynamic. Having several indy will bloat the code and if we use instanceof, why not using instanceof all along the way. > Regarding efficiency, it may be a balance between classfile overhead (which > will be higher if we need to desugar every guard to a separate method), and > the possibility to tweak the matching at runtime. I fear more the 2 bytes length bytecode limit of a method than the constant pool length limit mostly because people are already using a bunch of lambdas to simulate pattern matching. - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 14:22:57 GMT, Jan Lahoda wrote:
> The reason for this integer (which is not a constant in the case of this
> switch) is to restart the matching in case guards fail to "match".
> Considering the example here:
>
> ```
> class Example {
> void example(Object o) {
> switch (o) {
> case String s && s.length() == 0 ->
> System.out.println("1st case");
> case String s && s.length() == 1 -> // line 6
> System.out.println("2nd case"); // line 7
> case String s -> // line 8
> System.out.println("3rd case"); // line 9
> default -> // line 10
> System.out.println("default case"); // line 11
> }
> }
> }
> ```
>
> If `o` is `String`, then the first call to indy will be `indy[...](o, 0)`,
> returning `0`. Then the guard will be evaluated `s.length() == 0`. If the
> length is not zero, the local variable 3 will be reassigned to `1`(bytecode
> index 58, 59) and the whole switch is restarted - just this time, the
> matching in the indy will start at index `1`, not `0`, i.e. `indy[...](o,
> 1)`. And so on. I believe there is a text explaining the meaning of the
> parameter in the javadoc of the bootstrap, and in TransPatterns in javac.
The problem with this design is that calling example("foo") forces the VM will
do 6 checkcast String on "foo", and it doesn't work with sub-patterns.
Desugaring the guards as static method like with lambdas seems more promising,
indy can be called with the pairs [String, MethodHandle(s -> s.length() == 0)],
[String, MethodHandle(s -> s.length() == 0)] and [_,_] (with the guards passed
as a constant method handles again like lambdas are desugared).
It means that the indy needs to capture all local variables that are used in
the guards, instead of passing an int.
I believe that a translation using constant method handles for guards is far
more efficient than using an int and a backward branch but it has a higher cost
in term of runtime data structures.
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v4]
On Tue, 25 May 2021 12:20:02 GMT, Jan Lahoda wrote: > Thanks Evgeny, I'll take a look. > > @forax, do you mean why there is "0" in: > 11: invokedynamic #13, 0 > ? Not this one, the one on the stack. 7: iconst_0 < this zero 8: istore_3 9: aload_2 10: iload_3 11: invokedynamic #13, 0 // InvokeDynamic #0:typeSwitch:(Ljava/lang/Object;I)I Why the descriptor is (Ljava/lang/Object;I)I instead of (Ljava/lang/Object;)I, what is the semantics associated to that integer ? > The "0" is an artifact of how invokedynamic is represented in the classfile > (invokedynamic, 2 bytes of constant pool reference, byte 0, byte 0) - javap > shows the value of the first zero byte. That is probably not needed anymore, > but there is nothing special in this patch, I think - all invokedynamic calls > look like this, AFAIK. I know that a little to well, i'm one of the guys behind the design of indy :) - PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 20:37:57 GMT, Roger Riggs wrote: > OutputStreamWriter would be a better choice with that in mind. It does not > have the convenience methods for converting various types to strings but > would not hide the exceptions. Developers could wrap it in a PrintWriter to > get the convenience and take on the responsibility of dealing with exceptions > by polling. yes, instead of OutputStreamWriter, i wonder if BufferedWriter is not better given that reader part uses BufferedReader and that is mirror java.nio.file.Files which also uses BufferedReader/BufferedWriter as types for the pair reader/writer. - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8191441: (Process) add Readers and Writer access to java.lang.Process streams
On Thu, 20 May 2021 19:57:22 GMT, Roger Riggs wrote: > Methods are added to java.lang.Process to read and write characters and lines > from and to a spawned Process. > The Charset used to encode and decode characters to bytes can be specified or > use the > operating system native encoding as is available from the "native.encoding" > system property. I've added the same comment on the bug itself. I don't think that using PrintWriter is a good idea here given that a PrintWriter shallow the IOException - PR: https://git.openjdk.java.net/jdk/pull/4134
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource my bad - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8266846: Add java.time.InstantSource [v3]
On Tue, 18 May 2021 23:18:42 GMT, Stephen Colebourne wrote: >> 8266846: Add java.time.InstantSource > > Stephen Colebourne has updated the pull request incrementally with one > additional commit since the last revision: > > 8266846: Add java.time.InstantSource It's a side effect of JEP 403, i believe - PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview) [v2]
On Mon, 17 May 2021 19:04:11 GMT, Jan Lahoda wrote: >> This is a preview of a patch implementing JEP 406: Pattern Matching for >> switch (Preview): >> https://bugs.openjdk.java.net/browse/JDK-8213076 >> >> The current draft of the specification is here: >> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html >> >> A summary of notable parts of the patch: >> -to support cases expressions and patterns in cases, there is a new common >> superinterface for expressions and patterns, `CaseLabelTree`, which >> expressions and patterns implement, and a list of case labels is returned >> from `CaseTree.getLabels()`. >> -to support `case default`, there is an implementation of `CaseLabelTree` >> that represents it (`DefaultCaseLabelTree`). It is used also to represent >> the conventional `default` internally and in the newly added methods. >> -in the parser, parenthesized patterns and expressions need to be >> disambiguated when parsing case labels. >> -Lower has been enhanced to handle `case null` for ordinary >> (boxed-primitive, String, enum) switches. This is a bit tricky for boxed >> primitives, as there is no value that is not part of the input domain so >> that could be used to represent `case null`. Requires a bit shuffling with >> values. >> -TransPatterns has been enhanced to handle the pattern matching switch. It >> produces code that delegates to a new bootstrap method, that will classify >> the input value to the switch and return the case number, to which the >> switch then jumps. To support guards, the switches (and the bootstrap >> method) are restartable. The bootstrap method as such is written very simply >> so far, but could be much more optimized later. >> -nullable type patterns are `case String s, null`/`case null, String >> s`/`case null: case String s:`/`case String s: case null:`, handling of >> these required a few tricks in `Attr`, `Flow` and `TransPatterns`. >> >> The specdiff for the change is here (to be updated): >> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html > > Jan Lahoda has updated the pull request incrementally with three additional > commits since the last revision: > > - Reflecting recent spec changes. > - Reflecting review comments. > - Reflecting review comments on SwitchBootstraps. It's not clear to me if it's the first change and several will follow or not. The code looks good but the metaprotocol is not the right one IMO, i would prefer the one we discuss in April https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-April/002992.html But this can be tackle in another PR - Marked as reviewed by fo...@github.com (no known OpenJDK username). PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8266846: Add java.time.InstantSource
On Mon, 17 May 2021 07:31:45 GMT, Stephen Colebourne
wrote:
>> src/java.base/share/classes/java/time/InstantSource.java line 93:
>>
>>> 91: * @since 17
>>> 92: */
>>> 93: public interface InstantSource {
>>
>> Should not we add `@FunctionalInterface`? I can easily imagine this
>> interface being used in tests where we can define the `InstantSource` with
>> lambdas.
>
> `@FunctionalInterface` isn't required for use by lambdas. I wasn't initially
> convinced using it here was the right choice.
Right, it's about signaling that it's safe to be used with a
lambda/method-reference, so it's not required but a nice to have, very like
@Override.
-
PR: https://git.openjdk.java.net/jdk/pull/4016
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote:
> This is a preview of a patch implementing JEP 406: Pattern Matching for
> switch (Preview):
> https://bugs.openjdk.java.net/browse/JDK-8213076
>
> The current draft of the specification is here:
> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>
> A summary of notable parts of the patch:
> -to support cases expressions and patterns in cases, there is a new common
> superinterface for expressions and patterns, `CaseLabelTree`, which
> expressions and patterns implement, and a list of case labels is returned
> from `CaseTree.getLabels()`.
> -to support `case default`, there is an implementation of `CaseLabelTree`
> that represents it (`DefaultCaseLabelTree`). It is used also to represent the
> conventional `default` internally and in the newly added methods.
> -in the parser, parenthesized patterns and expressions need to be
> disambiguated when parsing case labels.
> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive,
> String, enum) switches. This is a bit tricky for boxed primitives, as there
> is no value that is not part of the input domain so that could be used to
> represent `case null`. Requires a bit shuffling with values.
> -TransPatterns has been enhanced to handle the pattern matching switch. It
> produces code that delegates to a new bootstrap method, that will classify
> the input value to the switch and return the case number, to which the switch
> then jumps. To support guards, the switches (and the bootstrap method) are
> restartable. The bootstrap method as such is written very simply so far, but
> could be much more optimized later.
> -nullable type patterns are `case String s, null`/`case null, String s`/`case
> null: case String s:`/`case String s: case null:`, handling of these required
> a few tricks in `Attr`, `Flow` and `TransPatterns`.
>
> The specdiff for the change is here (to be updated):
> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 127:
> 125: Stream.of(labels).forEach(SwitchBootstraps::verifyLabel);
> 126:
> 127: return new TypeSwitchCallSite(invocationType, labels);
See why below
MethodHandle target = MethodHandles.insertArguments(DO_SWITCH, 2, labels);
return new ConstantCallsite(target);
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 134:
> 132: throw new IllegalArgumentException("null label found");
> 133: }
> 134: if (label.getClass() != Class.class &&
store `label.getClass` in a local variable,
it's too bad that you can no use pattern matching here :)
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 141:
> 139: }
> 140:
> 141: static class TypeSwitchCallSite extends ConstantCallSite {
That's weird, having a callsite extending MutableCallSite is expected but
having a callsite extending constant callsite is useless because you can not
change it after being constructed.
As an interesting trivia, the VM does not store the CallSite returned by the
BSM, but only the target inside it.
So there is no point of keeping a ConstantCallSite around.
So `doSwitch()` should be static and takes the array of Object[] as parameter,
will array will be injected with an insertArguments().
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 157:
> 155: Class targetClass = target.getClass();
> 156: for (int i = startIndex; i < labels.length; i++) {
> 157: if (labels[i] instanceof Class) {
label[i] should be stored is a local variable and
using `instanceof Class c` (like the other instanceof below) will make the
code more readable
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Thu, 8 Apr 2021 18:51:21 GMT, Jorn Vernee wrote:
> This patch adds a `tableSwitch` combinator that can be used to switch over a
> set of method handles given an index, with a fallback in case the index is
> out of bounds, much like the `tableswitch` bytecode. Here is a description of
> how it works (copied from the javadoc):
>
> Creates a table switch method handle, which can be used to switch over a
> set of target
> method handles, based on a given target index, called selector.
>
> For a selector value of {@code n}, where {@code n} falls in the range
> {@code [0, N)},
> and where {@code N} is the number of target method handles, the table
> switch method
> handle will invoke the n-th target method handle from the list of target
> method handles.
>
> For a selector value that does not fall in the range {@code [0, N)}, the
> table switch
> method handle will invoke the given fallback method handle.
>
> All method handles passed to this method must have the same type, with
> the additional
> requirement that the leading parameter be of type {@code int}. The
> leading parameter
> represents the selector.
>
> Any trailing parameters present in the type will appear on the returned
> table switch
> method handle as well. Any arguments assigned to these parameters will
> be forwarded,
> together with the selector value, to the selected method handle when
> invoking it.
>
> The combinator does not support specifying the starting index, so the switch
> cases always run from 0 to however many target handles are specified. A
> starting index can be added manually with another combination step that
> filters the input index by adding or subtracting a constant from it, which
> does not affect performance. One of the reasons for not supporting a starting
> index is that it allows for more lambda form sharing, but also simplifies the
> implementation somewhat. I guess an open question is if a convenience
> overload should be added for that case?
>
> Lookup switch can also be simulated by filtering the input through an
> injection function that translates it into a case index, which has also
> proven to have the ability to have comparable performance to, or even better
> performance than, a bytecode-native `lookupswitch` instruction. I plan to add
> such an injection function to the runtime libraries in the future as well.
> Maybe at that point it could be evaluated if it's worth it to add a lookup
> switch combinator as well, but I don't see an immediate need to include it in
> this patch.
>
> The current bytecode intrinsification generates a call for each switch case,
> which guarantees full inlining of the target method handles. Alternatively we
> could only have 1 callsite at the end of the switch, where each case just
> loads the target method handle, but currently this does not allow for
> inlining of the handles, since they are not constant.
>
> Maybe a future C2 optimization could look at the receiver input for
> invokeBasic call sites, and if the input is a phi node, clone the call for
> each constant input of the phi. I believe that would allow simplifying the
> bytecode without giving up on inlining.
>
> Some numbers from the added benchmarks:
>
> Benchmark(numCases) (offset)
> (sorted) Mode Cnt Score Error Units
> MethodHandlesTableSwitchConstant.testSwitch 5 0
> N/A avgt 30 4.186 � 0.054 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 5 150
> N/A avgt 30 4.164 � 0.057 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 0
> N/A avgt 30 4.124 � 0.023 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 10 150
> N/A avgt 30 4.126 � 0.025 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 0
> N/A avgt 30 4.137 � 0.042 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 25 150
> N/A avgt 30 4.113 � 0.016 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 0
> N/A avgt 30 4.118 � 0.028 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 50 150
> N/A avgt 30 4.127 � 0.019 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 0
> N/A avgt 30 4.116 � 0.013 ms/op
> MethodHandlesTableSwitchConstant.testSwitch 100 150
> N/A avgt 30 4.121 � 0.020 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 0
> N/A avgt 30 4.113 � 0.009 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 5 150
> N/A avgt 30 4.149 � 0.041 ms/op
> MethodHandlesTableSwitchOpaqueSingle.testSwitch 10 0
> N/A avgt 30 4.121 � 0.026 ms/op
>
Re: RFR: 8262891: Compiler implementation for Pattern Matching for switch (Preview)
On Tue, 4 May 2021 16:41:44 GMT, Jan Lahoda wrote:
> This is a preview of a patch implementing JEP 406: Pattern Matching for
> switch (Preview):
> https://bugs.openjdk.java.net/browse/JDK-8213076
>
> The current draft of the specification is here:
> http://cr.openjdk.java.net/~gbierman/jep406/jep406-20210430/specs/patterns-switch-jls.html
>
> A summary of notable parts of the patch:
> -to support cases expressions and patterns in cases, there is a new common
> superinterface for expressions and patterns, `CaseLabelTree`, which
> expressions and patterns implement, and a list of case labels is returned
> from `CaseTree.getLabels()`.
> -to support `case default`, there is an implementation of `CaseLabelTree`
> that represents it (`DefaultCaseLabelTree`). It is used also to represent the
> conventional `default` internally and in the newly added methods.
> -in the parser, parenthesized patterns and expressions need to be
> disambiguated when parsing case labels.
> -Lower has been enhanced to handle `case null` for ordinary (boxed-primitive,
> String, enum) switches. This is a bit tricky for boxed primitives, as there
> is no value that is not part of the input domain so that could be used to
> represent `case null`. Requires a bit shuffling with values.
> -TransPatterns has been enhanced to handle the pattern matching switch. It
> produces code that delegates to a new bootstrap method, that will classify
> the input value to the switch and return the case number, to which the switch
> then jumps. To support guards, the switches (and the bootstrap method) are
> restartable. The bootstrap method as such is written very simply so far, but
> could be much more optimized later.
> -nullable type patterns are `case String s, null`/`case null, String s`/`case
> null: case String s:`/`case String s: case null:`, handling of these required
> a few tricks in `Attr`, `Flow` and `TransPatterns`.
>
> The specdiff for the change is here (to be updated):
> http://cr.openjdk.java.net/~jlahoda/8265981/specdiff.preview.01/overview-summary.html
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 77:
> 75: }
> 76:
> 77: private static MethodHandle typeInitHook(T
> receiver) {
There is no point to have a type parameter here,
private static MethodHandle typeInitHook(CallSite receiver) {
will work the same
src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java line 131:
> 129:
> 130: private static void verifyLabel(Object label) {
> 131: if (Objects.isNull(label)) {
`if (label == true) {` is more readable as said in the javadoc of
`Objects.isNull`
-
PR: https://git.openjdk.java.net/jdk/pull/3863
Re: RFR: 8264777: Overload optimized FileInputStream::readAllBytes [v2]
On Wed, 5 May 2021 06:50:17 GMT, Vladimir Sitnikov
wrote:
>> Brian Burkhalter has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8264777: Handle cases where length() returns zero
>
> src/java.base/share/classes/java/io/FileInputStream.java line 284:
>
>> 282: long size = length - position;
>> 283: if (size > (long)Integer.MAX_VALUE)
>> 284: throw new OutOfMemoryError("Required array size too
>> large");
>
> What do you think of adding "length, position, and size" to the exception
> message?
We usually don't do that for OutOfMemoryError because concatenation implies
allocation
-
PR: https://git.openjdk.java.net/jdk/pull/3845
Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v6]
On Wed, 21 Apr 2021 13:13:16 GMT, Jim Laskey wrote: >> Move makeXXXSpilterator from public (@hidden) to protected. No API ch > > Jim Laskey 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 eight additional commits since > the last revision: > > - Merge branch 'master' into 8265137 > - Remove @hidden > - Correct the hierarchy of Random > - Remove extraneous references to makeXXXSpliterator > - Move makeXXXSpliterator methods to RandomSupport > - change static final from 'proxy' to 'PROXY' > - Make makeXXXSpliterator final > - Move makeXXXSpilterator from public (@hidden) to protected. No API change. Looks good +1 - PR: https://git.openjdk.java.net/jdk/pull/3469
Re: RFR: 8265137: java.util.Random suddenly has new public methods nowhere documented [v2]
On Wed, 14 Apr 2021 12:27:55 GMT, Jim Laskey wrote: >> Move makeXXXSpilterator from public (@hidden) to protected. No API ch > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Make makeXXXSpliterator final I don't see the point of having those methods virtual, they can be public static in RandomSupport given that there is only one implementation (the one with an if ... instanceof). Also the static final proxy should be spelled PROXY. - PR: https://git.openjdk.java.net/jdk/pull/3469
Re: RFR: 8263087: Add a MethodHandle combinator that switches over a set of MethodHandles
On Fri, 9 Apr 2021 19:57:10 GMT, Jorn Vernee wrote: >>> yes, for all the switches, pattern-switch, enum-switch but not for the >>> string switch which requires a lookup switch. >> Can you outline how to use the tableswitch combinator in the case of a >> switch on strings ? >> >> Jan Lahoda has made several good examples of that: >> https://github.com/lahodaj/jdk/blob/switch-bootstrap/src/java.base/share/classes/java/lang/runtime/SwitchBootstraps.java >> i.e. several filtering strategies for translating a String into a table >> index (which can then be fed to `tableswitch`) >> >> I ran some benchmarks: >> >>  >> >> Here, 'legacy' is what C2 does with `lookupswitch`. >> >> Maybe it's worth it to include such an example & benchmark in this patch as >> well (show off how to emulate `lookupswitch`) > > I've uploaded a benchmark that simulates a lookup switch using the > tableSwitch combinator as well, using a HashMap lookup as a filter: > https://github.com/openjdk/jdk/commit/a7157eb0ef1b7190aabfb2689539801c6692bbae > > For that particular key set (the same as from the graph above), HashMap is > faster: > > Benchmark Mode Cnt Score > Error Units > MethodHandlesEmulateLookupSwitch.emulatedLookupSwitch avgt 30 19.450 � > 0.079 ms/op > MethodHandlesEmulateLookupSwitch.nativeLookupSwitchavgt 30 25.370 � > 0.159 ms/op > > But, I've found it really depends on the key set. However, this is mostly to > demonstrate that emulation can have competitive performance with native > `lookupswitch`. e.g. to get constant folding for constant inputs another > filter has to be used, since C2 can not see through the HashMap lookups. Ok, let restart from the beginning, you have two strategy to de-sugar a switch, - if what you do after the case do not mutate any variables, you can desugar each case to a method more or less like a lambda (it's not exactly like a lambda because there is no capture) and you have an indy in front that will call the right method handles - you have a front end, with an indy that associate the object to an int and a backend which is tableswitch in the bytecode The first strategy is an optimization, it will get you good performance by example if you compare a switch on a hirerachy on types and the equivalent method call. But you can not use that strategy for all switch, it's more an optimization. The second strategy let you encode any switches. The tests above are using the first strategy, which I think is not what we should implement by default given that it doesn't work will all cases. In the particular case of a switch on string, javac generates two switches, the front one and the back one, if we want to compare, we should implement the second strategy, so indy or the equivalent constant handle should take a String as parameter and return an int. On the test themselves, for the hash, the Map should be directly bound, it will be more efficient, the asm version doesn't not appear in the graphics and there is a missing strategy that is using a MutableCallSite to switch from the a cascade of guards using the real values (store the String value even if it goes to `default`) and then switch to a lookup switch which i've found is the optimal strategy in real code (it works a lot like a bi-morphic cache but on string values instead of on classes). - PR: https://git.openjdk.java.net/jdk/pull/3401
Re: RFR: 8263668: Update java.time to use instanceof pattern variable
On Wed, 24 Mar 2021 09:56:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.time`
> package to make use of the `instanceof` pattern variable?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
168:
> 166: private static final TemporalQuery QUERY_REGION_ONLY =
> (temporal) -> {
> 167: ZoneId zone = temporal.query(TemporalQueries.zoneId());
> 168: return (zone != null && (!(zone instanceof ZoneOffset)) ? zone :
> null);
i find this code hard to read
`return (zone != null && (!(zone instanceof ZoneOffset))) ? zone : null;`
seems better`
-
PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8263668: Update java.time to use instanceof pattern variable
On Wed, 24 Mar 2021 09:56:16 GMT, Patrick Concannon
wrote:
> Hi,
>
> Could someone please review my code for updating the code in the `java.time`
> package to make use of the `instanceof` pattern variable?
>
> Kind regards,
> Patrick
src/java.base/share/classes/java/time/LocalDateTime.java line 1686:
> 1684: public long until(Temporal endExclusive, TemporalUnit unit) {
> 1685: LocalDateTime end = LocalDateTime.from(endExclusive);
> 1686: if (unit instanceof ChronoUnit u) {
`chronoUnit` is perhaps a better variable name than `u`
src/java.base/share/classes/java/time/LocalTime.java line 1412:
> 1410: if (unit instanceof ChronoUnit u) {
> 1411: long nanosUntil = end.toNanoOfDay() - toNanoOfDay(); // no
> overflow
> 1412: switch (u) {
same comment as above
src/java.base/share/classes/java/time/MonthDay.java line 448:
> 446: public long getLong(TemporalField field) {
> 447: if (field instanceof ChronoField f) {
> 448: switch (f) {
as above, `chronoField` instead of `f`
src/java.base/share/classes/java/time/OffsetDateTime.java line 599:
> 597: @Override
> 598: public int get(TemporalField field) {
> 599: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/OffsetDateTime.java line 636:
> 634: @Override
> 635: public long getLong(TemporalField field) {
> 636: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/OffsetTime.java line 1182:
> 1180: OffsetTime end = OffsetTime.from(endExclusive);
> 1181: if (unit instanceof ChronoUnit u) {
> 1182: long nanosUntil = end.toEpochNano() - toEpochNano(); // no
> overflow
see above
src/java.base/share/classes/java/time/Year.java line 500:
> 498: public long getLong(TemporalField field) {
> 499: if (field instanceof ChronoField f) {
> 500: switch (f) {
see above
src/java.base/share/classes/java/time/Year.java line 711:
> 709: @Override
> 710: public Year plus(long amountToAdd, TemporalUnit unit) {
> 711: if (unit instanceof ChronoUnit u) {
see above
src/java.base/share/classes/java/time/Year.java line 917:
> 915: public long until(Temporal endExclusive, TemporalUnit unit) {
> 916: Year end = Year.from(endExclusive);
> 917: if (unit instanceof ChronoUnit u) {
see above
src/java.base/share/classes/java/time/YearMonth.java line 488:
> 486: @Override
> 487: public long getLong(TemporalField field) {
> 488: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/YearMonth.java line 808:
> 806: @Override
> 807: public YearMonth plus(long amountToAdd, TemporalUnit unit) {
> 808: if (unit instanceof ChronoUnit u) {
see above
src/java.base/share/classes/java/time/YearMonth.java line 1049:
> 1047: public long until(Temporal endExclusive, TemporalUnit unit) {
> 1048: YearMonth end = YearMonth.from(endExclusive);
> 1049: if (unit instanceof ChronoUnit u) {
see above
src/java.base/share/classes/java/time/ZonedDateTime.java line 816:
> 814: @Override // override for Javadoc and performance
> 815: public int get(TemporalField field) {
> 816: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/ZonedDateTime.java line 853:
> 851: @Override
> 852: public long getLong(TemporalField field) {
> 853: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/chrono/ChronoLocalDateImpl.java line 380:
> 378: Objects.requireNonNull(endExclusive, "endExclusive");
> 379: ChronoLocalDate end = getChronology().date(endExclusive);
> 380: if (unit instanceof ChronoUnit u) {
see above
src/java.base/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java line
379:
> 377: if (unit.isTimeBased()) {
> 378: long amount = end.getLong(EPOCH_DAY) -
> date.getLong(EPOCH_DAY);
> 379: switch (u) {
see above
src/java.base/share/classes/java/time/chrono/ChronoZonedDateTime.java line 198:
> 196: @Override
> 197: default int get(TemporalField field) {
> 198: if (field instanceof ChronoField f) {
see above
src/java.base/share/classes/java/time/chrono/ChronoZonedDateTime.java line 212:
> 210: @Override
> 211: default long getLong(TemporalField field) {
> 212: if (field instanceof ChronoField f) {
see above
-
PR: https://git.openjdk.java.net/jdk/pull/3170
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:39:20 GMT, Jan Lahoda wrote: >> We have considered generics, that why parameterized generics with another >> type arguments are forbidden, but i think we have forgotten raw types. > > I don't think that cast from `Object` to a raw type is unchecked, so as error > does not seem warranted to me. > > However, I agree javac should produce the rawtype warning for rawtypes in > pattern matching instanceof, because we are introducing a new variable (and > casting). I've filled: > https://bugs.openjdk.java.net/browse/JDK-8263590 > > Note the non-pattern matching instanceof does not produce the rawtype > warning, and I don't think it should produce it. yes, javac should emit a warning in that case, that also the answer of Brian. https://mail.openjdk.java.net/pipermail/amber-spec-experts/2021-March/002925.html - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 11:01:48 GMT, Pavel Rappo wrote: >> The problem is that `desc` is a raw type and raw types doesn't play well >> with the rest of the language (in particular with inference). >> >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. >> >> The spec says that the narrow conversion should not be unchecked >> https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2 >> it depends if you consider the raw conversion as unchecked or not. > >> I think it's a bug in javac, it should not even raise a warning but an error. >> But the spec is not fully clear. > > If you think that it's a bug, consider providing feedback to authors of JEP > 394: > >> Other refinements may be incorporated based on further feedback. > > I find it hard to believe that authors didn't consider generic use case (even > though JEP 394 doesn't have examples of that). We have considered generics, that why parameterized generics with another type arguments are forbidden, but i think we have forgotten raw types. - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 10:18:26 GMT, Pavel Rappo wrote:
>> src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
>> 360:
>>
>>> 358: public final boolean equals(Object o) {
>>> 359: if (this == o) return true;
>>> 360: return (o instanceof DynamicConstantDesc desc)
>>
>> should be
>> `(o instanceof DynamicConstantDesc desc)`
>
> I noticed that too initially. However, `javac` does not seem to produce a
> "rawtypes" warning. Which makes sense, if you think about it.
The problem is that `desc` is a raw type and raw types doesn't play well with
the rest of the language (in particular with inference).
I think it's a bug in javac, it should not even raise a warning but an error.
But the spec is not fully clear.
The spec says that the narrow conversion should not be unchecked
https://docs.oracle.com/javase/specs/jls/se15/preview/specs/patterns-instanceof-jls.html#jls-14.30.2
it depends if you consider the raw conversion as unchecked or not.
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon wrote: >> Hi, >> >> Could someone please review my code for updating the code in the `java.lang` >> package to make use of the `instanceof` pattern variable? >> >> Kind regards, >> Patrick > > Patrick Concannon has updated the pull request incrementally with one > additional commit since the last revision: > > 8263358: Refactored missed equals method src/java.base/share/classes/java/lang/StackTraceElement.java line 413: > 411: && Objects.equals(moduleName, e.moduleName) > 412: && Objects.equals(moduleVersion, e.moduleVersion) > 413: && e.declaringClass.equals(declaringClass) testing the declaring class and the line number should be done first given they are primitive values, it will be a little more efficient if two StackTraceElement are not equals and one is using non interned String. return (obj instanceof StackTraceElement e) && e.lineNumber == lineNumber && e.declaringClass == declaringClass && ... - PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8263358: Update java.lang to use instanceof pattern variable [v5]
On Mon, 15 Mar 2021 09:21:22 GMT, Patrick Concannon
wrote:
>> Hi,
>>
>> Could someone please review my code for updating the code in the `java.lang`
>> package to make use of the `instanceof` pattern variable?
>>
>> Kind regards,
>> Patrick
>
> Patrick Concannon has updated the pull request incrementally with one
> additional commit since the last revision:
>
> 8263358: Refactored missed equals method
src/java.base/share/classes/java/lang/constant/DynamicConstantDesc.java line
360:
> 358: public final boolean equals(Object o) {
> 359: if (this == o) return true;
> 360: return (o instanceof DynamicConstantDesc desc)
should be
`(o instanceof DynamicConstantDesc desc)`
-
PR: https://git.openjdk.java.net/jdk/pull/2913
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 18 Nov 2020 13:45:46 GMT, Jim Laskey wrote: >> Need rebase > > Created new PR because of forced push: > https://github.com/openjdk/jdk/pull/1292 LGTM - PR: https://git.openjdk.java.net/jdk/pull/1273
Re: RFR: 8263102: Expand documention of Method.isBridge [v3]
On Tue, 9 Mar 2021 19:13:24 GMT, Joe Darcy wrote:
>> src/java.base/share/classes/java/lang/reflect/Method.java line 589:
>>
>>> 587: * different return type, the virtual machine does not.
>>> 588: * A
>>> 589: * common case where covariant overrides are used is for a {@link
>>
>> I think the example should be clearer because here, you don't show the code
>> of EnumSet.clone().
>> I wonder if it's not easier if all the code is visible
>> interface I {
>> Object m();
>> }
>> class A implements I {
>> String m() { return "hello"; }
>> }
>> so you can explain that the VM do the dispatch on I::m()Object so the
>> compiler has to insert a method A::m()Object,
>> the bridge method with this pseudo-code
>> class A implements I {
>> /* bridge */ Object m() { return m(); } // calls m()String
>> String m() { return "hello"; }
>> }
>
> Hi Remi,
>
> Thanks for the feedback. I did consider that kind of approach of an example
> from scratch. I judged referencing instances of bridge methods in the base
> module to be helpful to demonstrate it wasn't too esoteric of a feature in
> terms of it is something you, as a developer, may already have seen. Also,
> given the likely audience of the reading Class.isBridge, I didn't think a
> stand-alone example was warranted.
@jddarcy, the problem i see is that `EnumSet` is not used a lot and `clone` has
another layer of complexity because of CloneNotSupportedException, etc.
So i'm not sure that using EnumSet.clone() is the right example here.
Perhaps String.compareTo()/Comparable.compareTo() is a better example ? But
generics are in the middle too.
-
PR: https://git.openjdk.java.net/jdk/pull/2852
Re: RFR: 8263102: Expand documention of Method.isBridge [v3]
On Tue, 9 Mar 2021 03:27:29 GMT, Joe Darcy wrote:
>> The existing documentation of Method.isBridge isn't terribly helpful to the
>> reader. This RFE proposes to given a common example of how bridge methods
>> are used. The JLS does *not* have a section discussing bridge methods in
>> detail; bridge methods are a compilation technique for lowering the Java
>> language to the JVM, they are not a language feature per se. The example
>> given is not exhaustive; there can be and are other uses of bridge methods.
>>
>> Once the text is agreed to; I'll update the copyright year as well.
>
> Joe Darcy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Respond to review feedback.
src/java.base/share/classes/java/lang/reflect/Method.java line 589:
> 587: * different return type, the virtual machine does not.
> 588: * A
> 589: * common case where covariant overrides are used is for a {@link
I think the example should be clearer because here, you don't show the code of
EnumSet.clone().
I wonder if it's not easier if all the code is visible
interface I {
Object m();
}
class A implements I {
String m() { return "hello"; }
}
so you can explain that the VM do the dispatch on I::m()Object so the compiler
has to insert a method A::m()Object,
the bridge method with this pseudo-code
class A implements I {
/* bridge */ Object m() { return m(); } // calls m()String
String m() { return "hello"; }
}
-
PR: https://git.openjdk.java.net/jdk/pull/2852
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators
On Fri, 26 Feb 2021 13:13:19 GMT, Jim Laskey wrote: >> Stayin' alive > > Looking for some final code reviews. I still don't like the fact that the factory uses reflection but i don't see how to do better - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8261880: Change nested classes in java.base to static nested classes where possible [v2]
On Wed, 17 Feb 2021 17:24:50 GMT, Claes Redestad wrote: >> For static methods, since in java language you cannot declare static method >> in instance inner classes, I'd say making them static makes more sense >> language-wise. Also making them static reduces compiler synthetic instance >> field and constructors. > > Incidentally, Java-the-language allows static methods in inner instance > classes since JDK 16. And I'm not sure this was ever a restriction at the > JVMS level since we've been generating static methods (using ASM) into these > inner instance classes since at least JDK 9. Inner classes doesn't really exist for the JVM, it's just an attribute (in fact, a pair of attributes) that is read/write by javac (it's very similar to the way generics work). So it is Ok to have static methods in inner classes since Java 1.1 from the JVM POV with the caveat that you may not be all to call them from Java-the-language. Also since Java 11, inner classes are also nestmate and those attributes (NestHost/NestMembers) change the behavior of the VM. - PR: https://git.openjdk.java.net/jdk/pull/2589
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v17]
On Tue, 16 Feb 2021 14:03:56 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/jdk/internal/util/random/RandomSupport.java line
>> 1548:
>>
>>> 1546: * @return a stream of (pseudo)randomly chosen {@code int}
>>> values
>>> 1547: */
>>> 1548:
>>
>> Is `@Override` intentionally omitted?
>
> The interface method is a default method, so not technically an override.
It's not an override but @Override has a broader semantics than just overriding
an existing method.
Given that it helps to understand that this method is part of a larger
algorithm, i think this method should be tagged with @Override
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll [v2]
On Thu, 17 Dec 2020 11:03:45 GMT, Сергей Цыпанов wrote: >> ok, it means there is a bug in the compiler, the analysis done for unsafe >> varargs (with -Xlint:varargs) doesn't check that the called method (here >> Arrays.asList()) is tagged with @SafeVarargs. > > Should I then wait for the fix of that bug to remove `@SupressWarnings`? I don't think so, the code is fine, for me. - PR: https://git.openjdk.java.net/jdk/pull/1764
Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll [v2]
On Thu, 17 Dec 2020 10:45:17 GMT, Сергей Цыпанов
wrote:
>> src/java.base/share/classes/java/util/Collections.java line 5589:
>>
>>> 5587: */
>>> 5588: @SafeVarargs
>>> 5589: @SuppressWarnings("varargs")
>>
>> I don't think you need a SuppressWarnings here
>
> Hi, without this I get failed build:
> Compiling 3057 files for java.base
> Compiling 89 properties into resource bundles for java.desktop
>
> return c.addAll(Arrays.asList(elements));
> ^
> error: warnings found and -Werror specified
> 1 error
> 1 warning
> make[3]: ***
> [/home/s.tsypanov/IdeaProjects/jdk-github/build/linux-x86_64-server-release/jdk/modules/java.base/_the.java.base_batch]
> Error 1
> CompileJavaModules.gmk:604: recipe for target
> '/home/s.tsypanov/IdeaProjects/jdk-github/build/linux-x86_64-server-release/jdk/modules/java.base/_the.java.base_batch'
> failed
> make[2]: *** [java.base-java] Error 2
> make[2]: *** Waiting for unfinished jobs
> make/Main.gmk:193: recipe for target 'java.base-java' failed
ok, it means there is a bug in the compiler, the analysis done for unsafe
varargs (with -Xlint:varargs) doesn't check that the called method (here
Arrays.asList()) is tagged with @SafeVarargs.
-
PR: https://git.openjdk.java.net/jdk/pull/1764
Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll
On Thu, 17 Dec 2020 10:15:23 GMT, Сергей Цыпанов
wrote:
>> Hello, I feel like this was previously discussed in
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/ but since I cannot
>> find original mail I post this here.
>>
>> Currently `Collections.addAll()` is implemented and documented as:
>> /**
>> * ...
>> * The behavior of this convenience method is identical to that of
>> * {@code c.addAll(Arrays.asList(elements))}, but this method is likely
>> * to run significantly faster under most implementations.
>> */
>> @SafeVarargs
>> public static boolean addAll(Collection c, T... elements)
>> {
>> boolean result = false;
>> for (T element : elements)
>> result |= c.add(element);
>> return result;
>> }
>>
>> But it practice the notation `this method is likely to run significantly
>> faster under most implementations` is completely wrong. When I take this
>> [benchmark](https://github.com/stsypanov/benchmarks/blob/master/benchmark-runners/src/main/java/com/luxoft/logeek/benchmark/collection/CollectionsAddAllVsAddAllBenchmark.java)
>> and run it on JDK 14 I get the following results:
>>(collection) (size)
>> Score Error Units
>> addAllArrayList 10
>> 37.9 ± 1.9 ns/op
>> addAllArrayList 100
>> 83.8 ± 3.4 ns/op
>> addAllArrayList1000
>> 678.2 ±23.0 ns/op
>> collectionsAddAll ArrayList 10
>> 50.9 ± 1.1 ns/op
>> collectionsAddAll ArrayList 100
>> 751.4 ±47.4 ns/op
>> collectionsAddAll ArrayList1000
>> 8839.8 ± 710.7 ns/op
>>
>> addAll HashSet 10
>> 128.4 ± 5.9 ns/op
>> addAll HashSet 100
>> 1864.2 ± 102.4 ns/op
>> addAll HashSet1000
>> 16615.5 ± 1202.6 ns/op
>> collectionsAddAll HashSet 10
>> 172.8 ± 6.0 ns/op
>> collectionsAddAll HashSet 100
>> 2355.8 ± 195.4 ns/op
>> collectionsAddAll HashSet1000
>> 20364.7 ± 1164.0 ns/op
>>
>> addAll ArrayDeque 10
>> 54.0 ± 0.4 ns/op
>> addAll ArrayDeque 100
>> 319.7 ± 2.5 ns/op
>> addAll ArrayDeque1000
>> 3176.9 ±22.2 ns/op
>> collectionsAddAllArrayDeque 10
>> 66.5 ± 1.4 ns/op
>> collectionsAddAllArrayDeque 100
>> 808.1 ±55.9 ns/op
>> collectionsAddAllArrayDeque1000
>> 5639.6 ± 240.9 ns/op
>>
>> addAll CopyOnWriteArrayList 10
>> 18.0 ± 0.7 ns/op
>> addAll CopyOnWriteArrayList 100
>> 39.4 ± 1.7 ns/op
>> addAll CopyOnWriteArrayList1000
>> 371.1 ±17.0 ns/op
>> collectionsAddAll CopyOnWriteArrayList 10
>> 251.9 ±18.4 ns/op
>> collectionsAddAll CopyOnWriteArrayList 100
>> 3405.9 ± 304.8 ns/op
>> collectionsAddAll CopyOnWriteArrayList1000
>> 247496.8 ± 23502.3 ns/op
>>
>> addAllConcurrentLinkedDeque 10
>> 81.4 ± 2.8 ns/op
>> addAllConcurrentLinkedDeque 100
>> 609.1 ±26.4 ns/op
>> addAllConcurrentLinkedDeque1000
>> 4494.5 ± 219.3 ns/op
>> collectionsAddAll ConcurrentLinkedDeque 10
>> 189.8 ± 2.5 ns/op
>> collectionsAddAll ConcurrentLinkedDeque 100
>> 1660.0 ±62.0 ns/op
>> collectionsAddAll ConcurrentLinkedDeque1000
>> 17649.2 ± 300.9 ns/op
>>
>> addAll:·gc.alloc.rate.normArrayList 10
>> 160.0 ± 0.0B/op
>> addAll:·gc.alloc.rate.normArrayList 100
>> 880.0 ± 0.0B/op
>> addAll:·gc.alloc.rate.normArrayList1000
>> 8080.3 ± 0.0B/op
>> collectionsAddAll:·gc.alloc.rate.norm ArrayList 10
>> 80.0 ± 0.0B/op
>>
Re: RFR: 8193031: Collections.addAll is likely to perform worse than Collection.addAll [v2]
On Thu, 17 Dec 2020 10:33:05 GMT, Сергей Цыпанов
wrote:
>> Hello, I feel like this was previously discussed in
>> https://mail.openjdk.java.net/pipermail/core-libs-dev/ but since I cannot
>> find original mail I post this here.
>>
>> Currently `Collections.addAll()` is implemented and documented as:
>> /**
>> * ...
>> * The behavior of this convenience method is identical to that of
>> * {@code c.addAll(Arrays.asList(elements))}, but this method is likely
>> * to run significantly faster under most implementations.
>> */
>> @SafeVarargs
>> public static boolean addAll(Collection c, T... elements)
>> {
>> boolean result = false;
>> for (T element : elements)
>> result |= c.add(element);
>> return result;
>> }
>>
>> But it practice the notation `this method is likely to run significantly
>> faster under most implementations` is completely wrong. When I take this
>> [benchmark](https://github.com/stsypanov/benchmarks/blob/master/benchmark-runners/src/main/java/com/luxoft/logeek/benchmark/collection/CollectionsAddAllVsAddAllBenchmark.java)
>> and run it on JDK 14 I get the following results:
>>(collection) (size)
>> Score Error Units
>> addAllArrayList 10
>> 37.9 ± 1.9 ns/op
>> addAllArrayList 100
>> 83.8 ± 3.4 ns/op
>> addAllArrayList1000
>> 678.2 ±23.0 ns/op
>> collectionsAddAll ArrayList 10
>> 50.9 ± 1.1 ns/op
>> collectionsAddAll ArrayList 100
>> 751.4 ±47.4 ns/op
>> collectionsAddAll ArrayList1000
>> 8839.8 ± 710.7 ns/op
>>
>> addAll HashSet 10
>> 128.4 ± 5.9 ns/op
>> addAll HashSet 100
>> 1864.2 ± 102.4 ns/op
>> addAll HashSet1000
>> 16615.5 ± 1202.6 ns/op
>> collectionsAddAll HashSet 10
>> 172.8 ± 6.0 ns/op
>> collectionsAddAll HashSet 100
>> 2355.8 ± 195.4 ns/op
>> collectionsAddAll HashSet1000
>> 20364.7 ± 1164.0 ns/op
>>
>> addAll ArrayDeque 10
>> 54.0 ± 0.4 ns/op
>> addAll ArrayDeque 100
>> 319.7 ± 2.5 ns/op
>> addAll ArrayDeque1000
>> 3176.9 ±22.2 ns/op
>> collectionsAddAllArrayDeque 10
>> 66.5 ± 1.4 ns/op
>> collectionsAddAllArrayDeque 100
>> 808.1 ±55.9 ns/op
>> collectionsAddAllArrayDeque1000
>> 5639.6 ± 240.9 ns/op
>>
>> addAll CopyOnWriteArrayList 10
>> 18.0 ± 0.7 ns/op
>> addAll CopyOnWriteArrayList 100
>> 39.4 ± 1.7 ns/op
>> addAll CopyOnWriteArrayList1000
>> 371.1 ±17.0 ns/op
>> collectionsAddAll CopyOnWriteArrayList 10
>> 251.9 ±18.4 ns/op
>> collectionsAddAll CopyOnWriteArrayList 100
>> 3405.9 ± 304.8 ns/op
>> collectionsAddAll CopyOnWriteArrayList1000
>> 247496.8 ± 23502.3 ns/op
>>
>> addAllConcurrentLinkedDeque 10
>> 81.4 ± 2.8 ns/op
>> addAllConcurrentLinkedDeque 100
>> 609.1 ±26.4 ns/op
>> addAllConcurrentLinkedDeque1000
>> 4494.5 ± 219.3 ns/op
>> collectionsAddAll ConcurrentLinkedDeque 10
>> 189.8 ± 2.5 ns/op
>> collectionsAddAll ConcurrentLinkedDeque 100
>> 1660.0 ±62.0 ns/op
>> collectionsAddAll ConcurrentLinkedDeque1000
>> 17649.2 ± 300.9 ns/op
>>
>> addAll:·gc.alloc.rate.normArrayList 10
>> 160.0 ± 0.0B/op
>> addAll:·gc.alloc.rate.normArrayList 100
>> 880.0 ± 0.0B/op
>> addAll:·gc.alloc.rate.normArrayList1000
>> 8080.3 ± 0.0B/op
>> collectionsAddAll:·gc.alloc.rate.norm ArrayList 10
>> 80.0 ± 0.0B/op
>>
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 15:52:35 GMT, Daniel Fuchs wrote:
>> src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 1203:
>>
>>> 1201: @Override
>>> 1202: public String readUTF() throws IOException {
>>> 1203: return DataInputStream.readUTF(this);
>>
>> If i understand correctly the code, I believe readUTF should change a
>> boolean field named `countCanNotBeTrackedAnymore` from false to true, and in
>> the method `count()`, `countCanNotBeTrackedAnymore` has to be checked and
>> throws an ISE before returning `count`
>
> Hi Rémi, I do not think that that is required. `DataInputStream.readUTF` will
> call back into `this` to do the reading so the `count` should be properly
> incremented? Or maybe I'm missing something. Best regards!
Thanks,
i should have read the code more carefully.
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8255542: Attribute length of Module, ModulePackages and other attributes is ignored [v2]
On Thu, 3 Dec 2020 09:58:16 GMT, Alan Bateman wrote:
>> The attribute_length of known Module attributes in the module-info.class
>> is currently ignored. It should be checked and the class rejected if the
>> attribute length doesn't exactly match the length of the info in the
>> attribute.
>>
>> There are several ways to fix this. I initially limited the reading of the
>> attribute_info to the attribute length but this resulted in confusing
>> exception messages as the attribute appears truncated. The exception
>> messages are clearer when it checks that the attribute length corresponds to
>> the number of bytes read.
>
> Alan Bateman 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 11 additional
> commits since the last revision:
>
> - Restructure check to make it more obvious that it doesn't overflow
> - Merge
> - Merge
> - Merge
> - Trailing whitespace
> - Expand test to Module attribute
> - Merge
> - Test cleanup
> - Add test
> - Merge
> - ... and 1 more:
> https://git.openjdk.java.net/jdk/compare/914dd7e9...f15dbb1b
src/java.base/share/classes/jdk/internal/module/ModuleInfo.java line 1203:
> 1201: @Override
> 1202: public String readUTF() throws IOException {
> 1203: return DataInputStream.readUTF(this);
If i understand correctly the code, I believe readUTF should change a boolean
field named `countCanNotBeTrackedAnymore` from false to true, and in the method
`count()`, `countCanNotBeTrackedAnymore` has to be checked and throws an ISE
before returning `count`
-
PR: https://git.openjdk.java.net/jdk/pull/1407
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 19:48:32 GMT, Jim Laskey wrote: >> At least, it's more clear that it's reversed, i've initially miss the fact >> that f and g are swapped. >> And :: is able to do inference so, i believe it can be written >> >> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())` > > Unfortunately it couldn't be inferred It's because you have added reverse() as a postfix operation so the inference can not flow backward as it should, using Collections.reverseOrder() should fix that `.sorted(Collections.reverseOrder(Comparator.comparingInt(RandomGeneratorFactory::stateBits)))` using some import statics for reverseOrder and comparintInt make the code readable but given it's in the doc, we are out of luck here. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 16:22:34 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 151:
>>
>>> 149: if (fm == null) {
>>> 150: synchronized (RandomGeneratorFactory.class) {
>>> 151: if (RandomGeneratorFactory.factoryMap == null) {
>>
>> if `RandomGeneratorFactory.factoryMap` is not null, it returns null because
>> the value is not stored in fm.
>>
>> Please use the class holder idiom (cf Effective Java) instead of using the
>> double-check locking pattern.
>
> ? set in 148 and 152, but class holder idiom +1
If the second `RandomGeneratorFactory.factoryMap` return a non null value ...
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:59:01 GMT, Jim Laskey wrote:
>> src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java
>> line 88:
>>
>>> 86: * {@code
>>> 87: * RandomGeneratorFactory best =
>>> RandomGenerator.all()
>>> 88: * .sorted((f, g) -> Integer.compare(g.stateBits(),
>>> f.stateBits()))
>>
>> use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda
>
> Not sure that
> `.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`
> is simpler than `.sorted((f, g) -> Integer.compare(g.stateBits(),
> f.stateBits()))`.
At least, it's more clear that it's reversed, i've initially miss the fact that
f and g are swapped.
And :: is able to do inference so, i believe it can be written
`.sorted(Comparator.comparingInt(RandomGeneratorFactory::stateBits).reversed())`
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 15:43:39 GMT, Jim Laskey wrote: >> will investigate > > Needed to use ThreadLocalRandomProxy.proxy otherwise a cast would be required. yes, right ! - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote: >> This PR is to introduce a new random number API for the JDK. The primary API >> is found in RandomGenerator and RandomGeneratorFactory. Further description >> can be found in the JEP https://openjdk.java.net/jeps/356 . >> >> javadoc can be found at >> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html >> >> old PR: https://github.com/openjdk/jdk/pull/1273 > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > 8248862: Implement Enhanced Pseudo-Random Number Generators > > Changes to RandomGeneratorFactory requested by @PaulSandoz src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line 46: > 44: import java.util.stream.Stream; > 45: import jdk.internal.util.random.RandomSupport.RandomGeneratorProperty; > 46: Instead of calling a method properties to create a Map, it's usually far easier to use an annotation, annotation values supports less runtime type so BigInteger is not supported but using a String instead should be OK. - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
335:
> 333: ctorBytes = tmpCtorBytes;
> 334: ctorLong = tmpCtorLong;
> 335: ctor = tmpCtor;
This one is a volatile write, so the idea is that ctorBytes and ctorLong does
not need to be volatile too, which i think is not true if there is a code
somewhere that uses ctorBytes or ctorLong without reading ctor.
This code is too smart for me, i'm pretty sure it is wrong too on non TSO cpu.
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
480:
> 478: } catch (Exception ex) {
> 479: // Should never happen.
> 480: throw new IllegalStateException("Random algorithm " + name()
> + " is missing a default constructor");
chain the exception ...
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
497:
> 495: ensureConstructors();
> 496: return ctorLong.newInstance(seed);
> 497: } catch (Exception ex) {
this one is very dubious because the result in an exception is thrown is a
random generator with the wrong seed
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
517:
> 515: ensureConstructors();
> 516: return ctorBytes.newInstance((Object)seed);
> 517: } catch (Exception ex) {
same as above
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
235:
> 233: throws IllegalArgumentException {
> 234: Map> fm =
> getFactoryMap();
> 235: Provider provider =
> fm.get(name.toUpperCase());
again use of toUpperCase() instead of toUpperCase(Locale)
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
250:
> 248: * @return Stream of matching Providers.
> 249: */
> 250: static Stream>
> all(Class category) {
this signature is weird, T is not used in the parameter, so in case return any
type of Stream> from a type POV, should it be
` Stream> all(Class category)` instead ?
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
269:
> 267: * @throws IllegalArgumentException when either the name or category
> is null
> 268: */
> 269: static T of(String name, Class
> category)
Same issue as above, T is not used as a constraint
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
288:
> 286: * @throws IllegalArgumentException when either the name or category
> is null
> 287: */
> 288: static RandomGeneratorFactory
> factoryOf(String name, Class category)
Same as above
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
300:
> 298: */
> 299: @SuppressWarnings("unchecked")
> 300: private void getConstructors(Class
> randomGeneratorClass) {
method name get but do side effects
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
151:
> 149: if (fm == null) {
> 150: synchronized (RandomGeneratorFactory.class) {
> 151: if (RandomGeneratorFactory.factoryMap == null) {
if `RandomGeneratorFactory.factoryMap` is not null, it returns null because the
value is not stored in fm.
Please use the class holder idiom (cf Effective Java) instead of using the
double-check locking pattern.
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
157:
> 155: .stream()
> 156: .filter(p -> !p.type().isInterface())
> 157: .collect(Collectors.toMap(p ->
> p.type().getSimpleName().toUpperCase(),
toUpperCase() depends on the Locale !
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
175:
> 173: if (props == null) {
> 174: synchronized (provider) {
> 175: if (properties == null) {
same issue with the double check locking
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
177:
> 175: if (properties == null) {
> 176: try {
> 177: Method getProperties =
> provider.type().getDeclaredMethod("getProperties");
Is it not a better way than using reflection here ?
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
180:
> 178:
> PrivilegedExceptionAction> getAction =
> () -> {
> 179: getProperties.setAccessible(true);
> 180: return (Map Object>)getProperties.invoke(null);
Given that we have no control over the fact that the method properties()
doesn't return a Map of something else, this unsafe cast seems dangerous (from
the type system POV).
Having a type representing a reified version of the Map may be a better idea
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
88:
> 86: * {@code
> 87: * RandomGeneratorFactory best =
> RandomGenerator.all()
> 88: * .sorted((f, g) -> Integer.compare(g.stateBits(),
> f.stateBits()))
use Comparator.comparingInt(RandomGenerator::stateBits) instead of the lambda
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
116:
> 114: * Map of properties for provider.
> 115: */
> 116: private volatile Map properties =
> null;
`= null` is useless here
src/java.base/share/classes/java/util/random/RandomGeneratorFactory.java line
106:
> 104: * Map of provider classes.
> 105: */
> 106: private static volatile Map RandomGenerator>> factoryMap;
should be FACTORY_MAP given that it's a static field
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote: >> This PR is to introduce a new random number API for the JDK. The primary API >> is found in RandomGenerator and RandomGeneratorFactory. Further description >> can be found in the JEP https://openjdk.java.net/jeps/356 . >> >> javadoc can be found at >> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html >> >> old PR: https://github.com/openjdk/jdk/pull/1273 > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > 8248862: Implement Enhanced Pseudo-Random Number Generators > > Changes to RandomGeneratorFactory requested by @PaulSandoz src/java.base/share/classes/java/util/random/RandomGenerator.java line 745: > 743: * if the period is unknown. > 744: */ > 745: BigInteger UNKNOWN_PERIOD = BigInteger.ZERO; move those 3 values into RandomGeneratorFactory ? - PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8248862: Implement Enhanced Pseudo-Random Number Generators [v3]
On Wed, 25 Nov 2020 13:09:03 GMT, Jim Laskey wrote:
>> This PR is to introduce a new random number API for the JDK. The primary API
>> is found in RandomGenerator and RandomGeneratorFactory. Further description
>> can be found in the JEP https://openjdk.java.net/jeps/356 .
>>
>> javadoc can be found at
>> http://cr.openjdk.java.net/~jlaskey/prng/doc/api/java.base/java/util/random/package-summary.html
>>
>> old PR: https://github.com/openjdk/jdk/pull/1273
>
> Jim Laskey has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8248862: Implement Enhanced Pseudo-Random Number Generators
>
> Changes to RandomGeneratorFactory requested by @PaulSandoz
src/java.base/share/classes/java/security/SecureRandom.java line 223:
> 221: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 222: );
> 223: }
Using Map.of() instead of Map.ofEntries() should simplify the code
src/java.base/share/classes/java/util/Random.java line 129:
> 127: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 128: );
> 129: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/SplittableRandom.java line 181:
> 179: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 180: );
> 181: }
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
169:
> 167: Map.entry(RandomGeneratorProperty.IS_STOCHASTIC, false),
> 168: Map.entry(RandomGeneratorProperty.IS_HARDWARE, false)
> 169: );
Using Map.of() should simplify the code
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
433:
> 431: private static final class ThreadLocalRandomProxy extends Random {
> 432: @java.io.Serial
> 433: static final long serialVersionUID = 0L;
should be private
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
444:
> 442: }
> 443:
> 444: private static final AbstractSpliteratorGenerator proxy = new
> ThreadLocalRandomProxy();
should be declared inside ThreadLocalRandomProxy, so the value is only
initialized when proxy() is called
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
453:
> 451: * @return a {@code RandomGenerator} object that uses {@code
> ThreadLocalRandom}
> 452: */
> 453: public static RandomGenerator proxy() {
proxy is a generic name, is there a better name here ?
src/java.base/share/classes/java/util/concurrent/ThreadLocalRandom.java line
459:
> 457: // Methods required by class AbstractSpliteratorGenerator
> 458: public Spliterator.OfInt makeIntsSpliterator(long index, long fence,
> int origin, int bound) {
> 459: return new RandomIntsSpliterator(proxy, index, fence, origin,
> bound);
should use proxy()
-
PR: https://git.openjdk.java.net/jdk/pull/1292
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Wed, 4 Nov 2020 09:21:12 GMT, Tagir F. Valeev wrote:
>> src/java.base/share/classes/java/util/stream/Stream.java line 1192:
>>
>>> 1190: @SuppressWarnings("unchecked")
>>> 1191: default List toList() {
>>> 1192: return (List) Collections.unmodifiableList(new
>>> ArrayList<>(Arrays.asList(this.toArray(;
>>
>> Why can't we return `listFromTrustedArrayNullsAllowed` here as in
>> `ReferencePipeline`?
>> Or at least, we should avoid unnecessary copying of arrays. See [how this is
>> done](https://github.com/amaembo/streamex/blob/master/src/main/java/one/util/streamex/AbstractStreamEx.java#L1313)
>> in StreamEx.
>
> `listFromTrustedArrayNullsAllowed` is clearly not an option, as it will
> produce shared-secret leaking (see
> [JDK-8254090](https://bugs.openjdk.java.net/browse/JDK-8254090) for a similar
> case). StreamEx solution is dirty as it relies on the implementation detail.
> I believe, OpenJDK team is not very interested in providing optimal
> implementations for third-party stream implementations, as third-party
> implementations will likely update by themselves when necessary. At least, my
> suggestion to make the default `mapMulti` implementation better was declined.
This implementation duplicates the array twice, once in this.toArray() and once
in the constructor of ArrayList that calls toArray() on Collections.ArrayList.
I'm sure there is a better way
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Wed, 18 Nov 2020 22:20:26 GMT, Stuart Marks wrote: >> This change introduces a new terminal operation on Stream. This looks like a >> convenience method for Stream.collect(Collectors.toList()) or >> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this >> method directly on Stream enables it to do what can't easily by done by a >> Collector. In particular, it allows the stream to deposit results directly >> into a destination array (even in parallel) and have this array be wrapped >> in an unmodifiable List without copying. >> >> In the past we've kept most things from the Collections Framework as >> implementations of Collector, not directly on Stream, whereas only >> fundamental things (like toArray) appear directly on Stream. This is true of >> most Collections, but it does seem that List is special. It can be a thin >> wrapper around an array; it can handle generics better than arrays; and >> unlike an array, it can be made unmodifiable (shallowly immutable); and it >> can be value-based. See John Rose's comments in the bug report: >> >> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065 >> >> This operation is null-tolerant, which matches the rest of Streams. This >> isn't specified, though; a general statement about null handling in Streams >> is probably warranted at some point. >> >> Finally, this method is indeed quite convenient (if the caller can deal with >> what this operation returns), as collecting into a List is the most common >> stream terminal operation. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Adjust List.copyOf to null-check and copy allowNulls lists. > Fix equals, hashCode, indexOf, lastIndexOf to handle nulls properly. > Add MOAT tests for new lists; add equals and hashCode tests. src/java.base/share/classes/java/util/ImmutableCollections.java line 222: > 220: default: > 221: return (List) new ListN<>(input, false); > 222: } Using a switch expression + arrow (->) here will allow you too factor the cast, even if it disappear in the generated bytecode, I think it makes the code more readable - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v3]
On Thu, 5 Nov 2020 17:26:48 GMT, Stuart Marks wrote:
>> src/java.base/share/classes/java/util/ImmutableCollections.java line 199:
>>
>>> 197: * safely reused as the List's internal storage, avoiding a
>>> defensive copy. Declared
>>> 198: * with Object... instead of E... as the parameter type so that
>>> varargs calls don't
>>> 199: * accidentally create an array of type other than Object[].
>>
>> Why would that be a problem? If the resulting list is immutable, then the
>> actual array type doesn't really matter, right?
>
> It's an implementation invariant that the internal array be Object[]. Having
> it be something other than Object[] can lead to subtle bugs. See
> [JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for example.
you can still calls the varargs with an already created array
listFromTrustedArray(new String[] { "foo" });
I think at least an assert is missing
assert input.getClass() == Object.class;
-
PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8159746: (proxy) Support for default methods
On Fri, 20 Nov 2020 19:51:57 GMT, Mandy Chung wrote: >> @plevart I'm okay with this slight performance improvement. > > I have a fresh look at the different options we have explored (lots of > challenges in finding the right API with security, usability and performance > issues to consider). I agree with Remi that we should keep the design and > API simple and makes it easier to invoke default methods with today's Proxy > API.We can design a better Proxy API in the future. > > The options we have explored are: > 1. static `InvocationHandler::invokeDefault` method > 2. abstract `DelegatingInvocationHandler` class with a protected > `invokeDefault` method > 3. a new `newProxyInstance` factory method taking a function that produces an > invocation handler with the ability to invoke a default method via a > `superHandler` > > (1) is very simple API but caller-sensitive. No other API change. Access > check done at default method invocation time (which is consistent with the > core reflection `Method::invoke`).It shares the caller class caching in > `Method::checkAccess` which helps the performance. The performance > overhead is slightly higher than (2) & (3) which does access check at proxy > creation time. > > (2) is simple and I like that the `invokeDefault` can be enforced to be > invoked only by the proxy's invocation handler. However this requires more > API changes (including `newProxyInstance`, `getInvocationHandler`, and new > unchecked exception type). (3) is clever but a bit over-rotated (how Alan > describes it) to allow it to be expressed in a lambda expression. If an > invocation handler wants to save `superHandler`, it can't assign it to a > final field in lambda and would need to workaround it writing to an array > element. > > I will go with option (1) - static `invokeDefault` method [1] unless there > is any objection. > > Here is the specdiff: > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/ > > [1] > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/java.base/java/lang/reflect/InvocationHandler.html#invokeDefault(java.lang.Object,java.lang.reflect.Method,java.lang.Object...) Hi Mandy, thanks for taking the time to explore all the different options, The solution 1 is fine for me. - PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8186085: (opt) add filter(), flatMap(), and map() methods to OptionalDouble/Int/Long
On Sun, 1 Nov 2020 13:20:20 GMT, Kartik Ohri wrote: >> Hi ! >> thanks for taking the time to craft that pull request but this is typically >> the kind of patch that should be discussed on the mailing list first before >> creating a pull request. >> >> There are several issues, one is that we want to retrofit OptionalInt, >> OptionalLong, etc to be Optional, Optional as part of project >> Valhalla so we don't want to add more methods in the primitive variants of >> Optional and Stream to avoid to be blocked later while trying retrofitting >> those classes due to these methods. >> So we know that some methods are missing but we want to know first how the >> specialization of generics over primitive types works before adding those >> methods. >> >> The second issue is that when you want to change the public API of the JDK, >> you have to create a CSR to ask to the CSR guys/girls leaded by Joe Darcy if >> the signature and the semantics of the methods you want to add are coherent >> with the rest of the API. >> >> I recommend you to start by fixing bugs because those can be fixed by a >> simple pull request. >> >> regards, >> Rémi > > Thank you for the suggestions. I'll try fixing some bugs first. Should I > close this PR as well or leave it open ? If it doesn't break your hart too much, yes please. - PR: https://git.openjdk.java.net/jdk/pull/857
Re: RFR: 8186085: (opt) add filter(), flatMap(), and map() methods to OptionalDouble/Int/Long
On Sat, 31 Oct 2020 16:45:06 GMT, Kartik Ohri wrote: >> Hi all, >> This PR intends to add filter, map and flatMap methods to the Optional >> classes for primitives. The rationale is consistency with the Optional class >> for objects and user convenience. >> Thanks. >> Regards, >> Kartik > > Kindly review the patch. Thanks in advance! Hi ! thanks for taking the time to craft that pull request but this is typically the kind of patch that should be discussed on the mailing list first before creating a pull request. There are several issues, one is that we want to retrofit OptionalInt, OptionalLong, etc to be Optional, Optional as part of project Valhalla so we don't want to add more methods in the primitive variants of Optional and Stream to avoid to be blocked later while trying retrofitting those classes due to these methods. So we know that some methods are missing but we want to know first how the specialization of generics over primitive types works before adding those methods. The second issue is that when you want to change the public API of the JDK, you have to create a CSR to ask to the CSR guys/girls leaded by Joe Darcy if the signature and the semantics of the methods you want to add are coherent with the rest of the API. I recommend you to start by fixing bugs because those can be fixed by a simple pull request. regards, Rémi - PR: https://git.openjdk.java.net/jdk/pull/857
Re: RFR: 8159746: (proxy) Support for default methods
On Wed, 28 Oct 2020 19:28:36 GMT, Mandy Chung wrote: >> Hi Mandy and Peter, >> reading your exchanges, i wonder if it's not better to stop trying to add >> more and more features to the already clunky java.lang.reflect.Proxy but >> instead to move to provide the same set of feature but using an >> implementation based on java.lang.invoke.Lookup/MethodHandle. >> >> I propose to introduce a java.lang.invoke.Proxy has a replacement of >> java.lang.reflect.Proxy using j.l.i.MethodHandle instead of j.l.r.Method, >> the equivalent of the InvocationHandler acting as a linker called once per >> the first time a method of the Proxy is called instead of being called each >> time a method is called. >> >> Such proxy >> - has it's security model based on Lookup to define the class, >> so no supplementary restrictions on package/module of the interface >> implemented >> - avoid to mix java.lang.reflect API and java.invoke API >> - is transparent in term of exceptions (ther are propagated without any >> wrapping/unwrapping) >> - can support default methods natively >> - doesn't requires any caching (the user will be responsible to do the >> caching) >> - is more efficient, actually as efficient as the equivalent written code or >> a little more (thanks to @ForceInline) > > Hi Remi, > > I appreciate your proposal to modernize Proxy API. There are several > requests for this enhancement to support default methods in Proxy. Defining > a new `java.lang.invoke.Proxy` is a much bigger project that I can't tell > when the existing users of `java.lang.reflect.Proxy` will be able to get this > default method invocation support. > > I do agree that this API design has many challenges caused by what you listed > above. Well, I believe we are very close to have a consensus: > 1. New `newProxyInstance` factory method takes a handler factory doing the > access check > 2. Update `getInvocationHandler` to throw > `InaccessibleInvocationHandlerException` if access denied to get an > invocation handler associated with the proxy instance > > If this needs more time, I think I will consider to shelf this RFE and come > back to it later (and consider your proposal as well). The trick is that if we know that a class like java.lang.invoke.Proxy may exist, it means that instead of distorting the j.l.r.Proxy API to increase of few percents the performance when calling a default method, you can come with a simpler design in term of API that just add an API point to call a default method. Better performance becoming on the the goals of java.lang.invoke.Proxy. - PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8159746: (proxy) Support for default methods
On Wed, 28 Oct 2020 17:36:40 GMT, Mandy Chung wrote: >>> Hi Peter, >>> >>> > Question remains how do we distinguish proxies with old-fassioned >>> > InvocationHandlers from proxies with InvocationHandlers having access to >>> > super-default-handler. Both kinds of handlers look the same from Proxy's >>> > perspective... Perhaps we need a boolean flag in Proxy instance for that, >>> > coupled with an overloaded constructor that takes it... >>> >>> I have the Proxy constructor taking both the invocation handler and >>> `superHandler` in my >>> [mlchung:proxy-default-method-4](https://github.com/mlchung/jdk/tree/proxy-default-method-4) >>> branch. >>> >> >> Yes, but it is not really useful there. The super handler is captured by the >> user invocation handler created in the invocation handler factory function >> and it is only used by the user invocation handler. All we need in the proxy >> instance is a boolean flag indicating the way the invocation handler was >> created (which one of the two overloaded newProxyInstance methods was >> called). >> >>> > Problem with this approach is that it is not really useful in the proxy >>> > instance. We don't need it in the proxy instance. >>> >>> See above. >>> >>> About the exception thrown: we need to distinguish CCE and NPE thrown by >>> the default method vs thrown by arguments incompatible with the method >>> signature. In my proxy-default-method-4 branch, I define an internal >>> exception type for that. There is an overhead doing the wrapping but it's >>> an exception case which performance should not be critical. >> >> To be precise, we need to pass CCE and NPE thrown by the default method >> unchanged, but we need to wrap CCE and NPE thrown by the method handle >> transformation chain with IllegalArgumentException. Is that what you are >> referring to? In that case we need an internal exception type, not exposed >> to public API, used just to "tunnel" the exception from the default method >> unchanged. That's just implementation detail. >> >> But we have to ask ourselves a question: "Do we want to wrap CCE and NPE >> thrown by the method handle transformation chain with >> IllegalArgumentException?". We did that in previous variants to mimic the >> Method.invoke specification. But now we already departed from that >> specification by not wrapping exceptions thrown by the default method with >> InvocationTargetException. So we are not obligated to follow the rest of the >> Method.invoke specification either. All exception types thrown by method >> handle transformation chain (CCE, NPE, IAE) are unchecked exceptions and as >> such are not eligible to wrapping with UndeclaredThrowableException by the >> Proxy API. So it's just a matter of deciding what is more suitable: to have >> more specific exception types (CCE, NPE, IAE) for describing transformation >> exceptions or to reduce them to one common type (IAE). In either case those >> exception types can also be thrown by the default method and we can't or >> even want to distinguish their source. Let's be ho nest, all of CCE, NPE, IAE are usually describing the subtle variants of the same thing: that the parameters are wrong. Whether this happens in the logic of invocation handler, method handle transformation chain or the default method, we want those exceptions to propagate out of the invoked proxy method unchanged. >> >> So WDYT? > >> Yes, but it is not really useful there. The super handler is captured by the >> user invocation handler created in the invocation handler factory function >> and it is only used by the user invocation handler. All we need in the proxy >> instance is a boolean flag indicating the way the invocation handler was >> created (which one of the two overloaded newProxyInstance methods was >> called). > > Either way provides the distinction if this proxy class supports default > method invocation. I chose to store the superHandler rather than a boolean > that may allow us to do white-box testing (an alternative to the trick in > getting the superHandler in spinning another proxy instance). It's a > trivial change if we prefer to store a boolean field but keep the > constructor to take `superHandler` parameter. I'm fine with it. > >> To be precise, we need to pass CCE and NPE thrown by the default method >> unchanged, but we need to wrap CCE and NPE thrown by the method handle >> transformation chain with IllegalArgumentException. Is that what you are >> referring to? > > Yes and it's implementation details. For the specification side, I agree > that `InvocationHandler::invoke` throws the exception by the method and no > need to wrap it with `InvocationTargetException` . > >> "Do we want to wrap CCE and NPE thrown by the method handle transformation >> chain with IllegalArgumentException?". > > I did consider that and propose to go with IAE for simplicity and consistency > with the core reflection
Re: RFR: 8254354: Add an asExact() VarHandle combinator
On Fri, 23 Oct 2020 17:47:36 GMT, Jorn Vernee wrote: > Hi, > > This patch adds an asExact() combinator to VarHandle, that will return a new > VarHandle that performs exact type checks, similar to > MethodHandle::invokeExact, to help developers catch inexact VarHandle usage, > which can lead to performance degradation. > > This is implemented using a boolean flag in VarForm. If the flag is set, the > exact type of the invocation is checked against the exact type in the > VarForm. If there is a mismatch, a WrongMethodTypeException is thrown. > > Other than that, there is also an asGeneric() combinator added that does the > inverse operation (thanks to Rémi for the suggestion). I've also added The > `@Hidden` annotation to the VarHandleGuards methods, as well as a > type-checking helper method called from the generic invocation lambda form, > so that the stack trace we get points at the location where the VarHandle is > being used. > > Thanks, > Jorn > > CSR link: https://bugs.openjdk.java.net/browse/JDK-8255375 Looks good to me. - PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8254354: Add an asExact() VarHandle combinator [v2]
On Mon, 26 Oct 2020 16:34:10 GMT, Paul Sandoz wrote:
>> The direct use of the enum ordinal is because HotSpot accessing it from the
>> enum value is (or was) not optimal in C2.
>>
>> You can avoid the addition of the stable array by doing the following:
>>
>> public final MethodType accessModeType(AccessMode accessMode) {
>> return accessModeType(accessMode.at.ordinal());
>> }
>>
>> @ForceInline
>> final MethodType accessModeType(int accessModeOrdinal) {
>> TypesAndInvokers tis = getTypesAndInvokers();
>> MethodType mt = tis.methodType_table[accessModeOrdinal];
>> if (mt == null) {
>> mt = tis.methodType_table[accessModeOrdinal] =
>> accessModeTypeUncached(accessModeOrdinal);
>> }
>> return mt;
>> }
>>
>> final MethodType accessModeTypeUncached(int accessModeOrdinal) {
>> return
>> accessModeTypeUncached(AccessMode.values()[accessModeOrdinal]);
>> }
>> It's a little odd going back and forth between the ordinal and the enum
>> value, but it's all on the slow uncached path, and it avoids some
>> duplication of code.
>>
>> I'll take a closer looks at the other areas and respond in another comment.
>
> We can clarify the new methods and tie them closer to method handle
> semantics. I suggest the names `asExactInvoker` and `asInvoker`, referencing
> `MethodHandles.exactInvoker` and `MethodHandles.invoker` respectively. (The
> term "generic" is really reserved for erased method types, and otherwise used
> internally as the generic invoker.)
>
> The `VarHandle` documentation already specifies that:
>
> *
> * Invocation of an access mode method behaves as if an invocation of
> * {@link MethodHandle#invoke}, where the receiving method handle accepts the
> * VarHandle instance as the leading argument. More specifically, the
> * following, where {@code {access-mode}} corresponds to the access mode
> method
> * name:
> * {@code
> * VarHandle vh = ..
> * R r = (R) vh.{access-mode}(p1, p2, ..., pN);
> * }
> * behaves as if:
> * {@code
> * VarHandle vh = ..
> * VarHandle.AccessMode am =
> VarHandle.AccessMode.valueFromMethodName("{access-mode}");
> * MethodHandle mh = MethodHandles.varHandleExactInvoker(
> * am,
> * vh.accessModeType(am));
> *
> * R r = (R) mh.invoke(vh, p1, p2, ..., pN)
> * }
> * (modulo access mode methods do not declare throwing of {@code Throwable}).
> ...
>
> We will need to adjust this section, i can help, but first let us reach
> agreement on this approach.
Paul,
an invoker has the MethodHandle (resp. VarHandle) as first argument so it's not
the same semantics.
-
PR: https://git.openjdk.java.net/jdk/pull/843
Re: RFR: 8255398: Add a dropReturn MethodHandle combinator [v3]
On Mon, 26 Oct 2020 18:01:04 GMT, John R Rose wrote:
>> @rose00 My bad. You're right, I realized we could also use `asType` later on
>> (I did call it out in the CSR), but It's still a bit more wordy than what
>> I'm proposing.
>>
>> `dropReturn` seemed like a good choice since we already have
>> `dropArguments`. WRT changing to `MethodHandle::changeReturnType`, I think
>> doing that also begs for adding a `MethodHandle::changeParameterType` (with
>> a single index and varargs overload). But, in that case it seems fair to
>> just point users at `asType` instead.
>>
>> In other words; `dropReturn` seems fine to get parity with `dropArguments`,
>> but `changeReturnType` seems a step too close to `asType` (also since it
>> implies adding `changeParameterType` as well). So, maybe this RFE is a bust,
>> and users should use `asType` instead? Or do you think adding both
>> `changeReturnType` and `changeParameterType`(s) seems worth it? (I'm not so
>> sure).
>
>> …`dropReturn` seemed like a good choice since we already have
>> `dropArguments`. WRT changing to `MethodHandle::changeReturnType`...
>
> That's a very reasonable point. People might look for `dropRT` after they
> find `dropAs`. And `MHs` is designed as a large-ish set of utility methods.
>
> I agree that adding `MH::changeRT` is a slippery slope; it tends to lift more
> of the MT API onto MH, and it starts to put new stuff into the smaller MH
> API; that was my bad.
>
> But (in the spirit of brainstorming, and agreeing with your present
> proposal), I'll suggest a separate idea to think about. Use a HOFP API to
> give easy access to the entire `MT` API all in one go, by reifying the
> `MH.type` property as a temporary (lambda argument):
>
> MethodHandle mapType(MethodHandle mh, UnaryOperator fn) {
>return mh.asType(fn.apply(mh.type()));
> }
>
> This also suggests a possible argument transform utility, a sort of
> mini-version of Charlie Nutter's MH builder API:
>
> MethodHandle mapArguments(MethodHandle mh,
> UnaryOperator>> fn) {
> var args = mh.type().argumentList().stream().map(t -> makeArg(t)).asList();
> args = fn.apply(args);
> … do stuff to re-weave mh with the resulting argument transforms …
> }
> public sealed interface ArgumentValue {
> Class type();
>ArgumentValue asType(Class newType);
> … other stuff for pre-applying MHs …
> }
instead of mapArguments(), i believe it's better to have a method map on a
MethodType that takes two mapping functions, one for the return type and one
for the prameter types.
so one can write:
MHs.mapType(mh, mt -> mt.map(Function.identity(), t-> t.isPrimitive? t:
Object.class))
to not change the return type and erase the parameter type to Object if they
are not primitives
With that, changing the return type to void can be done that way too
MHs.mapType(mh, mt -> mt.map(__ -> void.class, Function.identity())
-
PR: https://git.openjdk.java.net/jdk/pull/866
Re: RFR: 8254354: Add an asExact() VarHandle combinator [v2]
On Fri, 23 Oct 2020 18:04:11 GMT, Jorn Vernee wrote:
>> Hi,
>>
>> This patch adds an asExact() combinator to VarHandle, that will return a new
>> VarHandle that performs exact type checks, similar to
>> MethodHandle::invokeExact, to help developers catch inexact VarHandle usage,
>> which can lead to performance degradation.
>>
>> This is implemented using a boolean flag in VarForm. If the flag is set, the
>> exact type of the invocation is checked against the exact type in the
>> VarForm. If there is a mismatch, a WrongMethodTypeException is thrown.
>>
>> Thanks,
>> Jorn
>
> Jorn Vernee has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Make internalName helper method static
src/java.base/share/classes/java/lang/invoke/MemoryAccessVarHandleGenerator.java
line 450:
> 448: }
> 449:
> 450: private String internalName(Class cls) {
should be static, no ?
-
PR: https://git.openjdk.java.net/jdk/pull/843
