Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]
On Thu, 26 May 2022 23:05:32 GMT, Joe Darcy wrote: >> Time to start getting ready for JDK 20... > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by dholmes (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
On Fri, 27 May 2022 05:31:28 GMT, Stuart Marks wrote: >> XenoAmess has updated the pull request with a new target base due to a merge >> or a rebase. The pull request now contains 16 commits: >> >> - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780 >> - Merge remote-tracking branch 'openjdk/master' into fix_8284780 >> >># Conflicts: >># test/jdk/java/util/HashMap/WhiteBoxResizeTest.java >> - add 8284780 to test >> - redo the tests >> - rename items to elements >> - add test for newHashSet and newLinkedHashSet >> - revert much too changes for newHashSet >> - add more replaces >> - add more replaces >> - add more replaces >> - ... and 6 more: >> https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4 > > Thanks for the updates. I've made a couple minor changes to the specs; please > merge the latest commit from this branch: > > https://github.com/stuart-marks/jdk/tree/pull/8302 > > I've created a CSR and have included these changes in it. Please review: > [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) > > I'll look at the test changes later. I wanted to get the CSR moving first, > since it will take a few days (and a long weekend in the US is coming up). @stuart-marks @liach done. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v16]
> as title. XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - Merge remote-tracking branch 'stuart-marks/pull/8302' into fix_8284780 # Conflicts: #src/java.base/share/classes/java/util/HashSet.java #src/java.base/share/classes/java/util/LinkedHashSet.java - Minor terminology fixes: change "item" and "key" to element; remove "insertion-ordered" from LinkedHashSet static factory method because LHS is implicit insertion-ordered. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/42fba932..230a767e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=14-15 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v15]
> as title. XenoAmess has updated the pull request incrementally with six additional commits since the last revision: - Update src/java.base/share/classes/java/util/LinkedHashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/java/util/LinkedHashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/java/util/LinkedHashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/java/util/HashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/java/util/HashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Update src/java.base/share/classes/java/util/HashSet.java Co-authored-by: liach <7806504+li...@users.noreply.github.com> - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/117918f4..42fba932 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=13-14 Stats: 6 lines in 2 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
On Thu, 26 May 2022 18:08:13 GMT, XenoAmess wrote: >> as title. > > XenoAmess has updated the pull request with a new target base due to a merge > or a rebase. The pull request now contains 16 commits: > > - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780 > - Merge remote-tracking branch 'openjdk/master' into fix_8284780 > ># Conflicts: ># test/jdk/java/util/HashMap/WhiteBoxResizeTest.java > - add 8284780 to test > - redo the tests > - rename items to elements > - add test for newHashSet and newLinkedHashSet > - revert much too changes for newHashSet > - add more replaces > - add more replaces > - add more replaces > - ... and 6 more: > https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4 Thanks for the updates. I've made a couple minor changes to the specs; please merge the latest commit from this branch: https://github.com/stuart-marks/jdk/tree/pull/8302 I've created a CSR and have included these changes in it. Please review: [JDK-8287419](https://bugs.openjdk.java.net/browse/JDK-8287419) I'll look at the test changes later. I wanted to get the CSR moving first, since it will take a few days (and a long weekend in the US is coming up). - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
On Thu, 26 May 2022 18:08:13 GMT, XenoAmess wrote:
>> as title.
>
> XenoAmess has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 16 commits:
>
> - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780
> - Merge remote-tracking branch 'openjdk/master' into fix_8284780
>
># Conflicts:
># test/jdk/java/util/HashMap/WhiteBoxResizeTest.java
> - add 8284780 to test
> - redo the tests
> - rename items to elements
> - add test for newHashSet and newLinkedHashSet
> - revert much too changes for newHashSet
> - add more replaces
> - add more replaces
> - add more replaces
> - ... and 6 more:
> https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4
src/java.base/share/classes/java/util/HashSet.java line 130:
> 128: * @apiNote
> 129: * To create a {@code HashSet} with an initial capacity that
> accommodates
> 130: * an expected number of items, use {@link #newHashSet(int)
> newHashSet}.
Suggestion:
* an expected number of elements, use {@link #newHashSet(int) newHashSet}.
src/java.base/share/classes/java/util/HashSet.java line 147:
> 145: * @apiNote
> 146: * To create a {@code HashSet} with an initial capacity that
> accommodates
> 147: * an expected number of items, use {@link #newHashSet(int)
> newHashSet}.
Suggestion:
* an expected number of elements, use {@link #newHashSet(int) newHashSet}.
src/java.base/share/classes/java/util/HashSet.java line 391:
> 389: *
> 390: * @param numElementsthe expected number of elements
> 391: * @param the type of keys maintained by this set
Suggestion:
* @param the type of elements maintained by this set
src/java.base/share/classes/java/util/LinkedHashSet.java line 131:
> 129: * @apiNote
> 130: * To create a {@code LinkedHashSet} with an initial capacity that
> accommodates
> 131: * an expected number of items, use {@link #newLinkedHashSet(int)
> newLinkedHashSet}.
Suggestion:
* an expected number of elements, use {@link #newLinkedHashSet(int)
newLinkedHashSet}.
src/java.base/share/classes/java/util/LinkedHashSet.java line 148:
> 146: * @apiNote
> 147: * To create a {@code LinkedHashSet} with an initial capacity that
> accommodates
> 148: * an expected number of items, use {@link #newLinkedHashSet(int)
> newLinkedHashSet}.
Suggestion:
* an expected number of elements, use {@link #newLinkedHashSet(int)
newLinkedHashSet}.
src/java.base/share/classes/java/util/LinkedHashSet.java line 212:
> 210: *
> 211: * @param numElementsthe expected number of elements
> 212: * @param the type of keys maintained by this set
Suggestion:
* @param the type of elements maintained by this set
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]
On Thu, 26 May 2022 23:05:32 GMT, Joe Darcy wrote: >> Time to start getting ready for JDK 20... > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by iris (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: 8284199: Implementation of Structured Concurrency (Incubator) [v3]
On Tue, 24 May 2022 15:15:43 GMT, Alan Bateman wrote:
>> This is the implementation of JEP 428: Structured Concurrency (Incubator).
>>
>> This is a non-final API that provides a gentle on-ramp to structure a task
>> as a family of concurrent subtasks, and to coordinate the subtasks as a unit.
>
> Alan Bateman has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Add statement to close about thread termination
src/jdk.incubator.concurrent/share/classes/jdk/incubator/concurrent/StructuredTaskScope.java
line 668:
> 666: sb.append('/');
> 667: }
> 668: String id = getClass().getName() + "@" +
> System.identityHashCode(this);
Can use Objects.toIdentityString() instead.
-
PR: https://git.openjdk.java.net/jdk/pull/8787
RFR: 8287125: [macos] Multiple jpackage tests fail/timeout on same host
Looks like regression from JDK-8277493. JDK-8277493 will always un-sign app image. Un-signing takes time since we enumerating all files and un-signing binaries one by one. Average increase 2-3 minutes for tests which generates multiple app images. Fixed by increasing timeout for reported tests. - Commit messages: - 8287125: [macos] Multiple jpackage tests fail/timeout on same host Changes: https://git.openjdk.java.net/jdk/pull/8911/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8911&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287125 Stats: 6 lines in 3 files changed: 0 ins; 0 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/8911.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8911/head:pull/8911 PR: https://git.openjdk.java.net/jdk/pull/8911
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v20]
On Wed, 25 May 2022 19:22:51 GMT, Roger Riggs wrote: > AccessFlags.SUPER can/should be removed; it is unused and will be redefined > in the [Value Objects JEP](https://openjdk.java.net/jeps/8277163). It will be > a cleaner transition if there is no opportunity to create a dependency on the > obsolete flag. Hmm. I don't agree with that conclusion. I see the role of AccessFlags is to model what is in the class file. ACC_SUPER has been documented and in multiple versions of the JVMS and has appeared set in class files. If the bit position is reused, that is fine and the API can accommodate that overlap, unlike a bit-flag based model. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]
On Fri, 27 May 2022 01:55:25 GMT, Mandy Chung wrote:
>> liach has updated the pull request with a new target base due to a merge or
>> a rebase. The incremental webrev excludes the unrelated changes brought in
>> by the merge/rebase. The pull request contains eight additional commits
>> since the last revision:
>>
>> - Merge branch 'master' into proxy-class-forname
>> - Move the try catch block as it doesn't throw checked exceptions
>> - remove unused field
>> - whitespace
>> - Copyright year
>> - typo
>> - 8285401: Proxy class initializer should use 3-arg `Class.forName` to
>> avoid unnecessary class initialization
>> - Test for eager initialization
>
> test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
>
>> 51: new Class[]{ Intf.class },
>> 52: (proxy, method, args) -> null);
>> 53: Assert.assertFalse(initialized, "parameter type initialized
>> eagerly");
>
> This expects "parameter type initialized eagerly" to be false. This may
> cause confusion to the reader. Maybe just simply "initialized expected:
> false" and a comment would help too. Similarly for line 56.
I renamed the message to "parameter type initialized unnecessarily", which
should be more clear.
-
PR: https://git.openjdk.java.net/jdk/pull/8800
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v5]
> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of > `Class.forName(String)`. `make test > TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new > `LazyInitializationTest` failing the eager initialization check on the > baseline and passing with this patch. > > On a side note, this might reduce the number of methods that can be encoded > in a proxy due to code attribute size restrictions; we probably would address > that in another issue, as we never mandated a count of methods that the proxy > must be able to implement. > > Mandy, would you mind review this? liach has updated the pull request incrementally with one additional commit since the last revision: Improve test messages - Changes: - all: https://git.openjdk.java.net/jdk/pull/8800/files - new: https://git.openjdk.java.net/jdk/pull/8800/files/1262e8fa..1090df4a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8800&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8800&range=03-04 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8800.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8800/head:pull/8800 PR: https://git.openjdk.java.net/jdk/pull/8800
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]
On Fri, 27 May 2022 00:16:00 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Doesn't hurt to be more specific Marked as reviewed by mchung (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]
On Thu, 26 May 2022 20:52:43 GMT, liach wrote:
>> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of
>> `Class.forName(String)`. `make test
>> TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new
>> `LazyInitializationTest` failing the eager initialization check on the
>> baseline and passing with this patch.
>>
>> On a side note, this might reduce the number of methods that can be encoded
>> in a proxy due to code attribute size restrictions; we probably would
>> address that in another issue, as we never mandated a count of methods that
>> the proxy must be able to implement.
>>
>> Mandy, would you mind review this?
>
> liach has updated the pull request with a new target base due to a merge or a
> rebase. The incremental webrev excludes the unrelated changes brought in by
> the merge/rebase. The pull request contains eight additional commits since
> the last revision:
>
> - Merge branch 'master' into proxy-class-forname
> - Move the try catch block as it doesn't throw checked exceptions
> - remove unused field
> - whitespace
> - Copyright year
> - typo
> - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid
> unnecessary class initialization
> - Test for eager initialization
Thanks for doing this. It looks okay. JDK-7194006, having a variant of
`Class::forName` that loads the named class with the class loader of the
current class, would simplify this. This can be updated when such an API is
defined in the future.
test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
> 51: new Class[]{ Intf.class },
> 52: (proxy, method, args) -> null);
> 53: Assert.assertFalse(initialized, "parameter type initialized
> eagerly");
This expects "parameter type initialized eagerly" to be false. This may cause
confusion to the reader. Maybe just simply "initialized expected: false" and a
comment would help too. Similarly for line 56.
test/jdk/java/lang/reflect/Proxy/LazyInitializationTest.java line 53:
> 51: new Class[]{ Intf.class },
> 52: (proxy, method, args) -> null);
> 53: Assert.assertFalse(initialized, "parameter type initialized
> eagerly");
This expects "parameter type initialized eagerly" to be false. This may cause
confusion to the reader. Maybe just simply "initialized expected: false" and a
comment would help too. Similarly for line 56.
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8800
RFR: 8284400: Improve XPath exception handling
Address some insufficiency of error handling in the XPath implementation. Some cleanup where appropriate, without attempting to do too much as this is an component that hasn't had much changes for 15 years, a fairly stable application. - Commit messages: - 8284400: Improve XPath exception handling Changes: https://git.openjdk.java.net/jdk/pull/8910/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8910&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284400 Stats: 861 lines in 11 files changed: 617 ins; 179 del; 65 mod Patch: https://git.openjdk.java.net/jdk/pull/8910.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8910/head:pull/8910 PR: https://git.openjdk.java.net/jdk/pull/8910
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v5]
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the > hash map with a simple lookup, similar to what's done in > [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) liach has updated the pull request incrementally with one additional commit since the last revision: Doesn't hurt to be more specific - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/6d171268..6aa89eff Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=03-04 Stats: 11 lines in 1 file changed: 1 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/8801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801 PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]
On Thu, 26 May 2022 23:42:27 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Make primitive type info more reader friendly src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 979: > 977: unwrapMethodDesc = "()" + baseTypeString; > 978: this.loadOpcode = loadOpcode; > 979: this.returnOpcode = loadOpcode - ILOAD + IRETURN; This could do it. It would be more explicit to take the return opcode as an argument to the constructor. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 23:35:10 GMT, liach wrote: >> src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: >> >>> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >>> 968: String baseTypeString = wrapper.basicTypeString(); >>> 969: wrapperClassName = >>> dotToSlash(wrapper.wrapperType().getName()); >> >> Suggestion: >> >> wrapperClassName = wrapper.wrapperType().descriptorString(); >> >> >> It may worth to replace similar use of `dotToSlash(c.getName())` pattern. > > Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor > (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor > construction. ah, you're right. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:55:02 GMT, Mandy Chung wrote: >> liach 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 three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > Have you considered including the opcode for load and return instruction > instead of `opcodeOffset`? Hi @mlchung Mandy, I've changed the offset to seperate load and return opcodes, which is more in-line with how other fields in `PrimitiveTypeInfo` are used. In addition, I specified that the `wrapperClassName` is an internal name for clarification. Would you mind review this again, and review #8800, which avoids eager class initialization for parameter types used by proxy? - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 22:51:39 GMT, Mandy Chung wrote: >> liach 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 three additional commits >> since the last revision: >> >> - Merge branch 'master' into proxy-primitivetypeinfo >> - Convert PrimitiveTypeInfo to an enum >> - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo > > src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > >> 967: // single-char BaseType descriptor (see JVMS section 4.3.2) >> 968: String baseTypeString = wrapper.basicTypeString(); >> 969: wrapperClassName = >> dotToSlash(wrapper.wrapperType().getName()); > > Suggestion: > > wrapperClassName = wrapper.wrapperType().descriptorString(); > > > It may worth to replace similar use of `dotToSlash(c.getName())` pattern. Unfortunately, we want an internal name (`xxx/Abc`) than a field descriptor (`Lxxx/Abc;`). But we can use descriptor string for the valueOf descriptor construction. - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v4]
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the > hash map with a simple lookup, similar to what's done in > [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) liach has updated the pull request incrementally with one additional commit since the last revision: Make primitive type info more reader friendly - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/96c0835e..6d171268 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=02-03 Stats: 26 lines in 1 file changed: 7 ins; 1 del; 18 mod Patch: https://git.openjdk.java.net/jdk/pull/8801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801 PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8283660: Convert com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java finalizer to Cleaner [v6]
On Sat, 23 Apr 2022 00:09:48 GMT, Brent Christian wrote:
>> src/java.naming/share/classes/com/sun/jndi/ldap/AbstractLdapNamingEnumeration.java
>> line 73:
>>
>>> 71: public void run() {
>>> 72: if (enumClnt != null) {
>>> 73: enumClnt.clearSearchReply(res, homeCtx.reqCtls);
>>
>> It's a bit strange to see that there is no guard here to verify that
>> `homeCtx != null`, when line 76 implies that it might. My reading is that
>> `homeCtxt` is not supposed to be `null` when `enumClnt` is not `null`. That
>> could be explained in a comment, or alternatively asserted just before line
>> 73 (`assert homeCtx != null;`)
>
> Yes, it is strange -- that code came from the finalizer. I will add an assert.
It appears that the update() method can set 'homeCtx' for 'ne' to null while
leaving 'enumClnt' non-null (~L410).
Perhaps here, the clearSearchReply() call should only happen if both are
non-null.
-
PR: https://git.openjdk.java.net/jdk/pull/8311
Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]
On Thu, 26 May 2022 23:05:32 GMT, Joe Darcy wrote: >> Time to start getting ready for JDK 20... > > Joe Darcy has updated the pull request incrementally with one additional > commit since the last revision: > > Respond to review feedback. Marked as reviewed by kcr (Author). - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 22:33:29 GMT, Mandy Chung wrote: >> The original check and `Modules.addOpen` calls were added in >> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the >> `invokeDefault` support was added. >> >> See: >> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667, >> or #313 > > Ah, I see. That assert was added in PR #313 as a clarification to the current > proxy implementation but not required by the > `InvocationHandle::invokeDefault`. It could have been added before the > default method support. It appears to me that it's required by `proxyClassLookup` to perform reflective access on the dynamic module or the module where the package-private interface is located. - PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v6]
> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, > the interfaces are iterated twice. The two passes can be merged into one, > yielding the whole proxy definition context (module, package, whether there's > package-private interface) when determining the module. > > Split from #8278. Helpful for moving proxies to hidden classes, but is a good > cleanup on its own. liach has updated the pull request incrementally with one additional commit since the last revision: Fixes suggested by mandy - Changes: - all: https://git.openjdk.java.net/jdk/pull/8281/files - new: https://git.openjdk.java.net/jdk/pull/8281/files/20edc525..9d7ebaab Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8281&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8281&range=04-05 Stats: 13 lines in 1 file changed: 4 ins; 0 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/8281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281 PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: JDK-8282798 java.lang.runtime.Carrier [v19]
On Thu, 31 Mar 2022 18:48:39 GMT, Jim Laskey wrote: >> We propose to provide a runtime anonymous carrier class object generator; >> java.lang.runtime.Carrier. This generator class is designed to share >> anonymous classes when shapes are similar. For example, if several clients >> require objects containing two integer fields, then Carrier will ensure that >> each client generates carrier objects using the same underlying anonymous >> class. >> >> See JBS for details. > > Jim Laskey has updated the pull request incrementally with one additional > commit since the last revision: > > Change Carriers API Will this be potentially revived if there's any progress in templated strings? - PR: https://git.openjdk.java.net/jdk/pull/7744
Withdrawn: JDK-8282798 java.lang.runtime.Carrier
On Tue, 8 Mar 2022 14:02:39 GMT, Jim Laskey wrote: > We propose to provide a runtime anonymous carrier class object generator; > java.lang.runtime.Carrier. This generator class is designed to share > anonymous classes when shapes are similar. For example, if several clients > require objects containing two integer fields, then Carrier will ensure that > each client generates carrier objects using the same underlying anonymous > class. > > See JBS for details. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/7744
Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]
On Thu, 26 May 2022 22:38:12 GMT, David Holmes wrote: >> Joe Darcy has updated the pull request incrementally with one additional >> commit since the last revision: >> >> Respond to review feedback. > > src/java.base/share/classes/jdk/internal/org/objectweb/asm/Opcodes.java line > 312: > >> 310: int V18 = 0 << 16 | 62; >> 311: int V19 = 0 << 16 | 63; >> 312: int V20 = 0 << 17 | 64; > > 17 ?? > > Though why do we even bother with this when the minor version is zero? Simple > assignment works. Doh! Will fix in the next pushed. - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: JDK-8284858: Start of release updates for JDK 20
On Thu, 26 May 2022 22:40:59 GMT, Kevin Rushforth wrote: > You also need to change the JBS version from 19 to 20 in > [`.jcheck/conf`](https://github.com/openjdk/jdk/blob/6a33974a6b8a629744c6d76c3b4fa1f772e52ac8/.jcheck/conf#L4): Acknowledged; will fix in the next push. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: JDK-8284858: Start of release updates for JDK 20 [v2]
> Time to start getting ready for JDK 20... Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Respond to review feedback. - Changes: - all: https://git.openjdk.java.net/jdk/pull/8236/files - new: https://git.openjdk.java.net/jdk/pull/8236/files/49c871d9..96be1787 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8236&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8236&range=00-01 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8236.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8236/head:pull/8236 PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
On Thu, 26 May 2022 20:55:53 GMT, liach wrote: >> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace >> the hash map with a simple lookup, similar to what's done in >> [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) > > liach 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 three additional commits since > the last revision: > > - Merge branch 'master' into proxy-primitivetypeinfo > - Convert PrimitiveTypeInfo to an enum > - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo Have you considered including the opcode for load and return instruction instead of `opcodeOffset`? src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 969: > 967: // single-char BaseType descriptor (see JVMS section 4.3.2) > 968: String baseTypeString = wrapper.basicTypeString(); > 969: wrapperClassName = > dotToSlash(wrapper.wrapperType().getName()); Suggestion: wrapperClassName = wrapper.wrapperType().descriptorString(); It may worth to replace similar use of `dotToSlash(c.getName())` pattern. src/java.base/share/classes/java/lang/reflect/ProxyGenerator.java line 986: > 984: if (cl == float.class) return FLOAT; > 985: if (cl == double.class) return DOUBLE; > 986: throw new AssertionError(); Suggestion: throw new AssertionError(cl); - PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: JDK-8284858: Start of release updates for JDK 20
On Thu, 14 Apr 2022 05:09:14 GMT, Joe Darcy wrote: > Time to start getting ready for JDK 20... One comment below. I ignored the sym files. Everything else appears okay. Thanks. src/java.base/share/classes/jdk/internal/org/objectweb/asm/Opcodes.java line 312: > 310: int V18 = 0 << 16 | 62; > 311: int V19 = 0 << 16 | 63; > 312: int V20 = 0 << 17 | 64; 17 ?? Though why do we even bother with this when the minor version is zero? Simple assignment works. - Marked as reviewed by dholmes (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: JDK-8284858: Start of release updates for JDK 20
On Thu, 14 Apr 2022 05:09:14 GMT, Joe Darcy wrote: > Time to start getting ready for JDK 20... You also need to change the JBS version from 19 to 20 in [`.jcheck/conf`](https://github.com/openjdk/jdk/blob/6a33974a6b8a629744c6d76c3b4fa1f772e52ac8/.jcheck/conf#L4): - PR: https://git.openjdk.java.net/jdk/pull/8236
Withdrawn: 8283726: x86 intrinsics for compare method in Integer and Long
On Sun, 27 Mar 2022 06:15:34 GMT, Srinivas Vamsi Parasa wrote: > Implements x86 intrinsics for compare() method in java.lang.Integer and > java.lang.Long. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/7975
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 22:01:56 GMT, liach wrote:
>> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
>>
>>> 511:
>>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>>> 513: // Required for default method invocation
>>
>> Is this comment true?
>>
>> The module of the proxy class opens the package to `java.base` if the proxy
>> interface is non-public in a named module or if all proxy interfaces are
>> public but a proxy interface is not unconditionally exported.
>
> The original check and `Modules.addOpen` calls were added in
> [8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the
> `invokeDefault` support was added.
>
> See:
> https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
> or #313
Ah, I see. That assert was added in PR #313 as a clarification to the current
proxy implementation but not required by the `InvocationHandle::invokeDefault`.
It could have been added before the default method support.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
RFR: JDK-8284858: Start of release updates for JDK 20
Time to start getting ready for JDK 20... - Commit messages: - Update symbol information for JDK 19 b24. - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - Update symbol information for JDK 19 b23. - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - Merge branch 'master' into JDK-8284858 - ... and 23 more: https://git.openjdk.java.net/jdk/compare/295be6f1...49c871d9 Changes: https://git.openjdk.java.net/jdk/pull/8236/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8236&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284858 Stats: 6210 lines in 67 files changed: 6166 ins; 0 del; 44 mod Patch: https://git.openjdk.java.net/jdk/pull/8236.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8236/head:pull/8236 PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: JDK-8284858: Start of release updates for JDK 20
On Thu, 14 Apr 2022 05:09:14 GMT, Joe Darcy wrote: > Time to start getting ready for JDK 20... The expected kinds of updates to start up JDK 20. - PR: https://git.openjdk.java.net/jdk/pull/8236
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei ok, the updates look fine. - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 21:32:53 GMT, Mandy Chung wrote:
>> liach 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 seven additional commits
>> since the last revision:
>>
>> - Group proxy location validation all into the class context constructor
>> - Merge branch 'master' into fix/proxy-single-pass
>> - Update
>> - Updates suggested by mandy
>> - Merge branch 'master' into fix/proxy-single-pass
>> - Don't need to complexify module cache
>> - 8284942: Proxy building can just iterate superinterfaces once
>
> src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
>
>> 511:
>> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
>> 513: // Required for default method invocation
>
> Is this comment true?
>
> The module of the proxy class opens the package to `java.base` if the proxy
> interface is non-public in a named module or if all proxy interfaces are
> public but a proxy interface is not unconditionally exported.
The original check and `Modules.addOpen` calls were added in
[8159476](https://bugs.openjdk.java.net/browse/JDK-8159746), when the
`invokeDefault` support was added.
See:
https://github.com/openjdk/jdk/commit/56b15fbbcc7c04252f2712d859ff7b820b7c79ad#diff-c15cc774a95bac204c294b9ca8e20332c81904e506e16bb7d5a82d1c30cced17R667,
or #313
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 21:22:23 GMT, Roger Riggs wrote: > Even using a Cleaner is a more overhead than necessary. I would have skipped > the overhead of a cleaner and Atomic classes with something more self > contained as a static method: I agreed that the using of Cleaner is still heavy, but it may be acceptable as the code is for testing only. If a new static method is introduced, test cases using the current API would also need update so as to benefit from it. I was hesitate for test cases update as it may involve more evaluations (and risks). But let me check. - PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
On Thu, 26 May 2022 20:53:29 GMT, liach wrote:
>> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass,
>> the interfaces are iterated twice. The two passes can be merged into one,
>> yielding the whole proxy definition context (module, package, whether
>> there's package-private interface) when determining the module.
>>
>> Split from #8278. Helpful for moving proxies to hidden classes, but is a
>> good cleanup on its own.
>
> liach 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 seven additional commits since
> the last revision:
>
> - Group proxy location validation all into the class context constructor
> - Merge branch 'master' into fix/proxy-single-pass
> - Update
> - Updates suggested by mandy
> - Merge branch 'master' into fix/proxy-single-pass
> - Don't need to complexify module cache
> - 8284942: Proxy building can just iterate superinterfaces once
src/java.base/share/classes/java/lang/reflect/Proxy.java line 498:
> 496: new ClassLoaderValue<>();
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
Suggestion:
private record ProxyClassContext(Module module, String packageName, int
accessFlags) {
src/java.base/share/classes/java/lang/reflect/Proxy.java line 499:
> 497:
> 498: private record ProxyClassContext(Module module, String pkg, int
> accessFlags) {
> 499: private ProxyClassContext {
We should validate the `accessFlags` value as it must be 0 or `PUBLIC`.
src/java.base/share/classes/java/lang/reflect/Proxy.java line 513:
> 511:
> 512: if (!module.isOpen(pkg, Proxy.class.getModule())) {
> 513: // Required for default method invocation
Is this comment true?
The module of the proxy class opens the package to `java.base` if the proxy
interface is non-public in a named module or if all proxy interfaces are public
but a proxy interface is not unconditionally exported.
-
PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8287384: Speed up jdk.test.lib.util.ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote:
> Hi,
>
> May I have this test update reviewed?
>
> The ForceGC could be enhanced by using smaller wait/sleep time, and shared
> cleaner.
>
> Thanks,
> Xuelei
Even using a Cleaner is a more overhead than necessary.
I would have skipped the overhead of a cleaner and Atomic classes with
something more self contained as a static method:
/**
* The garbage collector is invoked to find unreferenced objects.
* A new object is created and then freed. The method returns
* when the object has been reclaimed or the GC did not complete in 10
seconds.
*
* @return true if GC was complete, false if did not complete after 10
Seconds
*/
public static boolean waitForGC() {
ReferenceQueue queue = new ReferenceQueue<>();
Object obj = new Object();
PhantomReference ref = new PhantomReference<>(obj, queue);
obj = null;
Reference.reachabilityFence(obj);
Reference.reachabilityFence(ref);
System.gc();
for (int retries = 100; retries > 0; retries--) {
try {
var r = queue.remove(100L);
if (r != null) {
return true;
}
} catch (InterruptedException ie) {
// ignore, the loop will try again
}
System.gc();
}
return false;
}
YMMV
-
PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8287292: Improve TransformKey to pack more kinds of transforms efficiently [v3]
On Wed, 25 May 2022 14:13:52 GMT, Claes Redestad wrote:
>> The bespoke caching scheme in `jl.invoke.LambdaFormEditor.TransformKey`
>> allows keys to be compacted when all byte values of the key fit in 4 bits,
>> otherwise a byte array is allocated and used. This means that all transforms
>> with a kind value above 15 will be forced to allocate and use array
>> comparisons.
>>
>> Removing unused and folding some transforms to ensure all existing kinds can
>> fit snugly within the 0-15 value range realize a minor improvement to
>> footprint, speed and allocation pressure of affected transforms, e.g.
>> ~300bytes/op reduction in the `StringConcatFactoryBootstraps` microbenchmark:
>>
>> Baseline:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt 15
>> 2048.475 ? 69.887 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt 15
>> 3487.311 ? 80.385B/op
>>
>>
>> Patched:
>>
>> Benchmark Mode Cnt
>> Score Error Units
>> SCFB.makeConcatWithConstants avgt 15
>> 1961.985 ? 101.519 ns/op
>> SCFB.makeConcatWithConstants:?gc.alloc.rate.norm avgt 15
>> 3156.478 ? 183.600B/op
>
> Claes Redestad has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Missed correctly taking b1 into account in of(byte, int, int...)
> (java/lang/String/concat/ImplicitStringConcatShapes.java test failure)
LGTM with a couple of suggestions.
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 219:
> 217: return new TransformKey(fullBytes);
> 218: }
> 219: static TransformKey of(byte kind, int b1, int b2, int[] b3456) {
Nit: I can't figure out if `b3456` intends to be a different convention than
the others `b123` and `b234`. I would expect something like this:
Suggestion:
static TransformKey of(byte kind, int b1, int b2, int... b345) {
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 252:
> 250: if (!inRange(bitset))
> 251: return 0;
> 252: pb = pb | b2 << (2 * PACKED_BYTE_SIZE) | b1 <<
> PACKED_BYTE_SIZE | b0;
Suggestion:
pb = pb | packagedBytes(b0, b1, b2);
src/java.base/share/classes/java/lang/invoke/LambdaFormEditor.java line 267:
> 265: if (!inRange(bitset))
> 266: return 0;
> 267: pb = pb | b1 << PACKED_BYTE_SIZE | b0;
Suggestion:
pb = pb | packagedBytes(b0, b1);
-
Marked as reviewed by mchung (Reviewer).
PR: https://git.openjdk.java.net/jdk/pull/8881
RFR: [CSR] 8287376: BigDecimal(String) must allow a larger range for the exponent
Currently, BigDecimal(String) requires the exponent part to lie in the int range. This CSR [1] removes this limitation, as it otherwise precludes constructing an instance from a string generated by BigDecimal.toString(). Greetings Raffaello [1] https://bugs.openjdk.java.net/browse/JDK-8287376
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v8]
On Thu, 26 May 2022 06:19:40 GMT, Jatin Bhateja wrote: >> Yes. > >> @jatin-bhateja something wrong with merge. `vpadd()` is removed. It was >> added by #8778 and still is used in `x86.ad`. > > Hi @vnkozlov , after integration of PR 8778 there were there were two copies > of vpadd with same signature, so removed one of them. Okay. Got it. - PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8284960: Integration of JEP 426: Vector API (Fourth Incubator) [v10]
On Wed, 25 May 2022 06:29:23 GMT, Jatin Bhateja wrote: >> Hi All, >> >> Patch adds the planned support for new vector operations and APIs targeted >> for [JEP 426: Vector API (Fourth >> Incubator).](https://bugs.openjdk.java.net/browse/JDK-8280173) >> >> Following is the brief summary of changes:- >> >> 1) Extends the scope of existing lanewise API for following new vector >> operations. >>- VectorOperations.BIT_COUNT: counts the number of one-bits >>- VectorOperations.LEADING_ZEROS_COUNT: counts the number of leading zero >> bits >>- VectorOperations.TRAILING_ZEROS_COUNT: counts the number of trailing >> zero bits >>- VectorOperations.REVERSE: reversing the order of bits >>- VectorOperations.REVERSE_BYTES: reversing the order of bytes >>- compress and expand bits: Semantics are based on Hacker's Delight >> section 7-4 Compress, or Generalized Extract. >> >> 2) Adds following new APIs to perform cross lane vector compress and >> expansion operations under the influence of a mask. >>- Vector.compress >>- Vector.expand >>- VectorMask.compress >> >> 3) Adds predicated and non-predicated versions of following new APIs to load >> and store the contents of vector from foreign MemorySegments. >> - Vector.fromMemorySegment >> - Vector.intoMemorySegment >> >> 4) C2 Compiler IR enhancements and optimized X86 and AARCH64 backend support >> for each newly added operation. >> >> >> Patch has been regressed over AARCH64 and X86 targets different AVX levels. >> >> Kindly review and share your feedback. >> >> Best Regards, >> Jatin > > Jatin Bhateja has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 20 commits: > > - 8284960: Post merge cleanups. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 > - 8284960: Review comments resolved. > - 8284960: Integrating incremental patches. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 > - 8284960: Changes to enable jdk.incubator.vector to be treated as preview > participant. Code re-organization related to Reverse/ReverseByte IR > transforms. > - 8284960: Adding --enable-preview in vectorAPI benchmarks. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 > - 8284960: Review comments resolution. > - Merge branch 'master' of http://github.com/openjdk/jdk into JDK-8284960 > - ... and 10 more: > https://git.openjdk.java.net/jdk/compare/742644e2...0f6e1584 Good. - Marked as reviewed by kvn (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8425
Re: RFR: 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo [v3]
> Simplify opcode handling, use `final` in `PrimitiveTypeInfo`, and replace the > hash map with a simple lookup, similar to what's done in > [JDK-8284880](https://bugs.openjdk.java.net/browse/JDK-8284880) (#8242) liach 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 three additional commits since the last revision: - Merge branch 'master' into proxy-primitivetypeinfo - Convert PrimitiveTypeInfo to an enum - 8287064: Modernize ProxyGenerator.PrimitiveTypeInfo - Changes: - all: https://git.openjdk.java.net/jdk/pull/8801/files - new: https://git.openjdk.java.net/jdk/pull/8801/files/be9987bb..96c0835e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8801&range=01-02 Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 mod Patch: https://git.openjdk.java.net/jdk/pull/8801.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8801/head:pull/8801 PR: https://git.openjdk.java.net/jdk/pull/8801
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v5]
> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, > the interfaces are iterated twice. The two passes can be merged into one, > yielding the whole proxy definition context (module, package, whether there's > package-private interface) when determining the module. > > Split from #8278. Helpful for moving proxies to hidden classes, but is a good > cleanup on its own. liach 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 seven additional commits since the last revision: - Group proxy location validation all into the class context constructor - Merge branch 'master' into fix/proxy-single-pass - Update - Updates suggested by mandy - Merge branch 'master' into fix/proxy-single-pass - Don't need to complexify module cache - 8284942: Proxy building can just iterate superinterfaces once - Changes: - all: https://git.openjdk.java.net/jdk/pull/8281/files - new: https://git.openjdk.java.net/jdk/pull/8281/files/26bde80b..20edc525 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8281&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8281&range=03-04 Stats: 37340 lines in 627 files changed: 8639 ins; 27112 del; 1589 mod Patch: https://git.openjdk.java.net/jdk/pull/8281.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8281/head:pull/8281 PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization [v4]
> Simplify calls `Class.forName(String, boolean, ClassLoader)` instead of > `Class.forName(String)`. `make test > TEST="jtreg:test/jdk/java/lang/reflect/Proxy"` passes, with the new > `LazyInitializationTest` failing the eager initialization check on the > baseline and passing with this patch. > > On a side note, this might reduce the number of methods that can be encoded > in a proxy due to code attribute size restrictions; we probably would address > that in another issue, as we never mandated a count of methods that the proxy > must be able to implement. > > Mandy, would you mind review this? liach has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains eight additional commits since the last revision: - Merge branch 'master' into proxy-class-forname - Move the try catch block as it doesn't throw checked exceptions - remove unused field - whitespace - Copyright year - typo - 8285401: Proxy class initializer should use 3-arg `Class.forName` to avoid unnecessary class initialization - Test for eager initialization - Changes: - all: https://git.openjdk.java.net/jdk/pull/8800/files - new: https://git.openjdk.java.net/jdk/pull/8800/files/82f8eeb9..1262e8fa Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8800&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8800&range=02-03 Stats: 37306 lines in 626 files changed: 8637 ins; 27109 del; 1560 mod Patch: https://git.openjdk.java.net/jdk/pull/8800.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8800/head:pull/8800 PR: https://git.openjdk.java.net/jdk/pull/8800
Integrated: 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get
On Sat, 30 Apr 2022 08:56:23 GMT, Andrey Turbanov wrote: > The method `java.util.zip.ZipFile.Source#get` could be improved by usage of > `Map.putIfAbsent` instead of separate `containsKey`/`get`/`put` calls. We > known that HashMap `java.util.zip.ZipFile.Source#files` can contain only > non-null values. And to check if putIfAbsent was successful or not we can > just compare result with `null`. > It makes code a bit cleaner and faster. This pull request has now been integrated. Changeset: 295be6f1 Author:Andrey Turbanov URL: https://git.openjdk.java.net/jdk/commit/295be6f10ff50eb743c6840e7dcd319fe6f39d0f Stats: 6 lines in 1 file changed: 0 ins; 1 del; 5 mod 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get Reviewed-by: jpai, alanb - PR: https://git.openjdk.java.net/jdk/pull/8481
Re: RFR: 8282662: Use List.of() factory method to reduce memory consumption [v6]
> `List.of()` along with `Set.of()` create unmodifiable `List/Set` but with > smaller footprint comparing to `Arrays.asList()` / `new HashSet()` when > called with vararg of size 0, 1, 2. > > In general replacement of `Arrays.asList()` with `List.of()` is dubious as > the latter is null-hostile, however in some cases we are sure that arguments > are non-null. Into this PR I've included the following cases (in addition to > those where the argument is proved to be non-null at compile-time): > - `MethodHandles.longestParameterList()` never returns null > - parameter types are never null > - interfaces used for proxy construction and returned from > `Class.getInterfaces()` are never null > - exceptions types of method signature are never null Сергей Цыпанов has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains six commits: - 8282662: Revert wrong copyright year change - 8282662: Revert ProxyGenerator - 8282662: Revert ProxyGenerator - 8282662: Revert dubious changes in MethodType - 8282662: Revert dubious changes - 8282662: Use List/Set.of() factory methods to save memory - Changes: https://git.openjdk.java.net/jdk/pull/7729/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=7729&range=05 Stats: 12 lines in 4 files changed: 1 ins; 2 del; 9 mod Patch: https://git.openjdk.java.net/jdk/pull/7729.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7729/head:pull/7729 PR: https://git.openjdk.java.net/jdk/pull/7729
Re: RFR: 8287384: Speed up ForceGC
On Thu, 26 May 2022 18:50:07 GMT, Xue-Lei Andrew Fan wrote: > Hi, > > May I have this test update reviewed? > > The ForceGC could be enhanced by using smaller wait/sleep time, and shared > cleaner. > > Thanks, > Xuelei Please rename the RFE title to be less generic. How about "Speed up jdk.test.lib.util.ForceGC"? - Changes requested by iklam (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8907
RFR: 8287384: Speed up ForceGC
Hi, May I have this test update reviewed? The ForceGC could be enhanced by using smaller wait/sleep time, and shared cleaner. Thanks, Xuelei - Commit messages: - 8287384: Speed up ForceGC Changes: https://git.openjdk.java.net/jdk/pull/8907/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8907&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8287384 Stats: 21 lines in 1 file changed: 8 ins; 1 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8907/head:pull/8907 PR: https://git.openjdk.java.net/jdk/pull/8907
Re: RFR: 8284942: Proxy building can just iterate superinterfaces once [v4]
On Fri, 20 May 2022 02:25:32 GMT, liach wrote: >> Currently, in ProxyBuilder::mapToModule and ProxyBuilder::defineProxyClass, >> the interfaces are iterated twice. The two passes can be merged into one, >> yielding the whole proxy definition context (module, package, whether >> there's package-private interface) when determining the module. >> >> Split from #8278. Helpful for moving proxies to hidden classes, but is a >> good cleanup on its own. > > liach has updated the pull request incrementally with one additional commit > since the last revision: > > Update src/java.base/share/classes/java/lang/reflect/Proxy.java line 514: > 512: // Per JLS 7.4.2, unnamed package can only exist in > unnamed modules. > 513: // This means a package-private superinterface exist in > the unnamed > 514: // package of a named module. With this patch, I think line 505-517 can turn into assert or internal error as you suggested. Probably can be moved to the `ProxyClassContext` constructor too. - PR: https://git.openjdk.java.net/jdk/pull/8281
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v4]
On Wed, 18 May 2022 23:20:45 GMT, Stuart Marks wrote: >>> Need to add apiNote documentation section to capacity-based constructors >>> like for maps. >> >> @liach done. > > @XenoAmess oops, sorry for the delay. I think it would be good to get these > into 19 as companions to `HashMap.newHashMap` et. al. > > As before, I'd suggest reducing the number of changes to use sites in order > to make review easier. I would suggest keeping the changes under src in > java.base, java.net.http, java.rmi, and jdk.zipfs, and omitting all the other > changes. Also keep the changes under test/jdk. > > There needs to be a test for these new methods. I haven't thought much how to > do that. My first attempt would be to modify our favorite WhiteBoxResizeTest > and add a bit of machinery that asserts the table length of the HashMap > contained within the created HashSet/LinkedHashSet. I haven't looked at it > though, so it might not work out, in which case you should pursue an > alternative approach. > > I'll look at the specs and suggest updates as necessary and then handle > filing of a CSR. @stuart-marks @liach Hi guys. After a good sleep and a rethink, I re-wrote some part of the test. It seems be logically better than the way before. please have a look. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v3]
On Thu, 26 May 2022 16:04:17 GMT, Maxim Kartashev wrote: >> Following the logic from the comment directly above the changed line, since >> it doesn't matter which controller we pick, pick any available controller >> instead of the one called "memory" specifically. This way we are guarded >> against getting `null` as `anyController`, which is being immediately passed >> down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept >> `null` values. >> >> It is also worth noting that the previous checks (such as that at line 89) >> make sure that there exist at least one controller in the map. > > Maxim Kartashev has updated the pull request incrementally with one > additional commit since the last revision: > > Made requested changes Marked as reviewed by iklam (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v10]
On Thu, 26 May 2022 10:55:28 GMT, Yasumasa Suenaga wrote: >> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 >> on Fedora 36. >> As you can see, the warnings spreads several areas. Let me know if I should >> separate them by area. >> >> * -Wstringop-overflow >> * src/hotspot/share/oops/array.hpp >> * >> src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp >> >> In member function 'void Array::at_put(int, const T&) [with T = unsigned >> char]', >> inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, >> inlined from 'void ConstantPool::method_at_put(int, int, int)' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, >> inlined from 'ConstantPool* >> BytecodeConstantPool::create_constant_pool(JavaThread*) const' at >> /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: > > Yasumasa Suenaga has updated the pull request incrementally with one > additional commit since the last revision: > > Fix comments Marked as reviewed by kbarrett (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8233760: Result of BigDecimal.toString throws overflow exception on new BigDecimal(str)
On Thu, 26 May 2022 18:02:14 GMT, Raffaello Giulietti wrote: > BigDecimal(String) currently fails to accept some strings produced by > BigDecimal.toString(). This PR removes this limitation. This happens because the constructor currently only accepts exponents in the `int` range (more precisely, in the open range (-2^31, 2^31)). It should accept a larger range of exponents, as long as the resulting `BigDecimal` has a scale in the `int` range. - PR: https://git.openjdk.java.net/jdk/pull/8905
RFR: 8233760: Result of BigDecimal.toString throws overflow exception on new BigDecimal(str)
BigDecimal(String) currently fails to accept some strings produced by BigDecimal.toString(). This PR removes this limitation. - Commit messages: - 8233760: Result of BigDecimal.toString throws overflow exception on new BigDecimal(str) Changes: https://git.openjdk.java.net/jdk/pull/8905/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8905&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8233760 Stats: 48 lines in 2 files changed: 2 ins; 26 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/8905.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8905/head:pull/8905 PR: https://git.openjdk.java.net/jdk/pull/8905
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v13]
> as title. XenoAmess has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 15 commits: - Merge remote-tracking branch 'openjdk/master' into fix_8284780 # Conflicts: #test/jdk/java/util/HashMap/WhiteBoxResizeTest.java - add 8284780 to test - redo the tests - rename items to elements - add test for newHashSet and newLinkedHashSet - revert much too changes for newHashSet - add more replaces - add more replaces - add more replaces - add more replaces - ... and 5 more: https://git.openjdk.java.net/jdk/compare/7eb15593...59caa1f4 - Changes: https://git.openjdk.java.net/jdk/pull/8302/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=12 Stats: 162 lines in 31 files changed: 120 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v14]
> as title. XenoAmess has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 16 commits: - Merge branch 'master' of https://git.openjdk.java.net/jdk into fix_8284780 - Merge remote-tracking branch 'openjdk/master' into fix_8284780 # Conflicts: #test/jdk/java/util/HashMap/WhiteBoxResizeTest.java - add 8284780 to test - redo the tests - rename items to elements - add test for newHashSet and newLinkedHashSet - revert much too changes for newHashSet - add more replaces - add more replaces - add more replaces - ... and 6 more: https://git.openjdk.java.net/jdk/compare/7cb368b3...117918f4 - Changes: https://git.openjdk.java.net/jdk/pull/8302/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=13 Stats: 162 lines in 31 files changed: 120 ins; 0 del; 42 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:22:44 GMT, Stuart Marks wrote: >> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 360: >> >>> 358: throw new RuntimeException(e); >>> 359: } >>> 360: }) >> >> These probably need a `mapField.setAccessible(true)` call, or a `VarHandle` >> for the `HashSet.map` field. > > Yes, this test fails with IllegalAccessException. Probably it's easiest to > use a VarHandle to get private fields, similar to other usage already in this > test. > > This test case is a bit odd though in that it's supposed to test HashSet and > LinkedHashSet but it mostly actually tests HashMap. It creates the Set > instance and immediately extracts the HashMap, which is then passed to the > actual test, which operates directly on the HashMap. It would be preferable > to create a Set; add an element (so that it's properly allocated); and then > make assertions over the Set (which involve extracting the HashMap, etc.) It > seems like there should be factoring that allows this sort of arrangement to > be retrofitted without adding too much complication. > > Finally, please add "8284780" to the `@bug` line at the top of this test. @stuart-marks I refactored the tests. please have a look. @stuart-marks 8284780 added. - PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v12]
> as title. XenoAmess has updated the pull request incrementally with two additional commits since the last revision: - add 8284780 to test - redo the tests - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/4ec6ba9f..df5a8512 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=11 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=10-11 Stats: 92 lines in 1 file changed: 65 ins; 16 del; 11 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 04:50:33 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add test for newHashSet and newLinkedHashSet
>
> test/jdk/java/util/HashMap/WhiteBoxResizeTest.java line 354:
>
>> 352: rsc("rsls", size, cap, () -> {
>> 353: try {
>> 354: Field mapField =
>> HashSet.class.getDeclaredField("map");
>
> For clarity, consider using var handle/method handle and keep the reflection
> code in the `WhiteBoxResizeTest` constructor.
@liach I refactored the tests. please have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: JDK-8281001 Examine the behavior of Class::forName if the caller is null [v3]
> The Class::forName behavior change to match JNI FindClass is a compatible > change and seems pretty attractive as it would be expected that > Class::forName would give the same behavior as FindClass which uses the > system classloader. The test for 8281006 was enhanced to test for this > change. Merged master to pick up fixes to unrelated test failures to reduce > noise. Tim Prinzing has updated the pull request incrementally with one additional commit since the last revision: make javadoc consistent with other caller sensitve methods - Changes: - all: https://git.openjdk.java.net/jdk/pull/8711/files - new: https://git.openjdk.java.net/jdk/pull/8711/files/9d054ea6..4e5f8c2b Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8711&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8711&range=01-02 Stats: 4 lines in 1 file changed: 0 ins; 1 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8711.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8711/head:pull/8711 PR: https://git.openjdk.java.net/jdk/pull/8711
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v10]
On Wed, 25 May 2022 05:07:12 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add test for newHashSet and newLinkedHashSet
>
> src/java.base/share/classes/java/util/HashSet.java line 398:
>
>> 396: public static HashSet newHashSet(int numItems) {
>> 397: return new
>> HashSet<>(HashMap.calculateHashMapCapacity(numItems));
>> 398: }
>
> Please use "elements" instead of "items" throughout the specifications for
> the objects that are members of the HashSet. This includes the API notes
> above as well as the specs and API notes in LinkedHashSet.
@stuart-marks done.
-
PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8284780: Need methods to create pre-sized HashSet and LinkedHashSet [v11]
> as title. XenoAmess has updated the pull request incrementally with one additional commit since the last revision: rename items to elements - Changes: - all: https://git.openjdk.java.net/jdk/pull/8302/files - new: https://git.openjdk.java.net/jdk/pull/8302/files/56d029f4..4ec6ba9f Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=10 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8302&range=09-10 Stats: 12 lines in 2 files changed: 0 ins; 0 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/8302.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8302/head:pull/8302 PR: https://git.openjdk.java.net/jdk/pull/8302
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 15:25:32 GMT, Ioi Lam wrote:
>> Maxim Kartashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed unnecessary null checks
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
> line 113:
>
>> 111: CgroupInfo anyController = infos.values().iterator().next();
>> 112: CgroupSubsystem subsystem =
>> CgroupV2Subsystem.getInstance(anyController);
>> 113: return new CgroupMetrics(subsystem);
>
> Should add `Objects.requireNonNull(anyController)` and
> `Objects.requireNonNull(subsystem)`.
I added the first, but not the second as that looks like an overkill; see the
definition of the constructor:
CgroupMetrics(CgroupSubsystem subsystem) {
this.subsystem = Objects.requireNonNull(subsystem);
}
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 15:23:35 GMT, Ioi Lam wrote:
>> Maxim Kartashev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Removed unnecessary null checks
>
> src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
> line 116:
>
>> 114: } else {
>> 115: CgroupV1Subsystem subsystem =
>> CgroupV1Subsystem.getInstance(infos);
>> 116: return new CgroupV1MetricsImpl(subsystem);
>
> This shouldn't be changed because the current implementation of
> `CgroupV1Subsystem.getInstance(infos)` has a path that returns null.
>
> Maybe that's impossible, because when we call
> `CgroupV1Subsystem.getInstance`, we must have at least one v1 subsystem in
> `infos`. However, that's not related to this issue. Please fix that in a
> separate RFE. For example, `CgroupV1Subsystem.getInstance(infos)` can be
> changed to throw an exception instead if return null.
Fine by me; done.
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v3]
> Following the logic from the comment directly above the changed line, since > it doesn't matter which controller we pick, pick any available controller > instead of the one called "memory" specifically. This way we are guarded > against getting `null` as `anyController`, which is being immediately passed > down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` > values. > > It is also worth noting that the previous checks (such as that at line 89) > make sure that there exist at least one controller in the map. Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Made requested changes - Changes: - all: https://git.openjdk.java.net/jdk/pull/8803/files - new: https://git.openjdk.java.net/jdk/pull/8803/files/932ff6d1..9e6c0f37 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8803&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8803&range=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8803/head:pull/8803 PR: https://git.openjdk.java.net/jdk/pull/8803
Integrated: 8287187: Utilize HashMap.newHashMap() in CLDRConverter
On Wed, 25 May 2022 16:43:59 GMT, Naoto Sato wrote: > Refactoring the leftover self-calculations of the optimized `HashMap` initial > value with `newHashMap()` method. Also replaced some string literals using > text blocks for better readability. Confirmed that the output resource bundle > sources are effectively (sans indentation) the same as the original ones. This pull request has now been integrated. Changeset: c10749a6 Author:Naoto Sato URL: https://git.openjdk.java.net/jdk/commit/c10749a6a70977fbd6cd33b298410d212276fcf1 Stats: 65 lines in 1 file changed: 8 ins; 1 del; 56 mod 8287187: Utilize HashMap.newHashMap() in CLDRConverter Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/8887
Re: RFR: 8284638: store skip buffers in InputStream Object [v15]
On Wed, 25 May 2022 23:23:13 GMT, Sergey Kuksenko wrote: > Is there any practical scenario where the current code (skip buffer > allocation on each invocation) creates issues? @kuksenko Not found any yet :) - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 09:42:22 GMT, Maxim Kartashev
wrote:
>> Following the logic from the comment directly above the changed line, since
>> it doesn't matter which controller we pick, pick any available controller
>> instead of the one called "memory" specifically. This way we are guarded
>> against getting `null` as `anyController`, which is being immediately passed
>> down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept
>> `null` values.
>>
>> It is also worth noting that the previous checks (such as that at line 89)
>> make sure that there exist at least one controller in the map.
>
> Maxim Kartashev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Removed unnecessary null checks
Changes requested by iklam (Reviewer).
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
line 113:
> 111: CgroupInfo anyController = infos.values().iterator().next();
> 112: CgroupSubsystem subsystem =
> CgroupV2Subsystem.getInstance(anyController);
> 113: return new CgroupMetrics(subsystem);
Should add `Objects.requireNonNull(anyController)` and
`Objects.requireNonNull(subsystem)`.
src/java.base/linux/classes/jdk/internal/platform/CgroupSubsystemFactory.java
line 116:
> 114: } else {
> 115: CgroupV1Subsystem subsystem =
> CgroupV1Subsystem.getInstance(infos);
> 116: return new CgroupV1MetricsImpl(subsystem);
This shouldn't be changed because the current implementation of
`CgroupV1Subsystem.getInstance(infos)` has a path that returns null.
Maybe that's impossible, because when we call `CgroupV1Subsystem.getInstance`,
we must have at least one v1 subsystem in `infos`. However, that's not related
to this issue. Please fix that in a separate RFE. For example,
`CgroupV1Subsystem.getInstance(infos)` can be changed to throw an exception
instead if return null.
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v9]
On Thu, 26 May 2022 03:48:31 GMT, Kim Barrett wrote:
>> Yasumasa Suenaga has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - Change Array::data() implementation
>> - Avoid stringop-overflow warning in jfrTraceIdBits.inline.hpp
>
> src/java.base/unix/native/libjli/java_md_common.c line 133:
>
>> 131:
>> 132: snprintf_result = JLI_Snprintf(name, sizeof(name), "%s%c%s", indir,
>> FILE_SEPARATOR, cmd);
>> 133: if ((snprintf_result < 0) && (snprintf_result >=
>> (int)sizeof(name))) {
>
> That should be `||` rather than `&&`.
Good catch! I fixed it in new commit.
> src/java.base/unix/native/libjli/java_md_common.c line 135:
>
>> 133: if ((snprintf_result < 0) && (snprintf_result >=
>> (int)sizeof(name))) {
>> 134: return 0;
>> 135: }
>
> Pre-existing: It seems odd that this returns `0` above and below, rather than
> returning `NULL`. I think there are one or two other places in this file
> that are similarly using literal `0` for a null pointer, though others are
> using `NULL`. Something to report and clean up separately from this change.
I was wondering about that too.
I use `NULL` at L134, and have filed it as
[JDK-8287363](https://bugs.openjdk.java.net/browse/JDK-8287363). I will work
for it after this issue.
-
PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8286562: GCC 12 reports some compiler warnings [v10]
> I saw some compiler warnings when I tried to build OpenJDK with GCC 12.0.1 on > Fedora 36. > As you can see, the warnings spreads several areas. Let me know if I should > separate them by area. > > * -Wstringop-overflow > * src/hotspot/share/oops/array.hpp > * > src/hotspot/share/jfr/recorder/checkpoint/types/traceid/jfrTraceIdBits.inline.hpp > > In member function 'void Array::at_put(int, const T&) [with T = unsigned > char]', > inlined from 'void ConstantPool::tag_at_put(int, jbyte)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:126:64, > inlined from 'void ConstantPool::method_at_put(int, int, int)' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/oops/constantPool.hpp:380:15, > inlined from 'ConstantPool* > BytecodeConstantPool::create_constant_pool(JavaThread*) const' at > /home/ysuenaga/github-forked/jdk/src/hotspot/share/classfile/bytecodeAssembler.cpp:85:26: Yasumasa Suenaga has updated the pull request incrementally with one additional commit since the last revision: Fix comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/8646/files - new: https://git.openjdk.java.net/jdk/pull/8646/files/17cda224..851279ae Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8646&range=08-09 Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/8646.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8646/head:pull/8646 PR: https://git.openjdk.java.net/jdk/pull/8646
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
> Following the logic from the comment directly above the changed line, since > it doesn't matter which controller we pick, pick any available controller > instead of the one called "memory" specifically. This way we are guarded > against getting `null` as `anyController`, which is being immediately passed > down to `CgroupV2Subsystem.getInstance()` that is unprepared to accept `null` > values. > > It is also worth noting that the previous checks (such as that at line 89) > make sure that there exist at least one controller in the map. Maxim Kartashev has updated the pull request incrementally with one additional commit since the last revision: Removed unnecessary null checks - Changes: - all: https://git.openjdk.java.net/jdk/pull/8803/files - new: https://git.openjdk.java.net/jdk/pull/8803/files/f17af433..932ff6d1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8803&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8803&range=00-01 Stats: 2 lines in 1 file changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/8803.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8803/head:pull/8803 PR: https://git.openjdk.java.net/jdk/pull/8803
Re: RFR: 8287073: NPE from CgroupV2Subsystem.getInstance() [v2]
On Thu, 26 May 2022 06:28:22 GMT, Peter Levart wrote:
>> @plevart Are you asking about the reason for the crash or about the changes?
>> If it's the former, then I believe that the crash comes not from
>> `getInstance()` returning `null`, but from further down the stack because
>> `null` is being passed to `getInstance()`. I could be wrong in interpreting
>> the report, though.
>>
>> If the question's about the changes, then those are restricted to CgroupV2,
>> so I'm not sure how `CgroupV1Subsystem.getInstance(...)` returning null is
>> related. FWIW, I also don't think we are going to get here if there are no
>> active controllers. There's this code a few lines above:
>>
>> if (!result.isAnyControllersEnabled()) {
>> return null;
>> }
>
> I was just contemplating the code around the change as it appears to have
> unnecessary checks which result in dead code. From the point of fixing just
> this concrete NPE, they are irrelevant. So while this code might benefit from
> cleanup, perhaps this PR is not the place to do it. Perhaps it is a matter of
> another issue and PR.
@plevart I think I now understand what you meant and removed the unnecessary
checks. Please, have a look.
-
PR: https://git.openjdk.java.net/jdk/pull/8803
Integrated: 8287244: Add bound check in indexed memory access var handle
On Tue, 24 May 2022 14:40:56 GMT, Maurizio Cimadamore wrote: > Constructing indexed var handles using the `MemoryLayout` API produces > `VarHandle` which do not check the input indices for out-of-bounds conditions. > While this can never result in a VM crash (after all the memory segment will > protect against "true" OOB access), it is still possible for an access > expression to refer to parts of a segment that are logically unrelated. > > This patch adds a "logical" bound check to all indexed var handles generated > using the layout API. > Benchmarks are not affected by the check. Users are still able to create > custom "unchecked" var handles, using the combinator API in `MethodHandles`. This pull request has now been integrated. Changeset: f58c9a65 Author:Maurizio Cimadamore URL: https://git.openjdk.java.net/jdk/commit/f58c9a659ba181407ecdb2aacb81e6a7f1cbd9ff Stats: 92 lines in 4 files changed: 70 ins; 5 del; 17 mod 8287244: Add bound check in indexed memory access var handle Reviewed-by: psandoz, jvernee - PR: https://git.openjdk.java.net/jdk/pull/8868
Re: RFR: 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get
On Sat, 30 Apr 2022 08:56:23 GMT, Andrey Turbanov wrote: > The method `java.util.zip.ZipFile.Source#get` could be improved by usage of > `Map.putIfAbsent` instead of separate `containsKey`/`get`/`put` calls. We > known that HashMap `java.util.zip.ZipFile.Source#files` can contain only > non-null values. And to check if putIfAbsent was successful or not we can > just compare result with `null`. > It makes code a bit cleaner and faster. Marked as reviewed by alanb (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/8481
Withdrawn: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote: > This is a draft proposal for how we could improve stream performance for the > case where the streams are empty. Empty collections are common-place. If we > iterate over them with an Iterator, we would have to create one small > Iterator object (which could often be eliminated) and if it is empty we are > done. However, with Streams we first have to build up the entire pipeline, > until we realize that there is no work to do. With this example, we change > Collection#stream() to first check if the collection is empty, and if it is, > we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream > and EmptyDoubleStream. We have taken great care for these to have the same > characteristics and behaviour as the streams returned by Stream.empty(), > IntStream.empty(), etc. > > Some of the JDK tests fail with this, due to ClassCastExceptions (our > EmptyStream is not an AbstractPipeline) and AssertionError, since we can call > some methods repeatedly on the stream without it failing. On the plus side, > creating a complex stream on an empty stream gives us upwards of 50x increase > in performance due to a much smaller object allocation rate. This PR includes > the code for the change, unit tests and also a JMH benchmark to demonstrate > the improvement. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: 8287285: Avoid redundant HashMap.containsKey call in java.util.zip.ZipFile.Source.get
On Sat, 30 Apr 2022 08:56:23 GMT, Andrey Turbanov wrote: > The method `java.util.zip.ZipFile.Source#get` could be improved by usage of > `Map.putIfAbsent` instead of separate `containsKey`/`get`/`put` calls. We > known that HashMap `java.util.zip.ZipFile.Source#files` can contain only > non-null values. And to check if putIfAbsent was successful or not we can > just compare result with `null`. > It makes code a bit cleaner and faster. The change looks fine to me. Please wait for at least one more review from someone having more knowledge of this area, before merging. - Marked as reviewed by jpai (Committer). PR: https://git.openjdk.java.net/jdk/pull/8481
