Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On 1/12/2020 10:39 am, Mandy Chung wrote:
On Mon, 30 Nov 2020 20:57:32 GMT, Harold Seigel wrote:
src/java.base/share/classes/java/lang/Class.java line 4480:
4478: }
4479:
4480: private native Class[] getPermittedSubclasses0();
Does this JVM method return the permitted subclasses or subinterfaces with the
following conditions enforced by JLS:
- If a sealed class C belongs to a named module, then every class named in
the permits clause of the declaration of C must belong to the same module as C
- If a sealed class C belongs to an unnamed module, then every class named
in the permits clause of the declaration of C must belong to the same package
as C
I didn't check the VM implementation.
If the return array contains only classes as specified above,
`checkPackageAccessForClasses` can be simplified.
The JVM method that returns the permitted subclasses (and interfaces) does not
weed out permitted subclasses based on the above module requirements. It
returns all the classes listed in the PermittedSubclasses attribute that it is
able to load.
So it could also return a class listed in `PermittedSubclasses` attribute but
not a subclass of this sealed class, right?
The specification of `Class::getPermittedSubclasses` says:
Returns an array containing {@code Class} objects representing the direct
subclasses or direct implementation classes permitted to extend or direct
subinterfaces or subclasses permitted to extend or implement this class or
interface if it is sealed.
I expect that `Class::getPermittedSubclasses` should do more work and return
only the subclasses or subinterfaces permitted at runtime.
These subtleties are what I was also getting at in the CSR discussion.
If Foo lists Bar as a permitted subtype, but Bar does not extend Foo, is
that an error? Should Bar not be returned by getPermittedSubclasses? The
answer depends on exactly how you specify getPermittedSubclasses() and
whether that aligns with reading the classfile attribute, or actually
checking the runtime relationships.
David
-
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 15:59:07 GMT, Jan Lahoda wrote:
>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>>
>> From the original PR:
>>
>>> Please review the code for the second iteration of sealed classes. In this
>>> iteration we are:
>>>
>>> * Enhancing narrowing reference conversion to allow for stricter
>>> checking of cast conversions with respect to sealed type hierarchies
>>>
>>> * Also local classes are not considered when determining implicitly
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>>
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>>> still in the same method, the return type has been changed to Class[]
>>> instead of the previous ClassDesc[]
>>>
>>> * adding code to make sure that annotations can't be sealed
>>>
>>> * improving some tests
>>>
>>>
>>> TIA
>>>
>>> Related specs:
>>> [Sealed Classes
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>>
>> This PR strives to reflect the review comments from 1227:
>> * adjustments to javadoc of j.l.Class methods
>> * package access checks in Class.getPermittedSubclasses()
>> * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> 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:
>
> - Improving getPermittedSubclasses javadoc.
> - Merge branch 'master' into JDK-8246778
> - Moving checkPackageAccess from getPermittedSubclasses to a separate method.
> - Improving getPermittedSubclasses() javadoc.
> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both
> for sealed classes in named and unnamed modules.
> - Removing unnecessary file.
> - Tweaking javadoc.
> - Reflecting review comments w.r.t. narrowing conversion.
> - Improving checks in getPermittedSubclasses()
> - Merging master into JDK-8246778
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179
test/hotspot/jtreg/runtime/sealedClasses/GetPermittedSubclassesTest.java line
54:
> 52: public static void testSealedInfo(Class c, String[] expected) {
> 53: Object[] permitted = c.getPermittedSubclasses();
> 54:
Why `permitted` is not `Class[]`? or simply `var permitted = ...`
test/hotspot/jtreg/runtime/sealedClasses/GetPermittedSubclassesTest.java line
69:
> 67: for (int i = 0; i < permitted.length; i++) {
> 68: permittedNames.add(((Class)permitted[i]).getName());
> 69: }
This cast is not needed if `permitted` is `var` or `Class[]` type.
test/jdk/java/lang/reflect/sealed_classes/TestSecurityManagerChecks.java line 3:
> 1: /*
> 2: * Copyright (c) 2018, 2020, Oracle and/or its affiliates. All rights
> reserved.
> 3: * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
the copyright start year should be 2020.
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 20:57:32 GMT, Harold Seigel wrote:
>> src/java.base/share/classes/java/lang/Class.java line 4480:
>>
>>> 4478: }
>>> 4479:
>>> 4480: private native Class[] getPermittedSubclasses0();
>>
>> Does this JVM method return the permitted subclasses or subinterfaces with
>> the following conditions enforced by JLS:
>>
>>- If a sealed class C belongs to a named module, then every class named
>> in the permits clause of the declaration of C must belong to the same module
>> as C
>>- If a sealed class C belongs to an unnamed module, then every class
>> named in the permits clause of the declaration of C must belong to the same
>> package as C
>>
>> I didn't check the VM implementation.
>>
>> If the return array contains only classes as specified above,
>> `checkPackageAccessForClasses` can be simplified.
>
> The JVM method that returns the permitted subclasses (and interfaces) does
> not weed out permitted subclasses based on the above module requirements. It
> returns all the classes listed in the PermittedSubclasses attribute that it
> is able to load.
So it could also return a class listed in `PermittedSubclasses` attribute but
not a subclass of this sealed class, right?
The specification of `Class::getPermittedSubclasses` says:
> Returns an array containing {@code Class} objects representing the direct
> subclasses or direct implementation classes permitted to extend or direct
> subinterfaces or subclasses permitted to extend or implement this class or
> interface if it is sealed.
I expect that `Class::getPermittedSubclasses` should do more work and return
only the subclasses or subinterfaces permitted at runtime.
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8230501: Class data support for hidden classes [v7]
> Provide the `Lookup::defineHiddenClassWithClassData` API that allows live > objects > be shared between a hidden class and other classes. A hidden class can load > these live objects as dynamically-computed constants via this API. > > Specdiff > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8230501/specdiff/overview-summary.html > > With this class data support and hidden classes, > `sun.misc.Unsafe::defineAnonymousClass` > will be deprecated for removal. Existing libraries should replace their > calls to `sun.misc.Unsafe::defineAnonymousClass` with > `Lookup::defineHiddenClass` > or `Lookup::defineHiddenClassWithClassData`. > > This patch also updates the implementation of lambda meta factory and > `MemoryAccessVarHandleGenerator` to use class data. No performance > difference > observed in the jdk.incubator.foreign microbenchmarks. A side note: > `MemoryAccessVarHandleGenerator` is removed in the upcoming integration of > JDK-8254162 but it helps validating the class data support. > > Background > -- > > This is an enhancement following up JEP 371: Hidden Classes w.r.t. > "Constant-pool patching" in the "Risks and Assumption" section. > > A VM-anonymous class can be defined with its constant-pool entries already > resolved to concrete values. This allows critical constants to be shared > between a VM-anonymous class and the language runtime that defines it, and > between multiple VM-anonymous classes. For example, a language runtime will > often have `MethodHandle` objects in its address space that would be useful > to newly-defined VM-anonymous classes. Instead of the runtime serializing > the objects to constant-pool entries in VM-anonymous classes and then > generating bytecode in those classes to laboriously `ldc` the entries, > the runtime can simply supply `Unsafe::defineAnonymousClass` with references > to its live objects. The relevant constant-pool entries in the newly-defined > VM-anonymous class are pre-linked to those objects, improving performance > and reducing footprint. In addition, this allows VM-anonymous classes to > refer to each other: Constant-pool entries in a class file are based on names. > They thus cannot refer to nameless VM-anonymous classes. A language runtime > can, > however, easily track the live Class objects for its VM-anonymous classes and > supply them to `Unsafe::defineAnonymousClass`, thus pre-linking the new > class's > constant pool entries to other VM-anonymous classes. > > This extends the hidden classes to allow live objects to be injected > in a hidden class and loaded them via condy. > > Details > --- > > A new `Lookup::defineHiddenClassWithClassData` API takes additional > `classData` argument compared to `Lookup::defineHiddenClass`. > Class data can be method handles, lookup objects, arbitrary user objects > or collections of all of the above. > > This method behaves as if calling `Lookup::defineHiddenClass` to define > a hidden class with a private static unnamed field that is initialized > with `classData` at the first instruction of the class initializer. > > `MethodHandles::classData(Lookup lookup, String name, Class type)` and > `MethodHandles::classDataAt(Lookup lookup, String name, Class type, int > index)` > are the bootstrap methods to load the class data of the given lookup's lookup > class. > The hidden class will be initialized when `classData` method is called if > the hidden class has not been initialized. > > For a class data containing more than one single element, libraries can > create their convenience method to load a single live object via condy. > > Frameworks sometimes want to dynamically create a hidden class (HC) and add it > it the lookup class nest and have HC to carry secrets hidden from that nest. > In this case, frameworks should not to use private static finals (in the HCs > they spin) to hold secrets because a nestmate of HC may obtain access to > such a private static final and observe the framework's secret. It should use > condy. In addition, we need to differentiate if a lookup object is created > from > the original lookup class or created from teleporting e.g. `Lookup::in` > and `MethodHandles::privateLookupIn`. > > This proposes to add a new `ORIGINAL` bit that is only set if the lookup > object is created by `MethodHandles::lookup` or by bootstrap method > invocation. > The operations only apply to a Lookup object with original access are: >- create method handles for caller-sensitve methods >- obtain class data associated with the lookup class > > No change to `Lookup::hasFullPrivilegeAccess` and `Lookup::toString` which > ignores the ORIGINAL bit. > > > Compatibility Risks > --- > > `Lookup::lookupModes` includes a new `ORIGINAL` bit. Most lookup operations > ignore this original bit except creating method handles for caller-sensitive > methods > that expects the lookup from the original lookup class. Existing code > compares > the r
Integrated: 8230501: Class data support for hidden classes
On Wed, 11 Nov 2020 18:52:10 GMT, Mandy Chung wrote: > Provide the `Lookup::defineHiddenClassWithClassData` API that allows live > objects > be shared between a hidden class and other classes. A hidden class can load > these live objects as dynamically-computed constants via this API. > > Specdiff > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8230501/specdiff/overview-summary.html > > With this class data support and hidden classes, > `sun.misc.Unsafe::defineAnonymousClass` > will be deprecated for removal. Existing libraries should replace their > calls to `sun.misc.Unsafe::defineAnonymousClass` with > `Lookup::defineHiddenClass` > or `Lookup::defineHiddenClassWithClassData`. > > This patch also updates the implementation of lambda meta factory and > `MemoryAccessVarHandleGenerator` to use class data. No performance > difference > observed in the jdk.incubator.foreign microbenchmarks. A side note: > `MemoryAccessVarHandleGenerator` is removed in the upcoming integration of > JDK-8254162 but it helps validating the class data support. > > Background > -- > > This is an enhancement following up JEP 371: Hidden Classes w.r.t. > "Constant-pool patching" in the "Risks and Assumption" section. > > A VM-anonymous class can be defined with its constant-pool entries already > resolved to concrete values. This allows critical constants to be shared > between a VM-anonymous class and the language runtime that defines it, and > between multiple VM-anonymous classes. For example, a language runtime will > often have `MethodHandle` objects in its address space that would be useful > to newly-defined VM-anonymous classes. Instead of the runtime serializing > the objects to constant-pool entries in VM-anonymous classes and then > generating bytecode in those classes to laboriously `ldc` the entries, > the runtime can simply supply `Unsafe::defineAnonymousClass` with references > to its live objects. The relevant constant-pool entries in the newly-defined > VM-anonymous class are pre-linked to those objects, improving performance > and reducing footprint. In addition, this allows VM-anonymous classes to > refer to each other: Constant-pool entries in a class file are based on names. > They thus cannot refer to nameless VM-anonymous classes. A language runtime > can, > however, easily track the live Class objects for its VM-anonymous classes and > supply them to `Unsafe::defineAnonymousClass`, thus pre-linking the new > class's > constant pool entries to other VM-anonymous classes. > > This extends the hidden classes to allow live objects to be injected > in a hidden class and loaded them via condy. > > Details > --- > > A new `Lookup::defineHiddenClassWithClassData` API takes additional > `classData` argument compared to `Lookup::defineHiddenClass`. > Class data can be method handles, lookup objects, arbitrary user objects > or collections of all of the above. > > This method behaves as if calling `Lookup::defineHiddenClass` to define > a hidden class with a private static unnamed field that is initialized > with `classData` at the first instruction of the class initializer. > > `MethodHandles::classData(Lookup lookup, String name, Class type)` and > `MethodHandles::classDataAt(Lookup lookup, String name, Class type, int > index)` > are the bootstrap methods to load the class data of the given lookup's lookup > class. > The hidden class will be initialized when `classData` method is called if > the hidden class has not been initialized. > > For a class data containing more than one single element, libraries can > create their convenience method to load a single live object via condy. > > Frameworks sometimes want to dynamically create a hidden class (HC) and add it > it the lookup class nest and have HC to carry secrets hidden from that nest. > In this case, frameworks should not to use private static finals (in the HCs > they spin) to hold secrets because a nestmate of HC may obtain access to > such a private static final and observe the framework's secret. It should use > condy. In addition, we need to differentiate if a lookup object is created > from > the original lookup class or created from teleporting e.g. `Lookup::in` > and `MethodHandles::privateLookupIn`. > > This proposes to add a new `ORIGINAL` bit that is only set if the lookup > object is created by `MethodHandles::lookup` or by bootstrap method > invocation. > The operations only apply to a Lookup object with original access are: >- create method handles for caller-sensitve methods >- obtain class data associated with the lookup class > > No change to `Lookup::hasFullPrivilegeAccess` and `Lookup::toString` which > ignores the ORIGINAL bit. > > > Compatibility Risks > --- > > `Lookup::lookupModes` includes a new `ORIGINAL` bit. Most lookup operations > ignore this original bit except creating method handles for caller-sensitive > methods > that expects the lookup from the ori
Re: RFR: 8251989: Hex formatting and parsing utility [v12]
On Mon, 30 Nov 2020 20:56:17 GMT, Roger Riggs wrote: >> java.util.HexFormat utility: >> >> - Format and parse hexadecimal strings, with parameters for delimiter, >> prefix, suffix and upper/lowercase >> - Static factories and builder methods to create HexFormat copies with >> modified parameters. >> - Consistent naming of methods for conversion of byte arrays to formatted >> strings and back: formatHex and parseHex >> - Consistent naming of methods for conversion of primitive types: >> toHexDigits... and fromHexDigits... >> - Prefix and suffixes now apply to each formatted value, not the string as >> a whole >> - Using java.util.Appendable as a target for buffered conversions so output >> to Writers and PrintStreams >>like System.out are supported in addition to StringBuilder. (IOExceptions >> are converted to unchecked exceptions) >> - Immutable and thread safe, a "value-based" class >> >> See the [HexFormat >> javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html) >> for details. >> >> Review comments and suggestions welcome. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Clarified hexadecimal characters used in converting from characters to > values to be strictly 0-9, a-f, and A-F. > Added a test to verify isHexDigit and fromHexDigit for the entire range of > chars Looks good to me. - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8166026: refactor shell tests to java
On Fri, 27 Nov 2020 18:50:19 GMT, Ivan Šipka wrote:
> @iignatev could you please review? Thank you.
>
> note to self:
> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
it would be much easier to review if it was 4 separate RFEs/RFRs.
many new files miss a newline at the end.
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 60:
> 58: outputAnalyzer.shouldHaveExitValue(0);
> 59: outputAnalyzer.stderrShouldMatch(EXPECTED_RESULT_STDERR);
> 60: outputAnalyzer.stdoutShouldMatch(EXPECTED_RESULT_STDOUT);
you can chain these methods.
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 61:
> 59: outputAnalyzer.stderrShouldMatch(EXPECTED_RESULT_STDERR);
> 60: outputAnalyzer.stdoutShouldMatch(EXPECTED_RESULT_STDOUT);
> 61: assertEquals(outputAnalyzer.getOutput(),EXPECTED_RESULT_STDOUT +
> EXPECTED_RESULT_STDERR);
I'd rather check stdout and stderr independently, this will also make checks at
59 and 60 redundant.
test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line
70:
> 68: outputAnalyzer.shouldHaveExitValue(exitValue);
> 69: outputAnalyzer.stderrShouldMatch(stdErrMatch);
> 70: outputAnalyzer.stdoutShouldMatch(stdOutMatch);
why do you use `ShouldMatch` and not `ShouldContain` here?
test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java line 53:
> 51: Files.createDirectories(REPOSITORY_PATH);
> 52: List classes = List.of("A.class", "B.class", "C.class");
> 53: for (String fileName : classes) {
is this really needed for the test to operate correctly? or can we just use
_regular_ `TEST_CLASSES` as CP?
-
Changes requested by iignatyev (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: JDK-8249836 java/io/IOException/LastErrorString.java should have bug-id as 1st word in @ignore
On Fri, 27 Nov 2020 16:11:54 GMT, Mahendra Chhipa wrote: > …id as 1st word in @ignore > > https://bugs.openjdk.java.net/browse/JDK-8249836 LGTM - Marked as reviewed by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1482
Re: RFR: 8256894: define test groups
On Tue, 24 Nov 2020 16:13:59 GMT, Ivan Šipka wrote: > Defined new test groups as defined in ticket. @fguallini @frkator, you will need to open a new JBS ticket for this change. test/jdk/TEST.groups line 326: > 324: :jdk_text \ > 325: :core_tools \ > 326: :jdk_other it would seem you have introduced a trailing space - Changes requested by iignatyev (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8256894: define test groups
On Wed, 25 Nov 2020 20:31:50 GMT, Ivan Šipka wrote: >> test/jdk/TEST.groups line 327: >> >>> 325: :core_tools \ >>> 326: :jdk_other \ >>> 327: :jdk_core_manual >> >> Please don't add manual tests to jdk_core. > > Removed. aren't they already a part of `jdk_core` test group? e.g. `java/net/HugeDataTransferTest.java` is in `:jdk_net` (as `jdk_net` includes `java/net` directory), and `:jdk_core` includes `:jdk_net`? - PR: https://git.openjdk.java.net/jdk/pull/1416
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 19:44:52 GMT, Mandy Chung wrote: >> 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: >> >> - Improving getPermittedSubclasses javadoc. >> - Merge branch 'master' into JDK-8246778 >> - Moving checkPackageAccess from getPermittedSubclasses to a separate >> method. >> - Improving getPermittedSubclasses() javadoc. >> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both >> for sealed classes in named and unnamed modules. >> - Removing unnecessary file. >> - Tweaking javadoc. >> - Reflecting review comments w.r.t. narrowing conversion. >> - Improving checks in getPermittedSubclasses() >> - Merging master into JDK-8246778 >> - ... and 2 more: >> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179 > > src/java.base/share/classes/java/lang/Class.java line 4480: > >> 4478: } >> 4479: >> 4480: private native Class[] getPermittedSubclasses0(); > > Does this JVM method return the permitted subclasses or subinterfaces with > the following conditions enforced by JLS: > >- If a sealed class C belongs to a named module, then every class named in > the permits clause of the declaration of C must belong to the same module as C >- If a sealed class C belongs to an unnamed module, then every class named > in the permits clause of the declaration of C must belong to the same package > as C > > I didn't check the VM implementation. > > If the return array contains only classes as specified above, > `checkPackageAccessForClasses` can be simplified. The JVM method that returns the permitted subclasses (and interfaces) does not weed out permitted subclasses based on the above module requirements. It returns all the classes listed in the PermittedSubclasses attribute that it is able to load. - PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8159746: (proxy) Support for default methods [v8]
> This proposes a new static `Proxy::invokeDefaultMethod` method to invoke > the given default method on the given proxy instance. > > The implementation looks up a method handle for `invokespecial` instruction > as if called from with the proxy class as the caller, equivalent to calling > `X.super::m` where `X` is a proxy interface of the proxy class and > `X.super::m` will resolve to the specified default method. > > The implementation will call a private static `proxyClassLookup(Lookup > caller)` > method of the proxy class to obtain its private Lookup. This private method > in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy > class > with full privilege access to use, or else `IllegalAccessException` will be > thrown. > > This patch also proposes to define a proxy class in an unnamed module to > a dynamic module to strengthen encapsulation such that they are only > unconditionally exported from a named module but not open for deep reflective > access. This only applies to the case if all the proxy interfaces are public > and in a package that is exported or open. > > One dynamic module is created for each class loader that defines proxies. > The change changes the dynamic module to contain another package (same > name as the module) that is unconditionally exported and is opened to > `java.base` only. > > There is no change to the package and module of the proxy class for > the following cases: > > - if at least one proxy interface is non-public, then the proxy class is > defined > in the package and module of the non-public interfaces > - if at least one proxy is in a package that is non-exported and non-open, > if all proxy interfaces are public, then the proxy class is defined in > a non-exported, non-open package of a dynamic module. > > The spec change is that a proxy class used to be defined in an unnamed > module, i.e. in a exported and open package, is defined in an unconditionally > exported but non-open package. Programs that assume it to be open > unconditionally > will be affected and cannot do deep reflection on such proxy classes. > > Peter Levart contributed an initial prototype [1] (thanks Peter). I think > the exceptions could be simplified as more checking should be done prior to > the invocation of the method handle like checking the types of the arguments > with the method type. This approach avoids defining a public API > `protected Proxy::$$proxyClassLookup$$` method. Instead it defines a > private static method that is restricted for Proxy class to use (by > taking a caller parameter to ensure it's a private lookup on Proxy class). > > javadoc/specdiff: > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/ > http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/ > > [1] > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: Improve comments per Alan's feedback - Changes: - all: https://git.openjdk.java.net/jdk/pull/313/files - new: https://git.openjdk.java.net/jdk/pull/313/files/d72d627f..bb23cfa8 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=313&range=07 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=313&range=06-07 Stats: 17 lines in 1 file changed: 7 ins; 1 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/313.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/313/head:pull/313 PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v4]
On Mon, 30 Nov 2020 13:39:12 GMT, Chris Hegarty wrote: >> The ByteBuffers micro benchmark seems to be a little dated. >> >> It should be a useful resource to leverage when analysing the performance >> impact of any potential implementation changes in the byte buffer classes. >> More specifically, the impact of such changes on the performance of sharp >> memory access operations. >> >> This issue proposes to update the benchmark in the following ways to meet >> the aforementioned use-case: >> >> 1. Remove allocation from the individual benchmarks - it just creates noise. >> 2. Consolidate per-thread shared heap and direct buffers. >> 3. All scenarios now use absolute memory access operations - so no state of >> the shared buffers is considered. >> 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. >> 5. There seems to have been an existing bug in the test where the size >> parameter was not always considered - this is now fixed. > > Chris Hegarty has updated the pull request incrementally with two additional > commits since the last revision: > > - Add explicitly allocated heap carrier buffer tests > - Replace Single with Loop Marked as reviewed by bpb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1430
Re: RFR: 8251989: Hex formatting and parsing utility [v12]
> java.util.HexFormat utility: > > - Format and parse hexadecimal strings, with parameters for delimiter, > prefix, suffix and upper/lowercase > - Static factories and builder methods to create HexFormat copies with > modified parameters. > - Consistent naming of methods for conversion of byte arrays to formatted > strings and back: formatHex and parseHex > - Consistent naming of methods for conversion of primitive types: > toHexDigits... and fromHexDigits... > - Prefix and suffixes now apply to each formatted value, not the string as a > whole > - Using java.util.Appendable as a target for buffered conversions so output > to Writers and PrintStreams >like System.out are supported in addition to StringBuilder. (IOExceptions > are converted to unchecked exceptions) > - Immutable and thread safe, a "value-based" class > > See the [HexFormat > javadoc](http://cr.openjdk.java.net/~rriggs/8251989-hex-formatter/java.base/java/util/HexFormat.html) > for details. > > Review comments and suggestions welcome. Roger Riggs has updated the pull request incrementally with one additional commit since the last revision: Clarified hexadecimal characters used in converting from characters to values to be strictly 0-9, a-f, and A-F. Added a test to verify isHexDigit and fromHexDigit for the entire range of chars - Changes: - all: https://git.openjdk.java.net/jdk/pull/482/files - new: https://git.openjdk.java.net/jdk/pull/482/files/29f9bf7b..a1ce9d7c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=482&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=482&range=10-11 Stats: 46 lines in 2 files changed: 24 ins; 0 del; 22 mod Patch: https://git.openjdk.java.net/jdk/pull/482.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/482/head:pull/482 PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 15:59:07 GMT, Jan Lahoda wrote:
>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>>
>> From the original PR:
>>
>>> Please review the code for the second iteration of sealed classes. In this
>>> iteration we are:
>>>
>>> * Enhancing narrowing reference conversion to allow for stricter
>>> checking of cast conversions with respect to sealed type hierarchies
>>>
>>> * Also local classes are not considered when determining implicitly
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>>
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>>> still in the same method, the return type has been changed to Class[]
>>> instead of the previous ClassDesc[]
>>>
>>> * adding code to make sure that annotations can't be sealed
>>>
>>> * improving some tests
>>>
>>>
>>> TIA
>>>
>>> Related specs:
>>> [Sealed Classes
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>>
>> This PR strives to reflect the review comments from 1227:
>> * adjustments to javadoc of j.l.Class methods
>> * package access checks in Class.getPermittedSubclasses()
>> * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> 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:
>
> - Improving getPermittedSubclasses javadoc.
> - Merge branch 'master' into JDK-8246778
> - Moving checkPackageAccess from getPermittedSubclasses to a separate method.
> - Improving getPermittedSubclasses() javadoc.
> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both
> for sealed classes in named and unnamed modules.
> - Removing unnecessary file.
> - Tweaking javadoc.
> - Reflecting review comments w.r.t. narrowing conversion.
> - Improving checks in getPermittedSubclasses()
> - Merging master into JDK-8246778
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179
src/java.base/share/classes/java/lang/Class.java line 3042:
> 3040: for (Class c : classes) {
> 3041: // skip the package access check on a proxy class in
> default proxy package
> 3042: if (!Proxy.isProxyClass(c) ||
> ReflectUtil.isNonPublicProxyClass(c)) {
If a sealed class is in a named module, the permitted subclasses/subinterfaces
are in the same module as the sealed class. If a sealed class is in an unnamed
module, it will be in the same runtime package as the sealed class. A proxy
class is dynamically generated and not intended for statically named in
`permits` clause of a sealed class`. It can be in a different module or
different package. So a permitted subclass or interface should never be a
proxy class.
So the package access check for permitted subclasses/subinterfaces can be
simplified. I would suggest this check be inlined in `getPermittedSubclasses`
as follows:
SecurityManager sm = System.getSecurityManager();
if (subclasses.length > 0 && sm != null) {
ClassLoader ccl =
ClassLoader.getClassLoader(Reflection.getCallerClass());
ClassLoader cl = getClassLoader0();
if (ReflectUtil.needsPackageAccessCheck(ccl, cl)) {
Set packages = new HashSet<>();
for (Class c : subclasses) {
if (Proxy.isProxyClass(c))
throw new InternalError("a permitted subclass should
not be a proxy class: " + c);
String pkg = c.getPackageName();
if (!pkg.isEmpty())
packages.add(pkg);
}
for (String pkg : packages) {
sm.checkPackageAccess(pkg);
}
}
}
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: Lookup.defineAnonymousClass() vs indy
On 11/30/20 12:25 PM, fo...@univ-mlv.fr wrote: The real question is why the descriptor is not rewritten to replace the owning class by the hidden class when lookup.defineHiddenClass is called, i believe this is the behavior of unsafe.defineAnonymousClass. JVM does not do any magic for hidden classes which is a feature, not a bug. A hidden class _cannot be named_ in any descriptor. The only adjustment for hidden classes, compared to normal class, after a hidden class C is derived and linked by JVM is that: (1) During verification, whenever it is necessary to load the class named CN, the attempt succeeds, producing class C. No request is made of any class loader. (2) On any attempt to resolve the entry in the run-time constant pool indicated by `this_class`, the symbolic reference is considered to be resolved to C and resolution always succeeds immediately. Mandy
Re: Lookup.defineAnonymousClass() vs indy
> De: "mandy chung"
> À: "Remi Forax" , "core-libs-dev"
>
> Envoyé: Lundi 30 Novembre 2020 19:16:25
> Objet: Re: Lookup.defineAnonymousClass() vs indy
> The implementation method to be invoked for this lambda is a static bridge
> method
> taking this hidden class's instance as the parameter, i.e. a descriptor
> referencing
> a hidden class, which will fail to resolve.
> 0: #104 REF_invokeStatic
>
> java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
> Method arguments:
> #111 ()V
> #112 REF_invokeStatic
> HiddenClassWithIndy.lambda$test$0:(LHiddenClassWithIndy;)V
> #111 ()V
> Now that a hidden class is a nestmate, should/can this bridge method be an
> instance method if this class is defined as a hidden class ?
Here the parameter of the lambda bridge is a captured local variable, it feels
wrong to me to have s special case if the first captured local variable as the
same type as the owning/enclosing class.
And when you compile a class and generate the lambda bridge method, the
compiler has no idea if the class will be used as an hidden class or not.
The real question is why the descriptor is not rewritten to replace the owning
class by the hidden class when lookup.defineHiddenClass is called, i believe
this is the behavior of unsafe.defineAnonymousClass.
> Mandy
Rémi
> On 11/29/20 7:34 AM, Remi Forax wrote:
>> Hi Mandy, hi all,
>> it seems that when defineAnonymousClass rewrites the currentClass, it doesn't
>> work if there is an invokedynamic in the classfile, so defineHiddenClass
>> fails
>> with a VerifyError when the hidden class is verified.
>> Here is an example showing the issue
>> ---
>> import java.io.IOException;
>> import java.lang.invoke.MethodHandles;
>> public class HiddenClassWithIndy {
>> public void test() {
>> var a = new HiddenClassWithIndy();
>> Runnable r = () -> System.out.println(a);
>> }
>> public static void main(String[] args) throws IOException,
>> IllegalAccessException {
>> byte[] bytecode;
>>try(var input =
>>
>> HiddenClassWithIndy.class.getClassLoader().getResourceAsStream(HiddenClassWithIndy.class.getName().replace('.',
>> '/') + ".class")) {
>> if (input == null) {
>> throw new AssertionError();
>> }
>> bytecode = input.readAllBytes();
>> }
>> var hiddenLookup = MethodHandles.lookup().defineHiddenClass(bytecode,
>> true);
>> }
>> }
>> ---
>> The error message:
>> Exception in thread "main" java.lang.VerifyError: Bad type on operand stack
>> Exception Details:
>> Location:
>>fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400.test()V @9:
>> invokedynamic
>> Reason:
>>Type 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' (current
>> frame, stack[0]) is not assignable to
>> 'fr/umlv/transmogrif/HiddenClassWithIndy'
>> Current Frame:
>> bci: @9
>> flags: { }
>>locals: { 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400',
>> 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' }
>> stack: { 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' }
>> Bytecode:
>> 000: bb00 0759 b700 094c 2bba 000a 4db1
>> 010:
>> at java.base/java.lang.ClassLoader.defineClass0(Native Method)
>> at java.base/java.lang.System$2.defineClass(System.java:2193)
>> at
>>
>> java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2235)
>> at
>>
>> java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClassAsLookup(MethodHandles.java:2216)
>> at
>>
>> java.base/java.lang.invoke.MethodHandles$Lookup.defineHiddenClass(MethodHandles.java:1952)
>> at
>> fr.umlv.transmogrif.HiddenClassWithIndy.main(HiddenClassWithIndy.java:20)
>> regards,
>> Rémi
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 15:59:07 GMT, Jan Lahoda wrote: >> This pull request replaces https://github.com/openjdk/jdk/pull/1227. >> >> From the original PR: >> >>> Please review the code for the second iteration of sealed classes. In this >>> iteration we are: >>> >>> * Enhancing narrowing reference conversion to allow for stricter >>> checking of cast conversions with respect to sealed type hierarchies >>> >>> * Also local classes are not considered when determining implicitly >>> declared permitted direct subclasses of a sealed class or sealed interface >>> >>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >>> still in the same method, the return type has been changed to Class[] >>> instead of the previous ClassDesc[] >>> >>> * adding code to make sure that annotations can't be sealed >>> >>> * improving some tests >>> >>> >>> TIA >>> >>> Related specs: >>> [Sealed Classes >>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >>> [Sealed Classes >>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >>> [Additional: Contextual >>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) >> >> This PR strives to reflect the review comments from 1227: >> * adjustments to javadoc of j.l.Class methods >> * package access checks in Class.getPermittedSubclasses() >> * fixed to the narrowing conversion/castability as pointed out by Maurizio > > 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: > > - Improving getPermittedSubclasses javadoc. > - Merge branch 'master' into JDK-8246778 > - Moving checkPackageAccess from getPermittedSubclasses to a separate method. > - Improving getPermittedSubclasses() javadoc. > - Enhancing the Class.getPermittedSubclasses() test to verify behavior both > for sealed classes in named and unnamed modules. > - Removing unnecessary file. > - Tweaking javadoc. > - Reflecting review comments w.r.t. narrowing conversion. > - Improving checks in getPermittedSubclasses() > - Merging master into JDK-8246778 > - ... and 2 more: > https://git.openjdk.java.net/jdk/compare/6e006223...4d484179 src/java.base/share/classes/java/lang/Class.java line 4480: > 4478: } > 4479: > 4480: private native Class[] getPermittedSubclasses0(); Does this JVM method return the permitted subclasses or subinterfaces with the following conditions enforced by JLS: - If a sealed class C belongs to a named module, then every class named in the permits clause of the declaration of C must belong to the same module as C - If a sealed class C belongs to an unnamed module, then every class named in the permits clause of the declaration of C must belong to the same package as C Should the library implementation of `Class::getPermittedSubclasses` filter that if not done by `getPermittedSubclasses0`? If the return array contains only classes as specified above, `checkPackageAccessForClasses` can be simplified. - PR: https://git.openjdk.java.net/jdk/pull/1483
Integrated: 8180352: Add Stream.toList() method
On Tue, 3 Nov 2020 01:33:32 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&page=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. This pull request has now been integrated. Changeset: 41dbc139 Author:Stuart Marks URL: https://git.openjdk.java.net/jdk/commit/41dbc139 Stats: 445 lines in 8 files changed: 358 ins; 42 del; 45 mod 8180352: Add Stream.toList() method Reviewed-by: psandoz - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8180352: Add Stream.toList() method [v4]
On Wed, 25 Nov 2020 17:14:43 GMT, Peter Levart wrote: >> An alternative with similar performance would be to do a Stream.toArray() >> and then copy that array into new Object[] and then wrap that copy with >> listFromTrustedArrayNullsAllowed(). The difference would be in the >> serialization format of the resulting List and maybe also in the access >> performance of resulting List (no indirection via the unmodifiableList >> wrapper and different type for JIT to speculate about). So if we want the >> resulting List to behave exactly the same in both implementations of >> toList(), then this alternative might be preferable. WDYT? > > Such alternative might also be faster using intrisified array copying method > (here measuring just copying overhead): > > Benchmark (len) Mode Cnt Score Error Units > ToListBench.toList1 10 avgt 10 14.213 ±0.061 ns/op > ToListBench.toList1 1000 avgt 10 541.883 ±3.845 ns/op > ToListBench.toList1 100 avgt 10 753223.523 ± 4656.664 ns/op > ToListBench.toList2 10 avgt 10 8.810 ±0.052 ns/op > ToListBench.toList2 1000 avgt 10 264.748 ±0.807 ns/op > ToListBench.toList2 100 avgt 10 349518.502 ± 3242.061 ns/op > > https://gist.github.com/plevart/974b67b65210f8dd122773f481c0a603 I don't want to change the default implementation and its specification, for a couple reasons. First, this implementation won't be used in normal operation, as it's overridden by the JDK implementation. (I expect that stream extension libraries will be updated to override it as well.) Second, I'm quite uncomfortable using an internal method from within a default implementation. Right now this method is an agreement between the unmodifiable collections implementation and the ReferencePipeline implementation, and we have complete freedom to change this agreement at any time. If this internal method were used from a default implementation, its behavior would have to be specified in `@implSpec` somehow. Certainly one could write some very vague words for the specification that allow some freedom to make changes, but this has to choose between a better specification and more implementation freedom. I don't see the value in having to make this tradeoff at all. An extra copy can be avoided via a private agreement between ArrayList and Arrays.asList, which should probably be done in any case. - PR: https://git.openjdk.java.net/jdk/pull/1026
Re: RFR: 8257164: Share LambdaForms for VH linkers/invokers.
On Thu, 26 Nov 2020 13:13:43 GMT, Vladimir Ivanov wrote: > Introduce sharing of `LambdaForms` for `VarHandle` linkers and invokers. > It reduces the number of LambdaForms needed at runtime. > > Testing: tier1-4 Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1455
Re: RFR: 8257189: Handle concurrent updates of MH.form better
On Thu, 26 Nov 2020 21:23:16 GMT, Vladimir Ivanov wrote: > Concurrent updates may lead to redundant LambdaForms created and unnecessary > class loading when those are compiled. > > Most notably, it severely affects MethodHandle customization: when a > MethodHandle is called from multiple threads, every thread starts > customization which takes enough time for other threads to join, but only one > of those customizations will be picked. > > Coordination between threads requesting the updates and letting a single > thread proceed avoids the aforementioned problem. Moreover, there's no need > to wait until the update in-flight is over: all other threads (except the one > performing the update) can just proceed with the invocation using the > existing MH.form. > > Testing: > - manually monitored the behavior on a stress test from > [JDK-8252049](https://bugs.openjdk.java.net/browse/JDK-8252049) > - tier1-4 Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1472
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 15:59:07 GMT, Jan Lahoda wrote:
>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>>
>> From the original PR:
>>
>>> Please review the code for the second iteration of sealed classes. In this
>>> iteration we are:
>>>
>>> * Enhancing narrowing reference conversion to allow for stricter
>>> checking of cast conversions with respect to sealed type hierarchies
>>>
>>> * Also local classes are not considered when determining implicitly
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>>
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>>> still in the same method, the return type has been changed to Class[]
>>> instead of the previous ClassDesc[]
>>>
>>> * adding code to make sure that annotations can't be sealed
>>>
>>> * improving some tests
>>>
>>>
>>> TIA
>>>
>>> Related specs:
>>> [Sealed Classes
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>>
>> This PR strives to reflect the review comments from 1227:
>> * adjustments to javadoc of j.l.Class methods
>> * package access checks in Class.getPermittedSubclasses()
>> * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> 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:
>
> - Improving getPermittedSubclasses javadoc.
> - Merge branch 'master' into JDK-8246778
> - Moving checkPackageAccess from getPermittedSubclasses to a separate method.
> - Improving getPermittedSubclasses() javadoc.
> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both
> for sealed classes in named and unnamed modules.
> - Removing unnecessary file.
> - Tweaking javadoc.
> - Reflecting review comments w.r.t. narrowing conversion.
> - Improving checks in getPermittedSubclasses()
> - Merging master into JDK-8246778
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179
src/java.base/share/classes/java/lang/Class.java line 4463:
> 4461: * @apiNote
> 4462: * Sealed class or interface has no relationship with
> 4463: * {@linkplain Package#isSealed package sealing}.
Package sealing is legacy. Remi suggests to take out this api note which
sounds good to me.
The API note in Package::isSealed has made this clear which has no relationship
with sealed class or interface.
src/java.base/share/classes/java/lang/Class.java line 4415:
> 4413: * array.
> 4414: *
> 4415: * @return an array of class objects of the permitted subclasses of
> this class or interface
Nit: s/class objects/{@code Class} objects/
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8257164: Share LambdaForms for VH linkers/invokers.
On Thu, 26 Nov 2020 13:13:43 GMT, Vladimir Ivanov wrote: > Introduce sharing of `LambdaForms` for `VarHandle` linkers and invokers. > It reduces the number of LambdaForms needed at runtime. > > Testing: tier1-4 Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/1455
Re: RFR: 8159746: (proxy) Support for default methods [v7]
On Mon, 30 Nov 2020 19:14:08 GMT, Mandy Chung wrote:
>> src/java.base/share/classes/java/lang/reflect/Proxy.java line 1281:
>>
>>> 1279: * @return a lookup for proxy class of this proxy instance
>>> 1280: */
>>> 1281: private static MethodHandles.Lookup
>>> proxyClassLookup(MethodHandles.Lookup caller, Class proxyClass) {
>>
>> The method description could be a bit clearer. It invokes the proxy's
>> proxyClassLookup method to get a Lookup on the proxy class ("this proxy
>> instance" is just a bit confusing as proxyClassLookup is static).
>> I guess the caller parameter isn't really needed as it could be crated in
>> proxyClassLookup.
>
> The caller parameter is just another level of defense. I updated as:
>
> /**
> - * Returns a Lookup object for the lookup class which is the class of
> this
> - * proxy instance.
> + * This method invokes the proxy's proxyClassLookup method to get a
> + * Lookup on the proxy class.
> *
> * @return a lookup for proxy class of this proxy instance
> */
Much better, thanks!
-
PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8159746: (proxy) Support for default methods [v7]
On Mon, 30 Nov 2020 17:28:52 GMT, Alan Bateman wrote:
>> Mandy Chung 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 25 additional
>> commits since the last revision:
>>
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> proxy-default-method
>> - update copyright header
>> - clean up DefaultMethodProxy test
>> - Performance improvement contributed by plevart
>> - Clean up the patch. Rename to
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> proxy-default-method
>> - move invokeDefaultMethod to InvocationHandler and throw IAE if access
>> check fails
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> proxy-default-method
>> - Merge branch 'master' of https://github.com/openjdk/jdk into
>> proxy-default-method
>> - minor tweak to the spec wording and impl
>> - ... and 15 more:
>> https://git.openjdk.java.net/jdk/compare/1580f68b...d72d627f
>
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 1227:
>
>> 1225: if (proxyInterfaces.contains(declaringClass))
>> 1226: return declaringClass;
>> 1227:
>
> It might be beneficial for future maintainers if you could include a comment
> here on the search order.
What about:
+// find the first proxy interface that inherits the default method
+// i.e. the declaring class of the default method is a superinterface
+// of the proxy interface
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 1281:
>
>> 1279: * @return a lookup for proxy class of this proxy instance
>> 1280: */
>> 1281: private static MethodHandles.Lookup
>> proxyClassLookup(MethodHandles.Lookup caller, Class proxyClass) {
>
> The method description could be a bit clearer. It invokes the proxy's
> proxyClassLookup method to get a Lookup on the proxy class ("this proxy
> instance" is just a bit confusing as proxyClassLookup is static).
> I guess the caller parameter isn't really needed as it could be crated in
> proxyClassLookup.
The caller parameter is just another level of defense. I updated as:
/**
- * Returns a Lookup object for the lookup class which is the class of this
- * proxy instance.
+ * This method invokes the proxy's proxyClassLookup method to get a
+ * Lookup on the proxy class.
*
* @return a lookup for proxy class of this proxy instance
*/
-
PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8166026: refactor shell tests to java
On Fri, 27 Nov 2020 18:50:19 GMT, Ivan Šipka wrote:
> @iignatev could you please review? Thank you.
>
> note to self:
> jtreg test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java
> test/jdk/java/lang/SecurityManager/modules/CustomSecurityManagerTest.java
> test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java
> test/jdk/java/lang/annotation/loaderLeak/LoaderLeakTest.java
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIo.java line 26:
> 24: import static java.lang.ProcessBuilder.Redirect.*;
> 25:
> 26: class InheritIo {
The rename of the class is unnecessary and less readable.
The function being tested is inheritIO and the test name should match. (As does
the directory it is in).
test/jdk/java/lang/ProcessBuilder/InheritIO/InheritIoTest.java line 42:
> 40:
> 41: public class InheritIoTest {
> 42:
Having two classes and files with similar names is confusing. TestInhieritIO
and InheritIOTest.
Since InheritIO already has subclasses that are the ones being invoked;
that leaves methods in InheritIO to have the test cases.
test/jdk/java/lang/Thread/uncaughtexceptions/UncaughtExceptionsTest.java line
40:
> 38: */
> 39: public class UncaughtExceptionsTest {
> 40:
As with InheritIO, the nested classes that are invoked can be included in a
single .java file.
-
PR: https://git.openjdk.java.net/jdk/pull/1484
Re: RFR: 8253751: Dependencies of automatic modules are not propagated through module layers [v2]
On Wed, 25 Nov 2020 22:09:11 GMT, Mandy Chung wrote:
>> 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 ten additional
>> commits since the last revision:
>>
>> - Formatting nit
>> - Merge
>> - Merge
>> - Merge
>> - Add @bug to test
>> - Fixed typo in comment
>> - Merge
>> - Merge
>> - If module reads automatic module in parent configuration then reads all
>> automatic modules
>
> src/java.base/share/classes/java/lang/module/Resolver.java line 580:
>
>> 578: if (m2.descriptor().isAutomatic()) {
>> 579: m2.reads()
>> 580: .stream()
>
> Nit: not sure if this formatting is intentional or you meant
> `m2.reads().stream()` in one line (which I like better).
Thanks, I suspect I just hit Return and the IDE formatted it. Fixed now.
-
PR: https://git.openjdk.java.net/jdk/pull/1391
Re: Lookup.defineAnonymousClass() vs indy
The implementation method to be invoked for this lambda is a static bridge
method
taking this hidden class's instance as the parameter, i.e. a descriptor
referencing
a hidden class, which will fail to resolve.
0: #104 REF_invokeStatic
java/lang/invoke/LambdaMetafactory.metafactory:(Ljava/lang/invoke/MethodHandles$Lookup;Ljava/lang/String;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodType;Ljava/lang/invoke/MethodHandle;Ljava/lang/invoke/MethodType;)Ljava/lang/invoke/CallSite;
Method arguments:
#111 ()V
#112 REF_invokeStatic
HiddenClassWithIndy.lambda$test$0:(LHiddenClassWithIndy;)V
#111 ()V
Now that a hidden class is a nestmate, should/can this bridge method be
an instance method if this class is defined as a hidden class?
Mandy
On 11/29/20 7:34 AM, Remi Forax wrote:
Hi Mandy, hi all,
it seems that when defineAnonymousClass rewrites the currentClass, it doesn't
work if there is an invokedynamic in the classfile, so defineHiddenClass fails
with a VerifyError when the hidden class is verified.
Here is an example showing the issue
---
import java.io.IOException;
import java.lang.invoke.MethodHandles;
public class HiddenClassWithIndy {
public void test() {
var a = new HiddenClassWithIndy();
Runnable r = () -> System.out.println(a);
}
public static void main(String[] args) throws IOException,
IllegalAccessException {
byte[] bytecode;
try(var input =
HiddenClassWithIndy.class.getClassLoader().getResourceAsStream(HiddenClassWithIndy.class.getName().replace('.',
'/') + ".class")) {
if (input == null) {
throw new AssertionError();
}
bytecode = input.readAllBytes();
}
var hiddenLookup = MethodHandles.lookup().defineHiddenClass(bytecode,
true);
}
}
---
The error message:
Exception in thread "main" java.lang.VerifyError: Bad type on operand stack
Exception Details:
Location:
fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400.test()V @9:
invokedynamic
Reason:
Type 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' (current
frame, stack[0]) is not assignable to 'fr/umlv/transmogrif/HiddenClassWithIndy'
Current Frame:
bci: @9
flags: { }
locals: { 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400',
'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' }
stack: { 'fr/umlv/transmogrif/HiddenClassWithIndy+0x000801002400' }
Bytecode:
000: bb00 0759 b700 094c 2bba 000a 4db1
010:
at java.base/java.lang.ClassLoader.defineClass0(Native Method)
at java.base/java.lang.System$2.defineClass(System.java:2193)
at
java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClass(MethodHandles.java:2235)
at
java.base/java.lang.invoke.MethodHandles$Lookup$ClassDefiner.defineClassAsLookup(MethodHandles.java:2216)
at
java.base/java.lang.invoke.MethodHandles$Lookup.defineHiddenClass(MethodHandles.java:1952)
at
fr.umlv.transmogrif.HiddenClassWithIndy.main(HiddenClassWithIndy.java:20)
regards,
Rémi
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 15:59:07 GMT, Jan Lahoda wrote:
>> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>>
>> From the original PR:
>>
>>> Please review the code for the second iteration of sealed classes. In this
>>> iteration we are:
>>>
>>> * Enhancing narrowing reference conversion to allow for stricter
>>> checking of cast conversions with respect to sealed type hierarchies
>>>
>>> * Also local classes are not considered when determining implicitly
>>> declared permitted direct subclasses of a sealed class or sealed interface
>>>
>>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>>> still in the same method, the return type has been changed to Class[]
>>> instead of the previous ClassDesc[]
>>>
>>> * adding code to make sure that annotations can't be sealed
>>>
>>> * improving some tests
>>>
>>>
>>> TIA
>>>
>>> Related specs:
>>> [Sealed Classes
>>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>>> [Sealed Classes
>>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>>> [Additional: Contextual
>>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>>
>> This PR strives to reflect the review comments from 1227:
>> * adjustments to javadoc of j.l.Class methods
>> * package access checks in Class.getPermittedSubclasses()
>> * fixed to the narrowing conversion/castability as pointed out by Maurizio
>
> 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:
>
> - Improving getPermittedSubclasses javadoc.
> - Merge branch 'master' into JDK-8246778
> - Moving checkPackageAccess from getPermittedSubclasses to a separate method.
> - Improving getPermittedSubclasses() javadoc.
> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both
> for sealed classes in named and unnamed modules.
> - Removing unnecessary file.
> - Tweaking javadoc.
> - Reflecting review comments w.r.t. narrowing conversion.
> - Improving checks in getPermittedSubclasses()
> - Merging master into JDK-8246778
> - ... and 2 more:
> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179
src/java.base/share/classes/java/lang/Class.java line 4412:
> 4410: * The {@code Class} objects which can be obtained using this
> procedure
> 4411: * are indicated by elements of the returned array. If a {@code
> Class} object
> 4412: * cannot be obtained, it is silently ignored, and not included in
> the result
Thanks for the update, this reads much better.
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Integrated: 8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start, int end) is missing fast-path for String
On Tue, 29 Sep 2020 14:28:52 GMT, Сергей Цыпанов
wrote:
> Original mail:
> https://mail.openjdk.java.net/pipermail/core-libs-dev/2020-September/069197.html
>
> Hello,
>
> while working with `StringBuilder.insert()` I've spotted that its delegate
> `AbstractStringBuilder.insert()` is missing
> a fast-path for the most frequent case when its argument is `String`.
>
> Previously they did similart optimization for
> `StirngBuilder.append(CharSequence, int, int)`,
> see https://bugs.openjdk.java.net/browse/JDK-8224986
>
> I'd like to contribute a trivial patch that brings improvement for the case
> when SB's content is Latin1
> and inserted String is Latin1 as well.
>
> To measure improvement I've used simple benchmark:
> @BenchmarkMode(Mode.AverageTime)
> @OutputTimeUnit(TimeUnit.NANOSECONDS)
> @Fork(jvmArgsAppend = {"-Xms2g", "-Xmx2g"})
> public class StringBuilderInsertBenchmark {
>
> @Benchmark
> public StringBuilder insert(Data data) {
> String string = data.string;
> return new StringBuilder().append("ABC").insert(1, string, 1, data.length
> + 1);
> }
>
> @State(Scope.Thread)
> public static class Data {
> String string;
>
> @Param({"true", "false"})
> private boolean latin;
>
> @Param({"8", "64", "128", "1024"})
> private int length;
>
> @Setup
> public void setup() {
> String alphabet = latin
> ? "abcdefghijklmnopqrstuvwxyz"// English
> : "абвгдеёжзиклмнопрстуфхцчшщьыъэюя"; // Russian
>
> string = new RandomStringGenerator().randomString(alphabet, length + 2);
> }
> }
> }
>
> public final class RandomStringGenerator {
>
> public String randomString(String alphabet, int length) {
> char[] chars = alphabet.toCharArray();
>
> ThreadLocalRandom random = ThreadLocalRandom.current();
>
> char[] array = new char[length];
> for (int i = 0; i < length; i++) {
> array[i] = chars[random.nextInt(chars.length)];
> }
>
> return new String(array);
> }
> }
> Which gives
>
> (latin) (length) original patchedUnits
> insert true 824.2 ± 0.1 22.2 ± 0.0ns/op
> insert true6453.8 ± 0.2 36.1 ± 0.1ns/op
> insert true 12880.9 ± 0.2 44.6 ± 0.0ns/op
> insert true 1024 365.4 ± 0.5 109.8 ± 3.9ns/op
>
> insertfalse 833.5 ± 0.5 32.3 ± 0.2ns/op
> insertfalse6473.2 ± 0.3 73.2 ± 0.2ns/op
> insertfalse 128 103.9 ± 0.6 103.3 ± 0.1ns/op
> insertfalse 1024 576.5 ± 4.8 569.5 ± 2.0ns/op
> Patch is attached. As of tests tier1 and tier2 are ok.
>
> With best regards,
> Sergey Tsypanov
This pull request has now been integrated.
Changeset: 6eb25d7c
Author:Sergey Tsypanov
Committer: Claes Redestad
URL: https://git.openjdk.java.net/jdk/commit/6eb25d7c
Stats: 41 lines in 2 files changed: 28 ins; 4 del; 9 mod
8254082: AbstractStringBuilder.insert(int dstOffset, CharSequence s, int start,
int end) is missing fast-path for String
Reviewed-by: redestad
-
PR: https://git.openjdk.java.net/jdk/pull/402
Re: RFR: 8159746: (proxy) Support for default methods [v7]
On Wed, 25 Nov 2020 22:24:40 GMT, Mandy Chung wrote:
>> This proposes a new static `Proxy::invokeDefaultMethod` method to invoke
>> the given default method on the given proxy instance.
>>
>> The implementation looks up a method handle for `invokespecial` instruction
>> as if called from with the proxy class as the caller, equivalent to calling
>> `X.super::m` where `X` is a proxy interface of the proxy class and
>> `X.super::m` will resolve to the specified default method.
>>
>> The implementation will call a private static `proxyClassLookup(Lookup
>> caller)`
>> method of the proxy class to obtain its private Lookup. This private method
>> in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy
>> class
>> with full privilege access to use, or else `IllegalAccessException` will be
>> thrown.
>>
>> This patch also proposes to define a proxy class in an unnamed module to
>> a dynamic module to strengthen encapsulation such that they are only
>> unconditionally exported from a named module but not open for deep reflective
>> access. This only applies to the case if all the proxy interfaces are public
>> and in a package that is exported or open.
>>
>> One dynamic module is created for each class loader that defines proxies.
>> The change changes the dynamic module to contain another package (same
>> name as the module) that is unconditionally exported and is opened to
>> `java.base` only.
>>
>> There is no change to the package and module of the proxy class for
>> the following cases:
>>
>> - if at least one proxy interface is non-public, then the proxy class is
>> defined
>> in the package and module of the non-public interfaces
>> - if at least one proxy is in a package that is non-exported and non-open,
>> if all proxy interfaces are public, then the proxy class is defined in
>> a non-exported, non-open package of a dynamic module.
>>
>> The spec change is that a proxy class used to be defined in an unnamed
>> module, i.e. in a exported and open package, is defined in an unconditionally
>> exported but non-open package. Programs that assume it to be open
>> unconditionally
>> will be affected and cannot do deep reflection on such proxy classes.
>>
>> Peter Levart contributed an initial prototype [1] (thanks Peter). I think
>> the exceptions could be simplified as more checking should be done prior to
>> the invocation of the method handle like checking the types of the arguments
>> with the method type. This approach avoids defining a public API
>> `protected Proxy::$$proxyClassLookup$$` method. Instead it defines a
>> private static method that is restricted for Proxy class to use (by
>> taking a caller parameter to ensure it's a private lookup on Proxy class).
>>
>> javadoc/specdiff:
>> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/
>> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/
>>
>> [1]
>> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html
>
> Mandy Chung 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 25 additional
> commits since the last revision:
>
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> proxy-default-method
> - update copyright header
> - clean up DefaultMethodProxy test
> - Performance improvement contributed by plevart
> - Clean up the patch. Rename to
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> proxy-default-method
> - move invokeDefaultMethod to InvocationHandler and throw IAE if access
> check fails
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> proxy-default-method
> - Merge branch 'master' of https://github.com/openjdk/jdk into
> proxy-default-method
> - minor tweak to the spec wording and impl
> - ... and 15 more:
> https://git.openjdk.java.net/jdk/compare/f4321c25...d72d627f
Overall, really good!
src/java.base/share/classes/java/lang/reflect/Proxy.java line 1281:
> 1279: * @return a lookup for proxy class of this proxy instance
> 1280: */
> 1281: private static MethodHandles.Lookup
> proxyClassLookup(MethodHandles.Lookup caller, Class proxyClass) {
The method description could be a bit clearer. It invokes the proxy's
proxyClassLookup method to get a Lookup on the proxy class ("this proxy
instance" is just a bit confusing as proxyClassLookup is static).
I guess the caller parameter isn't really needed as it could be crated in
proxyClassLookup.
-
Marked as reviewed by alanb (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/313
Re: RFR: 8251989: Hex formatting and parsing utility [v10]
Hi Roger,
Thanks for your thought and I agree with you. Since this is a utility
primarily meant for developers, not end users, limiting the "hexadecimal
string/character" in Latin-1 seems reasonable.
Naoto
On 11/30/20 7:42 AM, Roger Riggs wrote:
Hi Naoto,
There are a couple of ways consistency can be achieved (and with what).
The existing hex conversions from strings to hex all delegate to
Character.digit(ch, radix) which allows
both digits and letters beyond Latin1. (See Integer.valueOf(string,
radix), Long.valueOf(string, radix), etc.)
For conversions from primitive to string they support conversion to the
Latin1 characters "0-9", "a-f".
Making the conversion of strings to and from primitives consistent
within HexFormat seems attractive
but would diverge from existing conversions and typically the non-Latin1
digits and letters almost never appear.
There are uses cases (primarily in protocols and RFCs) where the
hexadecimal characters are
specifed as "0-9", "a-f", and "A-F". If HexFormat used
Character.digit(string, radix) it would fail
to detect unexpected or illegal characters and render HexFormat
unusable for those use cases.
Though it would diverge from consistency with existing parsing of
hexadecimal in Character, Integer, Long, etc,
I'll post an update to use the string parsing allowing only Latin1
hexadecimal characters.
Comments?
Thanks, Roger
On 11/27/20 5:43 PM, Naoto Sato wrote:
On Fri, 27 Nov 2020 16:57:07 GMT, Roger Riggs wrote:
src/java.base/share/classes/java/util/HexFormat.java line 853:
851: */
852: public int fromHexDigit(int ch) {
853: int value = Character.digit(ch, 16);
Do we need to limit parsing the hex digit for only [0-9a-fA-F]? This
would return `0` for other digits, say `fullwidth digit zero` (U+FF10)
The normal and conventional characters for hex encoding are limited
to the ASCII/Latin1 range.
I don't know of any use case that would take advantage of non-ASCII
characters.
My point is that probably we should define `hexadecimal string` more
clearly. In the class description, that exclusively means [0-9a-fA-F]
in the context of formatting, but in the parsing, it allows non-ASCII
digits. e.g.,
HexFormat.of().parseHex("\uff10\uff11")
Succeeds. I would like consistency here.
-
PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8159746: (proxy) Support for default methods [v7]
On Wed, 25 Nov 2020 22:24:40 GMT, Mandy Chung wrote: >> This proposes a new static `Proxy::invokeDefaultMethod` method to invoke >> the given default method on the given proxy instance. >> >> The implementation looks up a method handle for `invokespecial` instruction >> as if called from with the proxy class as the caller, equivalent to calling >> `X.super::m` where `X` is a proxy interface of the proxy class and >> `X.super::m` will resolve to the specified default method. >> >> The implementation will call a private static `proxyClassLookup(Lookup >> caller)` >> method of the proxy class to obtain its private Lookup. This private method >> in the proxy class only allows a caller Lookup on java.lang.reflect.Proxy >> class >> with full privilege access to use, or else `IllegalAccessException` will be >> thrown. >> >> This patch also proposes to define a proxy class in an unnamed module to >> a dynamic module to strengthen encapsulation such that they are only >> unconditionally exported from a named module but not open for deep reflective >> access. This only applies to the case if all the proxy interfaces are public >> and in a package that is exported or open. >> >> One dynamic module is created for each class loader that defines proxies. >> The change changes the dynamic module to contain another package (same >> name as the module) that is unconditionally exported and is opened to >> `java.base` only. >> >> There is no change to the package and module of the proxy class for >> the following cases: >> >> - if at least one proxy interface is non-public, then the proxy class is >> defined >> in the package and module of the non-public interfaces >> - if at least one proxy is in a package that is non-exported and non-open, >> if all proxy interfaces are public, then the proxy class is defined in >> a non-exported, non-open package of a dynamic module. >> >> The spec change is that a proxy class used to be defined in an unnamed >> module, i.e. in a exported and open package, is defined in an unconditionally >> exported but non-open package. Programs that assume it to be open >> unconditionally >> will be affected and cannot do deep reflection on such proxy classes. >> >> Peter Levart contributed an initial prototype [1] (thanks Peter). I think >> the exceptions could be simplified as more checking should be done prior to >> the invocation of the method handle like checking the types of the arguments >> with the method type. This approach avoids defining a public API >> `protected Proxy::$$proxyClassLookup$$` method. Instead it defines a >> private static method that is restricted for Proxy class to use (by >> taking a caller parameter to ensure it's a private lookup on Proxy class). >> >> javadoc/specdiff: >> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/api/ >> http://cr.openjdk.java.net/~mchung/jdk16/webrevs/8159746/specdiff/ >> >> [1] >> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-June/041629.html > > Mandy Chung 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 25 additional > commits since the last revision: > > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - update copyright header > - clean up DefaultMethodProxy test > - Performance improvement contributed by plevart > - Clean up the patch. Rename to > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - move invokeDefaultMethod to InvocationHandler and throw IAE if access > check fails > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - Merge branch 'master' of https://github.com/openjdk/jdk into > proxy-default-method > - minor tweak to the spec wording and impl > - ... and 15 more: > https://git.openjdk.java.net/jdk/compare/5fcea495...d72d627f src/java.base/share/classes/java/lang/reflect/Proxy.java line 1227: > 1225: if (proxyInterfaces.contains(declaringClass)) > 1226: return declaringClass; > 1227: It might be beneficial for future maintainers if you could include a comment here on the search order. - PR: https://git.openjdk.java.net/jdk/pull/313
Integrated: 8256541: Sort out what version of awk is used in the build system
On Thu, 26 Nov 2020 20:58:38 GMT, Magnus Ihse Bursie wrote: > For historical reasons, there exists a variety of different implementations > of awk: awk (the original implementation), gawk (the GNU version), nawk (new > awk, iirc) and the lesser known mawk. > > Things are complicated by the fact that the original awk is seldom used, but > instead gawk or nawk is typically symlinked to be named "awk". > > In terms of functionality there are very few differences. The original awk is > most limited, while nawk and gawk is mostly replaceable. > > So the conditions for this is somewhat messy, but we manage impressively to > mess it up even further. :-) > > We set up the following definitions: > `BASIC_REQUIRE_PROGS(NAWK, [nawk gawk awk])` > `BASIC_REQUIRE_SPECIAL(AWK, [AC_PROG_AWK])` > and `AC_PROG_AWK`, according to the documentation, "[c]heck for gawk, mawk, > nawk, and awk, in that order". > > So, if you have nawk and awk (but no other) installed, both NAWK and AWK will > be set to nawk. If you have only awk, both will be set to awk. The difference > is if you have gawk installed, then NAWK will be nawk and AWK will be gawk. > > As an example, on my mac, I only have the original awk, so both AWK and NAWK > will be awk. > > On my ubuntu box, things are even more confused. I have: > $ ls -l /usr/bin/*awk > lrwxrwxrwx 1 root root 21 Feb 6 10:36 awk -> /etc/alternatives/awk* > -rwxr-xr-x 1 root root 658072 Feb 11 2018 gawk* > -rwxr-xr-x 1 root root 3189 Feb 11 2018 igawk* > -rwxr-xr-x 1 root root 125416 Apr 3 2018 mawk* > lrwxrwxrwx 1 root root 22 Feb 6 10:37 nawk -> /etc/alternatives/nawk* > > $ ls -l /etc/alternatives/*awk > lrwxrwxrwx 1 root root 13 Feb 10 10:56 /etc/alternatives/awk -> > /usr/bin/gawk* > lrwxrwxrwx 1 root root 13 Feb 10 10:56 /etc/alternatives/nawk -> > /usr/bin/gawk* > > So awk, nawk and gawk all executes the same binary, i.e. gawk. Only mawk is > different. So on that machine, AWK would be gawk and NAWK would be nawk, but > both will execute gawk. > > I propose that we remove NAWK, and only use AWK, but we should stop using > AC_PROG_AWK and define it in an order that is transparent to us. I recommend > [gawk nawk awk], since on Linux systems nawk (as we've seen) is likely to be > gawk under disguise anyway, so it's better to be clear about that. > > This reasoning assumes that the awk scripts we write are portable enough to > be executed by any awk. If we run into any problem with this, we might have > to restrict the variation of awks we support. > > To make this work properly, I also needed to get rid of the awk launched by > fixpath in CompileCommand. (This only worked. since AWK was not evolved to a > full path by `AC_PROG_AWK`, but was only `awk`(or whatever). Otherwise this > could not work with fixpath, so it was very much a hack to begin with... This pull request has now been integrated. Changeset: a3e1980c Author:Magnus Ihse Bursie URL: https://git.openjdk.java.net/jdk/commit/a3e1980c Stats: 35 lines in 15 files changed: 4 ins; 9 del; 22 mod 8256541: Sort out what version of awk is used in the build system Reviewed-by: erikj - PR: https://git.openjdk.java.net/jdk/pull/1470
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
> This pull request replaces https://github.com/openjdk/jdk/pull/1227. > > From the original PR: > >> Please review the code for the second iteration of sealed classes. In this >> iteration we are: >> >> * Enhancing narrowing reference conversion to allow for stricter >> checking of cast conversions with respect to sealed type hierarchies >> >> * Also local classes are not considered when determining implicitly >> declared permitted direct subclasses of a sealed class or sealed interface >> >> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses, >> still in the same method, the return type has been changed to Class[] >> instead of the previous ClassDesc[] >> >> * adding code to make sure that annotations can't be sealed >> >> * improving some tests >> >> >> TIA >> >> Related specs: >> [Sealed Classes >> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html) >> [Sealed Classes >> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html) >> [Additional: Contextual >> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html) > > This PR strives to reflect the review comments from 1227: > * adjustments to javadoc of j.l.Class methods > * package access checks in Class.getPermittedSubclasses() > * fixed to the narrowing conversion/castability as pointed out by Maurizio 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: - Improving getPermittedSubclasses javadoc. - Merge branch 'master' into JDK-8246778 - Moving checkPackageAccess from getPermittedSubclasses to a separate method. - Improving getPermittedSubclasses() javadoc. - Enhancing the Class.getPermittedSubclasses() test to verify behavior both for sealed classes in named and unnamed modules. - Removing unnecessary file. - Tweaking javadoc. - Reflecting review comments w.r.t. narrowing conversion. - Improving checks in getPermittedSubclasses() - Merging master into JDK-8246778 - ... and 2 more: https://git.openjdk.java.net/jdk/compare/6e006223...4d484179 - Changes: https://git.openjdk.java.net/jdk/pull/1483/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1483&range=01 Stats: 918 lines in 12 files changed: 837 ins; 9 del; 72 mod Patch: https://git.openjdk.java.net/jdk/pull/1483.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1483/head:pull/1483 PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview) [v2]
On Mon, 30 Nov 2020 09:55:56 GMT, Alan Bateman wrote:
>> 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:
>>
>> - Improving getPermittedSubclasses javadoc.
>> - Merge branch 'master' into JDK-8246778
>> - Moving checkPackageAccess from getPermittedSubclasses to a separate
>> method.
>> - Improving getPermittedSubclasses() javadoc.
>> - Enhancing the Class.getPermittedSubclasses() test to verify behavior both
>> for sealed classes in named and unnamed modules.
>> - Removing unnecessary file.
>> - Tweaking javadoc.
>> - Reflecting review comments w.r.t. narrowing conversion.
>> - Improving checks in getPermittedSubclasses()
>> - Merging master into JDK-8246778
>> - ... and 2 more:
>> https://git.openjdk.java.net/jdk/compare/6e006223...4d484179
>
> src/java.base/share/classes/java/lang/Class.java line 4420:
>
>> 4418: * {@linkplain #getClassLoader() the defining class loader} of the
>> current
>> 4419: * {@code Class} object. If a name cannot be converted to the
>> {@code Class}
>> 4420: * instance, it is silently excluded from the result.
>
> I think this paragraph will need a little bit of wordsmithing. The 3rd
> paragraph of getNestMembers might be useful to examine as it more clearly
> describes how the method attempts to "obtain" the Class object for each of
> the class names in the NestMembers attribute and maybe some of that wording
> could be used instead of using the term "convert".
>
> Minor nit but the prevailing style for the @throws SecurityException is to
> align the description with the exception, probably best to keep it consistent
> if you can.
Thanks, I've tried to improve the javadoc here:
https://github.com/openjdk/jdk/pull/1483/commits/4d484179e6e4d64ed460b997d25b4dca5d964016
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: RFR: 8251989: Hex formatting and parsing utility [v10]
Hi Naoto,
There are a couple of ways consistency can be achieved (and with what).
The existing hex conversions from strings to hex all delegate to
Character.digit(ch, radix) which allows
both digits and letters beyond Latin1. (See Integer.valueOf(string,
radix), Long.valueOf(string, radix), etc.)
For conversions from primitive to string they support conversion to the
Latin1 characters "0-9", "a-f".
Making the conversion of strings to and from primitives consistent
within HexFormat seems attractive
but would diverge from existing conversions and typically the non-Latin1
digits and letters almost never appear.
There are uses cases (primarily in protocols and RFCs) where the
hexadecimal characters are
specifed as "0-9", "a-f", and "A-F". If HexFormat used
Character.digit(string, radix) it would fail
to detect unexpected or illegal characters and render HexFormat
unusable for those use cases.
Though it would diverge from consistency with existing parsing of
hexadecimal in Character, Integer, Long, etc,
I'll post an update to use the string parsing allowing only Latin1
hexadecimal characters.
Comments?
Thanks, Roger
On 11/27/20 5:43 PM, Naoto Sato wrote:
On Fri, 27 Nov 2020 16:57:07 GMT, Roger Riggs wrote:
src/java.base/share/classes/java/util/HexFormat.java line 853:
851: */
852: public int fromHexDigit(int ch) {
853: int value = Character.digit(ch, 16);
Do we need to limit parsing the hex digit for only [0-9a-fA-F]? This would
return `0` for other digits, say `fullwidth digit zero` (U+FF10)
The normal and conventional characters for hex encoding are limited to the
ASCII/Latin1 range.
I don't know of any use case that would take advantage of non-ASCII characters.
My point is that probably we should define `hexadecimal string` more clearly.
In the class description, that exclusively means [0-9a-fA-F] in the context of
formatting, but in the parsing, it allows non-ASCII digits. e.g.,
HexFormat.of().parseHex("\uff10\uff11")
Succeeds. I would like consistency here.
-
PR: https://git.openjdk.java.net/jdk/pull/482
Re: RFR: 8256541: Sort out what version of awk is used in the build system
On Thu, 26 Nov 2020 20:58:38 GMT, Magnus Ihse Bursie wrote: > For historical reasons, there exists a variety of different implementations > of awk: awk (the original implementation), gawk (the GNU version), nawk (new > awk, iirc) and the lesser known mawk. > > Things are complicated by the fact that the original awk is seldom used, but > instead gawk or nawk is typically symlinked to be named "awk". > > In terms of functionality there are very few differences. The original awk is > most limited, while nawk and gawk is mostly replaceable. > > So the conditions for this is somewhat messy, but we manage impressively to > mess it up even further. :-) > > We set up the following definitions: > `BASIC_REQUIRE_PROGS(NAWK, [nawk gawk awk])` > `BASIC_REQUIRE_SPECIAL(AWK, [AC_PROG_AWK])` > and `AC_PROG_AWK`, according to the documentation, "[c]heck for gawk, mawk, > nawk, and awk, in that order". > > So, if you have nawk and awk (but no other) installed, both NAWK and AWK will > be set to nawk. If you have only awk, both will be set to awk. The difference > is if you have gawk installed, then NAWK will be nawk and AWK will be gawk. > > As an example, on my mac, I only have the original awk, so both AWK and NAWK > will be awk. > > On my ubuntu box, things are even more confused. I have: > $ ls -l /usr/bin/*awk > lrwxrwxrwx 1 root root 21 Feb 6 10:36 awk -> /etc/alternatives/awk* > -rwxr-xr-x 1 root root 658072 Feb 11 2018 gawk* > -rwxr-xr-x 1 root root 3189 Feb 11 2018 igawk* > -rwxr-xr-x 1 root root 125416 Apr 3 2018 mawk* > lrwxrwxrwx 1 root root 22 Feb 6 10:37 nawk -> /etc/alternatives/nawk* > > $ ls -l /etc/alternatives/*awk > lrwxrwxrwx 1 root root 13 Feb 10 10:56 /etc/alternatives/awk -> > /usr/bin/gawk* > lrwxrwxrwx 1 root root 13 Feb 10 10:56 /etc/alternatives/nawk -> > /usr/bin/gawk* > > So awk, nawk and gawk all executes the same binary, i.e. gawk. Only mawk is > different. So on that machine, AWK would be gawk and NAWK would be nawk, but > both will execute gawk. > > I propose that we remove NAWK, and only use AWK, but we should stop using > AC_PROG_AWK and define it in an order that is transparent to us. I recommend > [gawk nawk awk], since on Linux systems nawk (as we've seen) is likely to be > gawk under disguise anyway, so it's better to be clear about that. > > This reasoning assumes that the awk scripts we write are portable enough to > be executed by any awk. If we run into any problem with this, we might have > to restrict the variation of awks we support. > > To make this work properly, I also needed to get rid of the awk launched by > fixpath in CompileCommand. (This only worked. since AWK was not evolved to a > full path by `AC_PROG_AWK`, but was only `awk`(or whatever). Otherwise this > could not work with fixpath, so it was very much a hack to begin with... Marked as reviewed by erikj (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/1470
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Mon, 30 Nov 2020 13:52:17 GMT, Pavel Rappo wrote:
>> @pavelrappo Please see my updated CSR below. Thanks.
>>
>> # Map::compute should have the implementation requirement match its default
>> implementation
>>
>> ## Summary
>>
>> The implementation requirement of Map::compute does not match its default
>> implementation. Besides, it has some other minor issues. We should fix it.
>>
>> ## Problem
>>
>> The documentation of the implementation requirements for Map::compute has
>> the following problems:
>> 1. It doesn't match its default implementation.
>> 1. It lacks of the return statements for most of the if-else cases.
>> 1. The indents are 3 spaces, while the convention is 4 spaces.
>> 1. The if-else is overly complicated and can be simplified.
>> 1. The surrounding prose contains incorrect statements.
>>
>> ## Solution
>>
>> Rewrite the documentation of Map::compute to match its default
>> implementation and solve the above mentioned problems.
>>
>> ## Specification
>>
>> diff --git a/src/java.base/share/classes/java/util/Map.java
>> b/src/java.base/share/classes/java/util/Map.java
>> index b1de34b42a5..b30e3979259 100644
>> --- a/src/java.base/share/classes/java/util/Map.java
>> +++ b/src/java.base/share/classes/java/util/Map.java
>> @@ -1107,23 +1107,17 @@ public interface Map {
>> *
>> * @implSpec
>> * The default implementation is equivalent to performing the following
>> - * steps for this {@code map}, then returning the current value or
>> - * {@code null} if absent:
>> + * steps for this {@code map}:
>> *
>> * {@code
>> * V oldValue = map.get(key);
>> * V newValue = remappingFunction.apply(key, oldValue);
>> - * if (oldValue != null) {
>> - *if (newValue != null)
>> - * map.put(key, newValue);
>> - *else
>> - * map.remove(key);
>> - * } else {
>> - *if (newValue != null)
>> - * map.put(key, newValue);
>> - *else
>> - * return null;
>> + * if (newValue != null) {
>> + * map.put(key, newValue);
>> + * } else if (oldValue != null || map.containsKey(key)) {
>> + * map.remove(key);
>> * }
>> + * return newValue;
>> * }
>> *
>> * The default implementation makes no guarantees about detecting if
>> the
>
> @johnlinp, thanks for updating the CSR draft; it is much better now.
>
> @stuart-marks, I think we could further improve this snippet. This `if`
> statement seems to use an optimization:
>
> if (oldValue != null || map.containsKey(key))
>
> I don't think we should include an optimization into the specification unless
> that optimization also improves readability. Is this the case here? Could
> this be better?
>
> if (map.containsKey(key))
I would even go as far as to rewrite that snippet like this:
if (newValue == null) {
remove(key);
} else {
put(key, newValue);
}
return newValue;
This rewrite is possible thanks to the following properties of
`Map.remove(Object key)`:
1. A call with an unmapped `key` has no effect.
2. A call with a mapped `key` has the same semantics regardless of the value
that this key is mapped to.
In particular, (2) covers `null` values.
To me, this rewrite reads better; however, I understand that readability is
subjective and that snippets used in `@implSpec` might be subject to additional
requirements.
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v3]
> Adding support for record classes in the historical data for ct.sym. This > includes a few changes not strictly needed for the change: > -updating and moving tests into test/langtools, so that it is easier to run > them. > -fixing Record attribute reading in javac's ClassReader (used for tests, but > seems like the proper thing to do anyway). > -fixing the -Xprint annotation processor to print record component > annotations. > > Changes to jdk.jdeps' classfile library are needed so that the ct.sym > creation works. Jan Lahoda has updated the pull request incrementally with one additional commit since the last revision: Fixing tests on Windows - normalizing line endings. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1480/files - new: https://git.openjdk.java.net/jdk/pull/1480/files/7ce38709..a3f79aba Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1480&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1480&range=01-02 Stats: 6 lines in 2 files changed: 3 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1480/head:pull/1480 PR: https://git.openjdk.java.net/jdk/pull/1480
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v3]
On Mon, 30 Nov 2020 12:17:46 GMT, Chris Hegarty wrote:
>> Jan Lahoda has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fixing tests on Windows - normalizing line endings.
>
> make/langtools/src/classes/build/tools/symbolgenerator/CreateSymbols.java
> line 965:
>
>> 963:new NestMembers_attribute(attributeString,
>> nestMembers));
>> 964: }
>> 965: if (header.recordComponents != null &&
>> !header.recordComponents.isEmpty()) {
>
> I am not sure of the exact logic here, but it is perfectly fine for a record
> attribute to contain zero components, and for the class to still be
> considered a "record class". But maybe that is not all that significant here?
> I just want to call it out so that it is considered.
Ah, right - fixed in:
https://github.com/openjdk/jdk/pull/1480/commits/e1ec2b7ff49e1307a348ad58b12e5ed39ebe7224
Thanks for the comment!
-
PR: https://git.openjdk.java.net/jdk/pull/1480
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols [v2]
> Adding support for record classes in the historical data for ct.sym. This > includes a few changes not strictly needed for the change: > -updating and moving tests into test/langtools, so that it is easier to run > them. > -fixing Record attribute reading in javac's ClassReader (used for tests, but > seems like the proper thing to do anyway). > -fixing the -Xprint annotation processor to print record component > annotations. > > Changes to jdk.jdeps' classfile library are needed so that the ct.sym > creation works. Jan Lahoda has updated the pull request incrementally with two additional commits since the last revision: - Merge branch 'JDK-8256950' of https://github.com/lahodaj/jdk into JDK-8256950 - CreateSymbols should support records with no components. - Changes: - all: https://git.openjdk.java.net/jdk/pull/1480/files - new: https://git.openjdk.java.net/jdk/pull/1480/files/c3696e72..7ce38709 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1480&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1480&range=00-01 Stats: 54 lines in 2 files changed: 51 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/1480.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1480/head:pull/1480 PR: https://git.openjdk.java.net/jdk/pull/1480
Re: RFR: 8247402: Documentation for Map::compute contains confusing implementation requirements
On Sat, 28 Nov 2020 08:42:52 GMT, John Lin
wrote:
>> @johnlinp In any case, you'd also need to update the surrounding prose; we
>> need to remove this:
>> returning the current value or {@code null} if absent:```
>
> @pavelrappo Please see my updated CSR below. Thanks.
>
> # Map::compute should have the implementation requirement match its default
> implementation
>
> ## Summary
>
> The implementation requirement of Map::compute does not match its default
> implementation. Besides, it has some other minor issues. We should fix it.
>
> ## Problem
>
> The documentation of the implementation requirements for Map::compute has the
> following problems:
> 1. It doesn't match its default implementation.
> 1. It lacks of the return statements for most of the if-else cases.
> 1. The indents are 3 spaces, while the convention is 4 spaces.
> 1. The if-else is overly complicated and can be simplified.
> 1. The surrounding prose contains incorrect statements.
>
> ## Solution
>
> Rewrite the documentation of Map::compute to match its default implementation
> and solve the above mentioned problems.
>
> ## Specification
>
> diff --git a/src/java.base/share/classes/java/util/Map.java
> b/src/java.base/share/classes/java/util/Map.java
> index b1de34b42a5..b30e3979259 100644
> --- a/src/java.base/share/classes/java/util/Map.java
> +++ b/src/java.base/share/classes/java/util/Map.java
> @@ -1107,23 +1107,17 @@ public interface Map {
> *
> * @implSpec
> * The default implementation is equivalent to performing the following
> - * steps for this {@code map}, then returning the current value or
> - * {@code null} if absent:
> + * steps for this {@code map}:
> *
> * {@code
> * V oldValue = map.get(key);
> * V newValue = remappingFunction.apply(key, oldValue);
> - * if (oldValue != null) {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * map.remove(key);
> - * } else {
> - *if (newValue != null)
> - * map.put(key, newValue);
> - *else
> - * return null;
> + * if (newValue != null) {
> + * map.put(key, newValue);
> + * } else if (oldValue != null || map.containsKey(key)) {
> + * map.remove(key);
> * }
> + * return newValue;
> * }
> *
> * The default implementation makes no guarantees about detecting if
> the
@johnlinp, thanks for updating the CSR draft; it is much better now.
@stuart-marks, I think we could further improve this snippet. This `if`
statement seems to use an optimization:
if (oldValue != null || map.containsKey(key))
I don't think we should include an optimization into the specification unless
that optimization also improves readability. Is this the case here? Could this
be better?
if (map.containsKey(key))
-
PR: https://git.openjdk.java.net/jdk/pull/714
Re: RFR: 8257074 Update the ByteBuffers micro benchmark [v4]
> The ByteBuffers micro benchmark seems to be a little dated. > > It should be a useful resource to leverage when analysing the performance > impact of any potential implementation changes in the byte buffer classes. > More specifically, the impact of such changes on the performance of sharp > memory access operations. > > This issue proposes to update the benchmark in the following ways to meet the > aforementioned use-case: > > 1. Remove allocation from the individual benchmarks - it just creates noise. > 2. Consolidate per-thread shared heap and direct buffers. > 3. All scenarios now use absolute memory access operations - so no state of > the shared buffers is considered. > 4. Provide more reasonable default fork, warmup, etc, out-of-the-box. > 5. There seems to have been an existing bug in the test where the size > parameter was not always considered - this is now fixed. Chris Hegarty has updated the pull request incrementally with two additional commits since the last revision: - Add explicitly allocated heap carrier buffer tests - Replace Single with Loop - Changes: - all: https://git.openjdk.java.net/jdk/pull/1430/files - new: https://git.openjdk.java.net/jdk/pull/1430/files/814e1819..5ee5d8bf Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1430&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1430&range=02-03 Stats: 407 lines in 1 file changed: 151 ins; 0 del; 256 mod Patch: https://git.openjdk.java.net/jdk/pull/1430.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/1430/head:pull/1430 PR: https://git.openjdk.java.net/jdk/pull/1430
Re: Why having the wrong InnerClasses attribute is an issue for the VM ?
- Mail original - > De: "David Holmes" > À: "Remi Forax" > Cc: "core-libs-dev" > Envoyé: Lundi 30 Novembre 2020 13:44:32 > Objet: Re: Why having the wrong InnerClasses attribute is an issue for the VM > ? > On 30/11/2020 8:04 pm, fo...@univ-mlv.fr wrote: >> - Mail original - >>> De: "David Holmes" >>> À: "Remi Forax" , "core-libs-dev" >>> >>> Envoyé: Lundi 30 Novembre 2020 08:57:37 >>> Objet: Re: Why having the wrong InnerClasses attribute is an issue for the >>> VM ? >> >>> On 30/11/2020 5:08 pm, Remi Forax wrote: I've forgotten a cast in an invokedynamic, hence a call to wrongTargetType, but in order to create the error message, MethodType.toString(), getSimpleName() is called and it fails because getDeclaringClass() verifies the InnerClasses attribute. For me InnerClasses was just an attribute for javac not something the VM should take care of, it seems that the VM strongly verifies this attribute and i wonder what is the reason behind that ? >>> >>> From the VM code - InstanceKlass::compute_enclosing_class: >>> >>>// Throws an exception if outer klass has not declared k as an inner >>> klass >>>// We need evidence that each klass knows about the other, or else >>>// the system could allow a spoof of an inner class to gain access >>> rights. >>>Reflection::check_for_inner_class(outer_klass, this, >>> *inner_is_member, CHECK_NULL); >> >> I don't understand how to spoof the InnerClasses attribute to gain access >> given >> that the access rights are not based on the InnerClasses attribute. > > Good point. > > That aside though, how do you answer the is_top_level_class question if > the enclosed and enclosing classes disagree about the relationship? Good question too, only by looking to the information stored in the class that can be top_level or not, i suppose. > > David > - > Rémi >>> >>> David >> >> Rémi >> >>> regards, Rémi Exception in thread "main" java.lang.IncompatibleClassChangeError: fr.umlv.transmogrif.ImplMap and fr.umlv.transmogrif.ImplMap$Row/0x000801007400 disagree on InnerClasses attribute at java.base/java.lang.Class.getDeclaringClass0(Native Method) at java.base/java.lang.Class.isTopLevelClass(Class.java:1970) at java.base/java.lang.Class.getSimpleBinaryName(Class.java:1955) at java.base/java.lang.Class.getSimpleName0(Class.java:1835) at java.base/java.lang.Class.getSimpleName(Class.java:1826) at java.base/java.lang.Class.getSimpleName0(Class.java:1833) at java.base/java.lang.Class.getSimpleName(Class.java:1826) at java.base/java.lang.invoke.MethodType.toString(MethodType.java:895) at java.base/java.lang.String.valueOf(String.java:3365) at java.base/java.lang.StringBuilder.append(StringBuilder.java:169) at java.base/java.lang.invoke.MethodHandle.standardString(MethodHandle.java:1611) at java.base/java.lang.invoke.MethodHandle.toString(MethodHandle.java:1608) at java.base/java.lang.String.valueOf(String.java:3365) at java.base/java.lang.invoke.CallSite.wrongTargetType(CallSite.java:203) at java.base/java.lang.invoke.CallSite.makeSite(CallSite.java:333) at java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:280) at java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:270) at fr.umlv.transmogrif.ImplMap/0x000801003c00.(ImplMap.java:21) > >>> at fr.umlv.transmogrif.Main.main(Main.java:7)
Re: Why having the wrong InnerClasses attribute is an issue for the VM ?
On 30/11/2020 8:04 pm, fo...@univ-mlv.fr wrote: - Mail original - De: "David Holmes" À: "Remi Forax" , "core-libs-dev" Envoyé: Lundi 30 Novembre 2020 08:57:37 Objet: Re: Why having the wrong InnerClasses attribute is an issue for the VM ? On 30/11/2020 5:08 pm, Remi Forax wrote: I've forgotten a cast in an invokedynamic, hence a call to wrongTargetType, but in order to create the error message, MethodType.toString(), getSimpleName() is called and it fails because getDeclaringClass() verifies the InnerClasses attribute. For me InnerClasses was just an attribute for javac not something the VM should take care of, it seems that the VM strongly verifies this attribute and i wonder what is the reason behind that ? From the VM code - InstanceKlass::compute_enclosing_class: // Throws an exception if outer klass has not declared k as an inner klass // We need evidence that each klass knows about the other, or else // the system could allow a spoof of an inner class to gain access rights. Reflection::check_for_inner_class(outer_klass, this, *inner_is_member, CHECK_NULL); I don't understand how to spoof the InnerClasses attribute to gain access given that the access rights are not based on the InnerClasses attribute. Good point. That aside though, how do you answer the is_top_level_class question if the enclosed and enclosing classes disagree about the relationship? David - David Rémi regards, Rémi Exception in thread "main" java.lang.IncompatibleClassChangeError: fr.umlv.transmogrif.ImplMap and fr.umlv.transmogrif.ImplMap$Row/0x000801007400 disagree on InnerClasses attribute at java.base/java.lang.Class.getDeclaringClass0(Native Method) at java.base/java.lang.Class.isTopLevelClass(Class.java:1970) at java.base/java.lang.Class.getSimpleBinaryName(Class.java:1955) at java.base/java.lang.Class.getSimpleName0(Class.java:1835) at java.base/java.lang.Class.getSimpleName(Class.java:1826) at java.base/java.lang.Class.getSimpleName0(Class.java:1833) at java.base/java.lang.Class.getSimpleName(Class.java:1826) at java.base/java.lang.invoke.MethodType.toString(MethodType.java:895) at java.base/java.lang.String.valueOf(String.java:3365) at java.base/java.lang.StringBuilder.append(StringBuilder.java:169) at java.base/java.lang.invoke.MethodHandle.standardString(MethodHandle.java:1611) at java.base/java.lang.invoke.MethodHandle.toString(MethodHandle.java:1608) at java.base/java.lang.String.valueOf(String.java:3365) at java.base/java.lang.invoke.CallSite.wrongTargetType(CallSite.java:203) at java.base/java.lang.invoke.CallSite.makeSite(CallSite.java:333) at java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:280) at java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:270) at fr.umlv.transmogrif.ImplMap/0x000801003c00.(ImplMap.java:21) at fr.umlv.transmogrif.Main.main(Main.java:7)
Re: RFR: JDK-8256950: Add record attribute support to symbol generator CreateSymbols
On Fri, 27 Nov 2020 13:21:15 GMT, Jan Lahoda wrote:
> Adding support for record classes in the historical data for ct.sym. This
> includes a few changes not strictly needed for the change:
> -updating and moving tests into test/langtools, so that it is easier to run
> them.
> -fixing Record attribute reading in javac's ClassReader (used for tests, but
> seems like the proper thing to do anyway).
> -fixing the -Xprint annotation processor to print record component
> annotations.
>
> Changes to jdk.jdeps' classfile library are needed so that the ct.sym
> creation works.
make/langtools/src/classes/build/tools/symbolgenerator/CreateSymbols.java line
965:
> 963:new NestMembers_attribute(attributeString,
> nestMembers));
> 964: }
> 965: if (header.recordComponents != null &&
> !header.recordComponents.isEmpty()) {
I am not sure of the exact logic here, but it is perfectly fine for a record
attribute to contain zero components, and for the class to still be considered
a "record class". But maybe that is not all that significant here? I just want
to call it out so that it is considered.
-
PR: https://git.openjdk.java.net/jdk/pull/1480
Re: Why having the wrong InnerClasses attribute is an issue for the VM ?
- Mail original - > De: "David Holmes" > À: "Remi Forax" , "core-libs-dev" > > Envoyé: Lundi 30 Novembre 2020 08:57:37 > Objet: Re: Why having the wrong InnerClasses attribute is an issue for the VM > ? > On 30/11/2020 5:08 pm, Remi Forax wrote: >> I've forgotten a cast in an invokedynamic, hence a call to wrongTargetType, >> but in order to create the error message, MethodType.toString(), >> getSimpleName() >> is called and it fails because getDeclaringClass() verifies the InnerClasses >> attribute. >> >> For me InnerClasses was just an attribute for javac not something the VM >> should >> take care of, >> it seems that the VM strongly verifies this attribute and i wonder what is >> the >> reason behind that ? > > From the VM code - InstanceKlass::compute_enclosing_class: > > // Throws an exception if outer klass has not declared k as an inner > klass > // We need evidence that each klass knows about the other, or else > // the system could allow a spoof of an inner class to gain access > rights. > Reflection::check_for_inner_class(outer_klass, this, > *inner_is_member, CHECK_NULL); I don't understand how to spoof the InnerClasses attribute to gain access given that the access rights are not based on the InnerClasses attribute. > > David Rémi > >> regards, >> Rémi >> >> Exception in thread "main" java.lang.IncompatibleClassChangeError: >> fr.umlv.transmogrif.ImplMap and >> fr.umlv.transmogrif.ImplMap$Row/0x000801007400 disagree on InnerClasses >> attribute >> at java.base/java.lang.Class.getDeclaringClass0(Native Method) >> at java.base/java.lang.Class.isTopLevelClass(Class.java:1970) >> at java.base/java.lang.Class.getSimpleBinaryName(Class.java:1955) >> at java.base/java.lang.Class.getSimpleName0(Class.java:1835) >> at java.base/java.lang.Class.getSimpleName(Class.java:1826) >> at java.base/java.lang.Class.getSimpleName0(Class.java:1833) >> at java.base/java.lang.Class.getSimpleName(Class.java:1826) >> at >> java.base/java.lang.invoke.MethodType.toString(MethodType.java:895) >> at java.base/java.lang.String.valueOf(String.java:3365) >> at java.base/java.lang.StringBuilder.append(StringBuilder.java:169) >> at >> >> java.base/java.lang.invoke.MethodHandle.standardString(MethodHandle.java:1611) >> at >> java.base/java.lang.invoke.MethodHandle.toString(MethodHandle.java:1608) >> at java.base/java.lang.String.valueOf(String.java:3365) >> at >> java.base/java.lang.invoke.CallSite.wrongTargetType(CallSite.java:203) >> at java.base/java.lang.invoke.CallSite.makeSite(CallSite.java:333) >> at >> >> java.base/java.lang.invoke.MethodHandleNatives.linkCallSiteImpl(MethodHandleNatives.java:280) >> at >> >> java.base/java.lang.invoke.MethodHandleNatives.linkCallSite(MethodHandleNatives.java:270) >> at >> fr.umlv.transmogrif.ImplMap/0x000801003c00.(ImplMap.java:21) >> at fr.umlv.transmogrif.Main.main(Main.java:7)
Re: RFR: 8246778: Compiler implementation for Sealed Classes (Second Preview)
On Fri, 27 Nov 2020 16:57:54 GMT, Jan Lahoda wrote:
> This pull request replaces https://github.com/openjdk/jdk/pull/1227.
>
> From the original PR:
>
>> Please review the code for the second iteration of sealed classes. In this
>> iteration we are:
>>
>> * Enhancing narrowing reference conversion to allow for stricter
>> checking of cast conversions with respect to sealed type hierarchies
>>
>> * Also local classes are not considered when determining implicitly
>> declared permitted direct subclasses of a sealed class or sealed interface
>>
>> * renaming Class::permittedSubclasses to Class::getPermittedSubclasses,
>> still in the same method, the return type has been changed to Class[]
>> instead of the previous ClassDesc[]
>>
>> * adding code to make sure that annotations can't be sealed
>>
>> * improving some tests
>>
>>
>> TIA
>>
>> Related specs:
>> [Sealed Classes
>> JSL](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jls.html)
>> [Sealed Classes
>> JVMS](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/sealed-classes-jvms.html)
>> [Additional: Contextual
>> Keywords](http://cr.openjdk.java.net/~gbierman/jep397/jep397-20201104/specs/contextual-keywords-jls.html)
>
> This PR strives to reflect the review comments from 1227:
> * adjustments to javadoc of j.l.Class methods
> * package access checks in Class.getPermittedSubclasses()
> * fixed to the narrowing conversion/castability as pointed out by Maurizio
src/java.base/share/classes/java/lang/Class.java line 4420:
> 4418: * {@linkplain #getClassLoader() the defining class loader} of the
> current
> 4419: * {@code Class} object. If a name cannot be converted to the
> {@code Class}
> 4420: * instance, it is silently excluded from the result.
I think this paragraph will need a little bit of wordsmithing. The 3rd
paragraph of getNestMembers might be useful to examine as it more clearly
describes how the method attempts to "obtain" the Class object for each of the
class names in the NestMembers attribute and maybe some of that wording could
be used instead of using the term "convert".
Minor nit but the prevailing style for the @throws SecurityException is to
align the description with the exception, probably best to keep it consistent
if you can.
-
PR: https://git.openjdk.java.net/jdk/pull/1483
Re: "loc: wrong sig" in ZipFileSystem on a valid ZIP file (extra data field of exactly 5 bytes)
> The ZipFileSystem check is case sensitive: > Ah, that's not obvious at all... and even if this is an undocumented option I'd add a comment in the test or remove it to Map.of() for clarity. That bug entry in Jira doesn't have "affects" pointing at older releases - I think it should since it affects a number of them... I'm sorry for not tracking head-of-development closely, perhaps I'd have caught your patch sooner; I only tested the problem with binary releases. Dawid
