RFR: JDK-8275187: Suppress warnings on non-serializable array component types in java.sql.rowset
After a refinement to the checks under development in #5709, the new checks examine array types of serial fields and warn if the underlying component type is not serializable. Per the JLS, all array types are serializable, but if the base component is not serializable, the serialization process can throw an exception. >From "Java Object Serialization Specification: 2 - Object Output Classes": "If the object is an array, writeObject is called recursively to write the ObjectStreamClass of the array. The handle for the array is assigned. It is followed by the length of the array. Each element of the array is then written to the stream, after which writeObject returns." The java.sql.rowset module has several instances of this coding pattern that need suppression to compile successfully under the future warning. - Commit messages: - JDK-8275187: Suppress warnings on non-serializable array component types in java.sql.rowset Changes: https://git.openjdk.java.net/jdk/pull/5925/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5925=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275187 Stats: 5 lines in 3 files changed: 3 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5925.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5925/head:pull/5925 PR: https://git.openjdk.java.net/jdk/pull/5925
Integrated: JDK-8275186: Suppress warnings on non-serializable array component types in xml
On Wed, 13 Oct 2021 04:49:22 GMT, Joe Darcy wrote: > After a refinement to the checks under development in > https://github.com/openjdk/jdk/pull/5709, the new checks examine array types > of serial fields and warn if the underlying component type is not > serializable. Per the JLS, all array types are serializable, but if the base > component is not serializable, the serialization process can throw an > exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." This pull request has now been integrated. Changeset: ab34cced Author:Joe Darcy URL: https://git.openjdk.java.net/jdk/commit/ab34cced3beae765fe9d6b6acfef7e6a7f3082cd Stats: 11 lines in 6 files changed: 6 ins; 0 del; 5 mod 8275186: Suppress warnings on non-serializable array component types in xml Reviewed-by: joehw - PR: https://git.openjdk.java.net/jdk/pull/5924
Re: RFR: JDK-8275186: Suppress warnings on non-serializable array component types in xml [v2]
> After a refinement to the checks under development in > https://github.com/openjdk/jdk/pull/5709, the new checks examine array types > of serial fields and warn if the underlying component type is not > serializable. Per the JLS, all array types are serializable, but if the base > component is not serializable, the serialization process can throw an > exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." Joe Darcy has updated the pull request incrementally with one additional commit since the last revision: Update LastModified info. - Changes: - all: https://git.openjdk.java.net/jdk/pull/5924/files - new: https://git.openjdk.java.net/jdk/pull/5924/files/c184ee63..41068620 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5924=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5924=00-01 Stats: 3 lines in 3 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/5924.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5924/head:pull/5924 PR: https://git.openjdk.java.net/jdk/pull/5924
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment Change in test/jdk/jdk/jfr/api/consumer/TestHiddenMethod.java looks good. - Marked as reviewed by egahlin (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v2]
On Tue, 12 Oct 2021 20:51:02 GMT, Maurizio Cimadamore
wrote:
>> This PR contains the API and implementation changes for JEP-419 [1]. A more
>> detailed description of such changes, to avoid repetitions during the review
>> process, is included as a separate comment.
>>
>> [1] - https://openjdk.java.net/jeps/419
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Fix TestLinkToNativeRBP test
Like with previous reviews of code for Panama JEPs there are not many comments
on this PR, since prior reviews occurred for PRs in the panama-foreign repo.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
77:
> 75: * Furthermore, if the function descriptor's return layout is a group
> layout, the resulting downcall method handle accepts
> 76: * an extra parameter of type {@link SegmentAllocator}, which is used by
> the linker runtime to allocate the
> 77: * memory region associated with the struct returned by the downcall
> method handle.
Suggestion:
* memory region associated with the struct returned by the downcall method
handle.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
88:
> 86: * in which the specialized signature of a given variable arity callsite
> is described in full. Alternatively,
> 87: * if the foreign library allows it, clients might also be able to
> interact with variable arity methods
> 88: * using by passing a trailing parameter of type {@link VaList}.
Suggestion:
* by passing a trailing parameter of type {@link VaList}.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/CLinker.java line
145:
> 143: * Lookup a symbol in the standard libraries associated with this
> linker.
> 144: * The set of symbols available for lookup is unspecified, as it
> depends on the platform and on the operating system.
> 145: * @return a linker-specific library lookup which is suitable to
> find symbols in the standard libraries associated with this linker.
Suggestion:
* @return a symbol in the standard libraries associated with this linker.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/FunctionDescriptor.java
line 93:
> 91: Objects.requireNonNull(argLayouts);
> 92: Arrays.stream(argLayouts).forEach(Objects::requireNonNull);
> 93: return new FunctionDescriptor(null, argLayouts);
We need to clone `argLayouts`, otherwise the user can modify the contents.
Internally, using `List.of` is useful, since it does the cloning and null
checks, and that is the type that is returned. Also `argumentLayouts` uses
`Arrays.asList` which supports modification of the contents.
src/jdk.incubator.foreign/share/classes/jdk/incubator/foreign/SegmentAllocator.java
line 394:
> 392: *
> 393: * The returned allocator might throw an {@link OutOfMemoryError} if
> the total memory allocated with this allocator
> 394: * exceeds the arena size, or the system capacity. Furthermore, the
> returned allocator is not thread safe, and all
The "the returned allocator is not thread safe" contradicts the prior sentence
"An allocator associated with a shared resource scope is thread-safe
and allocation requests may be performed concurrently".
Perhaps just say:
"
The returned allocator is not thread safe if the given scope is a shared scope.
Concurrent allocation needs to be guarded with synchronization primitives.
"
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/AbstractMemorySegmentImpl.java
line 260:
> 258:
> 259: @Override
> 260: public final MemorySegment asOverlappingSlice(MemorySegment other) {
Please ignore these comments if you wish.
Adding `max() // exclusive` to complement `min()` might be useful.
In these cases i find it easier to sort the segments e.g. `var a = this; var b
= other; if (a.min() > b.min()) { // swap a and b }` then the subsequent logic
tends to get simpler e.g. overlap test is `if (b.min() < a.max())`,
`segmentOffset` is always +ve.
src/jdk.incubator.foreign/share/classes/jdk/internal/foreign/ConfinedScope.java
line 100:
> 98: do {
> 99: value = (int)ASYNC_RELEASE_COUNT.getVolatile(this);
> 100: } while (!ASYNC_RELEASE_COUNT.compareAndSet(this, value,
> value + 1));
Use `getAndAdd` (and ignore the return value).
-
PR: https://git.openjdk.java.net/jdk/pull/5907
RFR: JDK-8275186: Suppress warnings on non-serializable array component types in xml
After a refinement to the checks under development in https://github.com/openjdk/jdk/pull/5709, the new checks examine array types of serial fields and warn if the underlying component type is not serializable. Per the JLS, all array types are serializable, but if the base component is not serializable, the serialization process can throw an exception. >From "Java Object Serialization Specification: 2 - Object Output Classes": "If the object is an array, writeObject is called recursively to write the ObjectStreamClass of the array. The handle for the array is assigned. It is followed by the length of the array. Each element of the array is then written to the stream, after which writeObject returns." - Commit messages: - JDK-8275186: Suppress warnings on non-serializable array component types in xml Changes: https://git.openjdk.java.net/jdk/pull/5924/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5924=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275186 Stats: 8 lines in 6 files changed: 6 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5924.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5924/head:pull/5924 PR: https://git.openjdk.java.net/jdk/pull/5924
Re: RFR: JDK-8275186: Suppress warnings on non-serializable array component types in xml
On Wed, 13 Oct 2021 04:49:22 GMT, Joe Darcy wrote: > After a refinement to the checks under development in > https://github.com/openjdk/jdk/pull/5709, the new checks examine array types > of serial fields and warn if the underlying component type is not > serializable. Per the JLS, all array types are serializable, but if the base > component is not serializable, the serialization process can throw an > exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." > > > I understand you'd update the header before push. Pls note that there's a > "@lastmodified" field that would need to be updated as well. Thanks. Hi Joe, I ran my copyright update script this time before sending out the review; I'll check for the "@lastmodified" fields before pushing. Thanks, -Joe - PR: https://git.openjdk.java.net/jdk/pull/5924
Re: RFR: JDK-8275186: Suppress warnings on non-serializable array component types in xml
On Wed, 13 Oct 2021 04:49:22 GMT, Joe Darcy wrote: > After a refinement to the checks under development in > https://github.com/openjdk/jdk/pull/5709, the new checks examine array types > of serial fields and warn if the underlying component type is not > serializable. Per the JLS, all array types are serializable, but if the base > component is not serializable, the serialization process can throw an > exception. > > From "Java Object Serialization Specification: 2 - Object Output Classes": > > "If the object is an array, writeObject is called recursively to write the > ObjectStreamClass of the array. The handle for the array is assigned. It is > followed by the length of the array. Each element of the array is then > written to the stream, after which writeObject returns." Marked as reviewed by joehw (Reviewer). I understand you'd update the header before push. Pls note that there's a "@LastModified" field that would need to be updated as well. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5924
Integrated: 8268764: Use Long.hashCode() instead of int-cast where applicable
On Tue, 15 Jun 2021 12:15:11 GMT, Сергей Цыпанов wrote:
> In some JDK classes there's still the following hashCode() implementation:
>
> long objNum;
>
> public int hashCode() {
> return (int) objNum;
> }
>
> This outdated expression should be replaced with Long.hashCode(long) as it
>
> - uses all bits of the original value, does not discard any information
> upfront. For example, depending on how you are generating the IDs, the upper
> bits could change more frequently (or the opposite).
>
> - does not introduce any bias towards values with more ones (zeros), as it
> would be the case if the two halves were combined with an OR (AND) operation.
>
> See https://stackoverflow.com/a/4045083
>
> This is related to https://github.com/openjdk/jdk/pull/4309
This pull request has now been integrated.
Changeset: 124f8237
Author:Sergey Tsypanov
Committer: Sergey Bylokhov
URL:
https://git.openjdk.java.net/jdk/commit/124f82377ba93359bc59118ee315ba194080fa92
Stats: 21 lines in 9 files changed: 6 ins; 0 del; 15 mod
8268764: Use Long.hashCode() instead of int-cast where applicable
Reviewed-by: kevinw, prr, kizune, serb
-
PR: https://git.openjdk.java.net/jdk/pull/4491
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v7]
On Thu, 3 Jun 2021 20:49:44 GMT, Maurizio Cimadamore
wrote:
>> This patch overhauls the library loading mechanism used by the Foreign
>> Linker API. We realized that, while handy, the *default* lookup abstraction
>> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms.
>>
>> This patch replaces `LibraryLookup` with a simpler `SymbolLookup`
>> abstraction, a functional interface. Crucially, `SymbolLookup` does not
>> concern with library loading, only symbol lookup. For this reason, two
>> factories are added:
>>
>> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to
>> lookup symbols in libraries loaded by current loader
>> * `CLinker::systemLookup` - a more stable replacement for the *default*
>> lookup, which looks for symbols in libc.so (or its equivalent in other
>> platforms). The contents of this lookup are unspecified.
>>
>> Both factories are *restricted*, so they can only be called when
>> `--enable-native-access` is set.
>
> Maurizio Cimadamore has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Address review comments on shim lib makefile
Just tried with `System.load()` but still ended up with pretty much the same
failure given both of them eventually invokes `ClassLoader.loadLibrary` to load
the library in which case there is no big difference at this point.
static {
System.load("/usr/lib/libc.a");
}
Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load
/usr/lib/libc.a <
at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1793)
at java.base/java.lang.System.load(System.java:672)
at StdLibTest.(StdLibTest.java:24)
-
PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8177814: jdk/editpad is not in jdk TEST.groups
On Thu, 23 Sep 2021 08:54:48 GMT, Aleksey Shipilev wrote: > @prrace notices this here: > https://github.com/openjdk/jdk/pull/5544#issuecomment-925396869. And I think > it is the already open issue that this patch is fixing. While the original > patch added the test in `jdk_other`, Phil suggests it to be added to > `jdk_desktop`. > > Additional testing: > - [x] `jdk_editpad` is passing Let's fix it this way - Marked as reviewed by serb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5648
Re: RFR: 8177814: jdk/editpad is not in jdk TEST.groups
On Thu, 23 Sep 2021 08:54:48 GMT, Aleksey Shipilev wrote: > @prrace notices this here: > https://github.com/openjdk/jdk/pull/5544#issuecomment-925396869. And I think > it is the already open issue that this patch is fixing. While the original > patch added the test in `jdk_other`, Phil suggests it to be added to > `jdk_desktop`. > > Additional testing: > - [x] `jdk_editpad` is passing Ok, thanks! - PR: https://git.openjdk.java.net/jdk/pull/5648
Integrated: 8177814: jdk/editpad is not in jdk TEST.groups
On Thu, 23 Sep 2021 08:54:48 GMT, Aleksey Shipilev wrote: > @prrace notices this here: > https://github.com/openjdk/jdk/pull/5544#issuecomment-925396869. And I think > it is the already open issue that this patch is fixing. While the original > patch added the test in `jdk_other`, Phil suggests it to be added to > `jdk_desktop`. > > Additional testing: > - [x] `jdk_editpad` is passing This pull request has now been integrated. Changeset: cfe7471f Author:Aleksey Shipilev URL: https://git.openjdk.java.net/jdk/commit/cfe7471f1769eca2a4e623f5ba9cddceb005f0bf Stats: 5 lines in 1 file changed: 4 ins; 0 del; 1 mod 8177814: jdk/editpad is not in jdk TEST.groups Reviewed-by: serb - PR: https://git.openjdk.java.net/jdk/pull/5648
Re: RFR: 8268764: Use Long.hashCode() instead of int-cast where applicable [v4]
On Thu, 1 Jul 2021 12:19:53 GMT, Сергей Цыпанов wrote:
>> In some JDK classes there's still the following hashCode() implementation:
>>
>> long objNum;
>>
>> public int hashCode() {
>> return (int) objNum;
>> }
>>
>> This outdated expression should be replaced with Long.hashCode(long) as it
>>
>> - uses all bits of the original value, does not discard any information
>> upfront. For example, depending on how you are generating the IDs, the upper
>> bits could change more frequently (or the opposite).
>>
>> - does not introduce any bias towards values with more ones (zeros), as it
>> would be the case if the two halves were combined with an OR (AND) operation.
>>
>> See https://stackoverflow.com/a/4045083
>>
>> This is related to https://github.com/openjdk/jdk/pull/4309
>
> Сергей Цыпанов has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8268764: Update copy-right year
Marked as reviewed by serb (Reviewer).
-
PR: https://git.openjdk.java.net/jdk/pull/4491
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote: >> Hi all, >> >> Please review this fix for Infinite loop in ZipOutputStream.close(). >> The main issue here is when ever there is an exception during close >> operations on GZip we are not setting the deflator to a finished state which >> is leading to an infinite loop when we try writing on the same GZip >> instance( since we use while(!def.finished()) inside the write operation). >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > 8193682 : Infinite loop in ZipOutputStream.close() Setting to "Request changes" until the spec impact is understood. - Changes requested by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:10:53 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>
>> 250: int len = def.deflate(buf, 0, buf.length);
>> 251: if (len > 0) {
>> 252: try {
>
> Shouldn't this use try with resources:
> try (out) { ...
the output stream is only closed if an exception is raised though ?
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote: >> Hi all, >> >> Please review this fix for Infinite loop in ZipOutputStream.close(). >> The main issue here is when ever there is an exception during close >> operations on GZip we are not setting the deflator to a finished state which >> is leading to an infinite loop when we try writing on the same GZip >> instance( since we use while(!def.finished()) inside the write operation). >> >> Thanks, >> Ravi > > Ravi Reddy has updated the pull request incrementally with one additional > commit since the last revision: > > 8193682 : Infinite loop in ZipOutputStream.close() test/jdk/java/util/zip/GZIP/GZipLoopTest.java line 55: > 53: private static Random rand = new Random(); > 54: > 55: @Test I think we can condense the test code to aid maintenance - please consider using DataProvider - PR: https://git.openjdk.java.net/jdk/pull/5522
Integrated: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
On Sat, 9 Oct 2021 17:54:16 GMT, Andrey Turbanov wrote: > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE This pull request has now been integrated. Changeset: 7d2633f7 Author:Andrey Turbanov Committer: Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/7d2633f795c27edc2dfbbd7a9d9e44bdb23ec6a1 Stats: 10 lines in 1 file changed: 0 ins; 8 del; 2 mod 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE Reviewed-by: prappo, jlaskey, martin - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE [v3]
On Mon, 11 Oct 2021 21:07:21 GMT, Andrey Turbanov wrote: >> 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE > avoid link in private javadoc Marked as reviewed by prappo (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5878
Re: RFR: 8245095: Implementation of JEP 408: Simple Web Server [v9]
> This change implements a simple web server that can be run on the > command-line with `java -m jdk.httpserver`. > > This is facilitated by adding an entry point for the `jdk.httpserver` module, > an implementation class whose main method is run when the above command is > executed. This is the first such module entry point in the JDK. > > The server is a minimal HTTP server that serves the static files of a given > directory, similar to existing alternatives on other platforms and convenient > for testing, development, and debugging. > > Additionally, a small API is introduced for programmatic creation and > customization. > > Testing: tier1-3. Julia Boes has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 24 commits: - Minor rewording of bind address output - Merge branch 'master' into simpleserver - Merge branch 'master' into simpleserver - update output for all interfaces - use ipv4/ipv6 specific loopback address and add add how-to output for all interfaces - Merge branch 'master' into simpleserver - change default bind address from anylocal to loopback - address PR comments - Merge branch 'master' into simpleserver - Merge remote-tracking branch 'origin/simpleserver' into simpleserver - ... and 14 more: https://git.openjdk.java.net/jdk/compare/b1b66965...e86609d0 - Changes: https://git.openjdk.java.net/jdk/pull/5505/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5505=08 Stats: 7181 lines in 42 files changed: 7146 ins; 15 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/5505.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5505/head:pull/5505 PR: https://git.openjdk.java.net/jdk/pull/5505
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 13:28:21 GMT, Ravi Reddy wrote:
>> Hi all,
>>
>> Please review this fix for Infinite loop in ZipOutputStream.close().
>> The main issue here is when ever there is an exception during close
>> operations on GZip we are not setting the deflator to a finished state which
>> is leading to an infinite loop when we try writing on the same GZip
>> instance( since we use while(!def.finished()) inside the write operation).
>>
>> Thanks,
>> Ravi
>
> Ravi Reddy has updated the pull request incrementally with one additional
> commit since the last revision:
>
> 8193682 : Infinite loop in ZipOutputStream.close()
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
> 250: int len = def.deflate(buf, 0, buf.length);
> 251: if (len > 0) {
> 252: try {
Shouldn't this use try with resources:
try (out) { ...
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 254:
> 252: try {
> 253: out.write(buf, 0, len);
> 254: } catch (Exception e) {
Shouldn't this be a finally block?
src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
> 254: } catch (Exception e) {
> 255: def.end();
> 256: out.close();
out.close not needed with try with resources.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:11:15 GMT, jmehrens wrote:
>> Ravi Reddy has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> 8193682 : Infinite loop in ZipOutputStream.close()
>
> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>
>> 254: } catch (Exception e) {
>> 255: def.end();
>> 256: out.close();
>
> out.close not needed with try with resources.
This changes deflate to close the compressor and the output stream when there
is an I/O exception. I expect this will need a spec change or a re-examination
of the issue to see if there are alternatives.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 15:00:11 GMT, Ravi Reddy wrote: >> the output stream is only closed if an exception is raised though ? > > Yes , we are closing the stream only when exception occurs during write > operation Yes, we are closing the stream only when an exception occurs during a write operation - PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Fri, 8 Oct 2021 21:45:21 GMT, Cheng Jin wrote: > That's what I thought to be the only way around but might need to figure out > the specifics on AIX. Is `libc.a` loadable on AIX (e.g. with System.loadLibrary) ? (Sorry I don't have a machine to test readily available). If so, we might just load `libc` and `libm` and drop the shim library generation on AIX. - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: 8193682 : Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/f6a678ed..d18eb3c1 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=01-02 Stats: 46 lines in 1 file changed: 44 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:35:17 GMT, Sean Coffey wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 252:
>>
>>> 250: int len = def.deflate(buf, 0, buf.length);
>>> 251: if (len > 0) {
>>> 252: try {
>>
>> Shouldn't this use try with resources:
>> try (out) { ...
>
> the output stream is only closed if an exception is raised though ?
Yes , we are closing the stream only when exception occurs during write
operation
-
PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Tue, 12 Oct 2021 15:04:02 GMT, Maurizio Cimadamore
wrote:
> Is libc.a loadable on AIX (e.g. with System.loadLibrary) ?
I tried to load `libc.a` and `libc` this way but neither of them works on AIX.
e.g.
public class StdLibTest {
private static CLinker clinker = CLinker.getInstance();
static {
System.loadLibrary("libc.a"); <-
}
private static final SymbolLookup defaultLibLookup =
SymbolLookup.loaderLookup();
public static void main(String args[]) throws Throwable {
Addressable strlenSymbol =
defaultLibLookup.lookup("strlen").get();
}
}
$ ./bin/java --enable-native-access=ALL-UNNAMED --add-modules
jdk.incubator.foreign
-Dforeign.restricted=permit --enable-native-access=ALL-UNNAMED StdLibTest
WARNING: Using incubator modules: jdk.incubator.foreign
Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load libc.a
<---
at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1822)
at java.base/java.lang.System.loadLibrary(System.java:694)
at StdLibTest.(StdLibTest.java:23)
and
public class StdLibTest {
private static CLinker clinker = CLinker.getInstance();
static {
System.loadLibrary("libc"); <---
}
private static final SymbolLookup defaultLibLookup =
SymbolLookup.loaderLookup();
public static void main(String args[]) throws Throwable {
Addressable strlenSymbol =
defaultLibLookup.lookup("strlen").get();
}
}
$ ./bin/java --enable-native-access=ALL-UNNAMED --add-modules
jdk.incubator.foreign
-Dforeign.restricted=permit --enable-native-access=ALL-UNNAMED StdLibTest
WARNING: Using incubator modules: jdk.incubator.foreign
Exception in thread "main" java.lang.UnsatisfiedLinkError: Can't load libc
<--
at java.base/java.lang.ClassLoader.loadLibrary(ClassLoader.java:1822)
at java.base/java.lang.System.loadLibrary(System.java:694)
at StdLibTest.(StdLibTest.java:23)
-
PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: JDK-8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior
On Mon, 11 Oct 2021 20:55:23 GMT, Mandy Chung wrote: > Classes compiled prior to the nestmate support will generate > `REF_invokeSpecial` if the implementation method is a private instance > method. Since a lambda proxy class is a hidden class, a nestmate of the > host class, it can invoke the private implementation method but it has to use > `REF_invokeVirtual` or `REF_invokeInterface`. In order to support the old > classes running on the new runtime, LMF implementation adjusts the reference > kind from `REF_invokeSpecial` to `REF_invokeVirtual/REF_invokeInterface`. > > This PR fixes the check properly to ensure the adjustment is only made if the > implementation method is private method in the host class. filelist line 1: > 1: test/jdk/java/lang/invoke/lambda/invokeSpecial/InvokeSpecialMethodImpl.java I assume "filelist" is not meant to be in this match, I assume this is why the "jdk" label was added. - PR: https://git.openjdk.java.net/jdk/pull/5901
Questions for JDK-4947890
I have some questions for JDK-4947890。 One of app’s JVM was upgraded from JDK11 to JDK17. Then the working behavior was changed on Windows 10 Multilingual User Interface (MUI). (Base was Japanese Windows 10, display language setting was English (United States)) In my investigation, the working behavior was changed by JDK12+b23. JDK-4947890 Minimize JNI upcalls in system-properties initialization [1] Following change was applied on src/java.base/share/native/libjava/System.c, From (not for MacOS platform) [2] Put sprops->sun_jnu_encoding into file.encoding system property To [3] Put sprops->encoding into file.encoding system property I checked JDK-4947890’s CSR JDK-8213895 [4] Modify JVM interface functions for property initialization But it seems that above CSR does not mention such a significant specification change. I checked JDK17u, same code was still used [5]. Questions: I’d like to confirm * This change was intentional or not ? * We can revert tp JDK11’s spec ? Thanks, Ichiroh Takiguchi [1] https://bugs.openjdk.java.net/browse/JDK-4947890 [2] http://hg.openjdk.java.net/jdk/jdk/rev/0bdbf854472f#l12.466 [3] http://hg.openjdk.java.net/jdk/jdk/rev/0bdbf854472f#l12.276 [4] https://bugs.openjdk.java.net/browse/JDK-8213895 [5] https://github.com/openjdk/jdk17u/blob/master/src/java.base/share/native/libjava/System.c#L149
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v3]
On Tue, 12 Oct 2021 14:46:33 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/util/zip/DeflaterOutputStream.java line 256:
>>
>>> 254: } catch (Exception e) {
>>> 255: def.end();
>>> 256: out.close();
>>
>> out.close not needed with try with resources.
>
> This changes deflate to close the compressor and the output stream when there
> is an I/O exception. I expect this will need a spec change or a
> re-examination of the issue to see if there are alternatives.
the out.close() call could be removed I guess. Leave it for user code to handle
etc. Safer for spec also.
Main goal is to break the looping of the deflate call. The usesDefaultDeflater
boolean might be useful in helping determine if def.end() should be called or
not. If that boolean is false, then maybe we could just alter the input buffer
by setting it to a size 0 buffer (ZipUtils.defaultBuf) -- worth a look.
-
PR: https://git.openjdk.java.net/jdk/pull/5522
RFR: 8275063: Implementation of Memory Access API (Second incubator)
This PR contains the API and implementation changes for JEP-419 [1]. A more detailed description of such changes, to avoid repetitions during the review process, is included as a separate comment. [1] - https://openjdk.java.net/jeps/419 - Commit messages: - Fix issues with Aarch64 VaList implementation - Drop argument type profiling for removed `MemoryAccess` class - Add missing files - Tweak javadoc for MemeorySegment/MemoryAddress::setUtf8String - Initial push from panama/foreign Changes: https://git.openjdk.java.net/jdk/pull/5907/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=5907=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8275063 Stats: 14262 lines in 186 files changed: 6583 ins; 5144 del; 2535 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8275063: Implementation of Memory Access API (Second incubator)
On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore
wrote:
> This PR contains the API and implementation changes for JEP-419 [1]. A more
> detailed description of such changes, to avoid repetitions during the review
> process, is included as a separate comment.
>
> [1] - https://openjdk.java.net/jeps/419
This PR contains mainly API changes. We have tried to simplify the API, by
removing redundant classes and moving functionalities where they belong. Below
we list the main changes introduced in this PR. A big thanks to all who helped
along the way: @briangoetz, @ChrisHegarty, @FrauBoes, @iwanowww, @JornVernee,
@PaulSandoz, @sundararajana and @rose00.
### Value layouts and carriers
This is perhaps the biggest change in the API, which has a knock-on effect in
other areas as well. In the past, a value layout used to be a fairly *neutral*
description of a piece of memory containing a scalar value. A value layout has
a size, alignment and endianness. Since a value layout contains no information
on *how* the value is to be dereferenced by Java clients, said clients have to
specify a *carrier* when obtaining a var handle from a value layout.
In this iteration, we have decided to attach carrier types to value layouts -
so, in addition to size, alignment and endianness, all value layouts now have a
`carrier` accessor, which returns a `j.l.Class`. You will find several
hand-specialized version of `ValueLayout`, one for each main carrier type (e.g.
`ValueLayout.OfInt` for the `int` carrier).
Attaching carrier information to layouts simplifies the API in many ways:
* When obtaining a var handle from a layout, it is no longer necessary to
provide a carrier; the layout in fact contains all the necessary information
for the dereference operation to occur.
* Similarly, when linking downcall method handles, the `MethodType` parameter
is no longer necessary, as now carrier information can be inferred from the
provided `FunctionDescriptor`.
* We can express dereference operations in a more general fashion - e.g.
`get(ValueLayout.OfInt)` instead of `getInt(ByteOrder)`. Note how the new form
is more *complete*.
This iteration also adds support for `boolean` and `MemoryAddress` carriers in
memory access var handles.
### Layout attributes
To help the `CLinker` distinguish between a 32-bit layout modelling to a C
`int` and a similar layout modelling a C `float` we have in the past resorted
to *layout attributes* - that is, we injected custom classification information
in layouts, and then required clients working with the `CLinker` API to only
work with such augmented layouts.
Because of the changes described above, this is no longer necessary: a layout
is always associated with a Java carrier, so the `CLinker` can always
disambiguate between `ValueLayout.OfInt` and `ValueLayout.OfFloat` even though
they have the same size. For this reason, API support for custom layout
attributes has been dropped in this iteration.
Similarly, platform-dependent layout constants in `CLinker` have been removed;
clients interacting with the foreign linker can simply use the basic layout
constants defined in `ValueLayout` (e.g. `JAVA_INT`, `JAVA_FLOAT`, ...) - which
is not too different from using the JNI types `jint` and `jfloat`. Of course
tools (such as `jextract`) are free to define *custom* layouts which model C
type for a specific platform.
### Memory dereference
Previous iterations of the API provided ready-made dereference operations as
static methods in the `MemoryAccess` class. This class is now removed, and all
the dereference operations have been moved to `MemorySegment` and
`MemoryAddress`. As mentioned before, the new dereference operations have a new
form. Instead of:
MemorySegment segment = ...
MemoryAccess.getIntAtOffset(segment, /*offset */ 100, /* endianness */
ByteOrder.nativeOrder());
The new API works as follows:
MemorySegment segment = ...
int val = segment.get(JAVA_INT, /*offset */ 100);
Note that the new dereference method is not static, and that parameters such as
endianness are now omitted, since clients can just specify the value layout
they want to work with. Also, since the new dereference methods are not static,
we no longer need the workaround to enable VM argument type profiling (this was
necessary to make static methods in `MemoryAccess` class perform reasonably
well in the face of profile pollution).
The same dereference operations are also available in `MemoryAddress`; when
working with native code it might be necessary to dereference a raw pointer. In
Java 17, to write a basic comparator function for qsort, the following code is
needed:
static int qsortCompare(MemoryAddress addr1, MemoryAddress addr2) {
return
MemoryAccess.getIntAtOffset(MemorySegment.globalNativeSegment(),
addr1.toRawLongValue()) -
MemoryAccess.getIntAtOffset(MemorySegment.globalNativeSegment(),
addr2.toRawLongValue());
With
Integrated: 8271737: Only normalize the cached user.dir property once
On Thu, 7 Oct 2021 14:05:19 GMT, Evgeny Astigeevich wrote: > The change completes the fix of > [JDK-8198997](https://bugs.openjdk.java.net/browse/JDK-8198997) which has > added normalisation in a constructor but not removed it from the get method. This pull request has now been integrated. Changeset: b8bd259b Author:Evgeny Astigeevich Committer: Paul Hohensee URL: https://git.openjdk.java.net/jdk/commit/b8bd259bb83096f8727222a4e5cd84e80e096275 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod 8271737: Only normalize the cached user.dir property once Reviewed-by: phh - PR: https://git.openjdk.java.net/jdk/pull/5850
Re: RFR: 8275063: Implementation of Memory Access API (Second incubator)
On Tue, 12 Oct 2021 11:16:51 GMT, Maurizio Cimadamore wrote: > This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Build changes look good. - Marked as reviewed by erikj (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
On Tue, 12 Oct 2021 15:24:41 GMT, Cheng Jin wrote:
> I tried to load `libc.a` and `libc` this way but neither of them works on AIX.
Sorry - what I meant is - `System::load`, which accepts a full path and
extension. E.g.
System.load("/usr/lib/libc.a");
-
PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8244202: Implementation of JEP 418: Internet-Address Resolution SPI [v3]
> This change implements a new service provider interface for host name and > address resolution, so that java.net.InetAddress API can make use of > resolvers other than the platform's built-in resolver. > > The following API classes are added to `java.net.spi` package to facilitate > this: > - `InetAddressResolverProvider` - abstract class defining a service, and is, > essentially, a factory for `InetAddressResolver` resolvers. > - `InetAddressResolverProvider.Configuration ` - an interface describing the > platform's built-in configuration for resolution operations that could be > used to bootstrap a resolver construction, or to implement partial delegation > of lookup operations. > - `InetAddressResolver` - an interface that defines methods for the > fundamental forward and reverse lookup operations. > - `InetAddressResolver.LookupPolicy` - a class whose instances describe the > characteristics of one forward lookup operation. > > More details in [JEP-418](https://openjdk.java.net/jeps/418). > > Testing: new and existing `tier1:tier3` tests Aleksei Efimov has updated the pull request incrementally with two additional commits since the last revision: - Add @since tags to new API classes - Add checks and test for empty stream resolver results - Changes: - all: https://git.openjdk.java.net/jdk/pull/5822/files - new: https://git.openjdk.java.net/jdk/pull/5822/files/41717fc7..d302a49a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5822=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5822=01-02 Stats: 150 lines in 6 files changed: 146 ins; 0 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/5822.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5822/head:pull/5822 PR: https://git.openjdk.java.net/jdk/pull/5822
Re: RFR: JDK-8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior [v2]
> Classes compiled prior to the nestmate support will generate > `REF_invokeSpecial` if the implementation method is a private instance > method. Since a lambda proxy class is a hidden class, a nestmate of the > host class, it can invoke the private implementation method but it has to use > `REF_invokeVirtual` or `REF_invokeInterface`. In order to support the old > classes running on the new runtime, LMF implementation adjusts the reference > kind from `REF_invokeSpecial` to `REF_invokeVirtual/REF_invokeInterface`. > > This PR fixes the check properly to ensure the adjustment is only made if the > implementation method is private method in the host class. Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: remove filelist which was added accidentally - Changes: - all: https://git.openjdk.java.net/jdk/pull/5901/files - new: https://git.openjdk.java.net/jdk/pull/5901/files/1358214d..cfdd036e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5901=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5901=00-01 Stats: 1 line in 1 file changed: 0 ins; 1 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5901.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5901/head:pull/5901 PR: https://git.openjdk.java.net/jdk/pull/5901
Re: RFR: JDK-8274848: LambdaMetaFactory::metafactory on REF_invokeSpecial impl method has incorrect behavior [v2]
On Tue, 12 Oct 2021 10:22:07 GMT, Alan Bateman wrote: >> Mandy Chung has updated the pull request incrementally with one additional >> commit since the last revision: >> >> remove filelist which was added accidentally > > filelist line 1: > >> 1: >> test/jdk/java/lang/invoke/lambda/invokeSpecial/InvokeSpecialMethodImpl.java > > I assume "filelist" is not meant to be in this match, I assume this is why > the "jdk" label was added. Fixed. It was accidentally added. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/5901
Re: RFR: 8273682: Upgrade Jline to 3.20.0
On Thu, 23 Sep 2021 15:43:08 GMT, Athijegannathan Sundararajan wrote: >> I'd like to upgrade the internal JLine to 3.20.0, to support the rxvt >> terminal (see JDK-8270943), and to generally use a newer version of the >> library. This patch is basically a application of relevant parts of the diff >> between JLine 3.14.0 and 3.20.0, with merge fixes as needed. >> >> Thanks! > > src/jdk.internal.le/share/classes/jdk/internal/org/jline/reader/CompletionMatcher.java > line 32: > >> 30: * >> 31: * @param candidates list of candidates >> 32: * @return a map of candidates that completion matcher matches > > a list of ..? Thanks, I've filled: https://github.com/jline/jline3/issues/711 - PR: https://git.openjdk.java.net/jdk/pull/5655
Re: Questions for JDK-4947890
Hi Ichiroh, This behavioral change is not intentional. It's a bug introduced in JDK-4947890. I create https://bugs.openjdk.java.net/browse/JDK-8275145 for this issue. Thanks for the investigation. Mandy On 10/12/21 2:44 AM, Ichiroh Takiguchi wrote: I have some questions for JDK-4947890。 One of app’s JVM was upgraded from JDK11 to JDK17. Then the working behavior was changed on Windows 10 Multilingual User Interface (MUI). (Base was Japanese Windows 10, display language setting was English (United States)) In my investigation, the working behavior was changed by JDK12+b23. JDK-4947890 Minimize JNI upcalls in system-properties initialization [1] Following change was applied on src/java.base/share/native/libjava/System.c, From (not for MacOS platform) [2] Put sprops->sun_jnu_encoding into file.encoding system property To [3] Put sprops->encoding into file.encoding system property I checked JDK-4947890’s CSR JDK-8213895 [4] Modify JVM interface functions for property initialization But it seems that above CSR does not mention such a significant specification change. I checked JDK17u, same code was still used [5]. Questions: I’d like to confirm * This change was intentional or not ? * We can revert tp JDK11’s spec ? Thanks, Ichiroh Takiguchi [1] https://bugs.openjdk.java.net/browse/JDK-4947890 [2] http://hg.openjdk.java.net/jdk/jdk/rev/0bdbf854472f#l12.466 [3] http://hg.openjdk.java.net/jdk/jdk/rev/0bdbf854472f#l12.276 [4] https://bugs.openjdk.java.net/browse/JDK-8213895 [5] https://github.com/openjdk/jdk17u/blob/master/src/java.base/share/native/libjava/System.c#L149
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Tue, 12 Oct 2021 17:42:01 GMT, Peter Levart wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Fix left-over assignment
>
> src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
> line 137:
>
>> 135: {
>> 136: if (isReadOnly()) {
>> 137: ensureObj(obj); // throw NPE if obj is null on instance
>> field
>
> I think ensureObj(obj) must go before if statement in setChar
No, it's OK. You are relying on `setter.invokeExact(obj, c)` to throw NPE
later...
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote:
>> This reimplements core reflection with method handles.
>>
>> For `Constructor::newInstance` and `Method::invoke`, the new implementation
>> uses `MethodHandle`. For `Field` accessor, the new implementation uses
>> `VarHandle`.For the first few invocations of one of these reflective
>> methods on a specific reflective object we invoke the corresponding method
>> handle directly. After that we spin a dynamic bytecode stub defined in a
>> hidden class which loads the target `MethodHandle` or `VarHandle` from its
>> class data as a dynamically computed constant. Loading the method handle
>> from a constant allows JIT to inline the method-handle invocation in order
>> to achieve good performance.
>>
>> The VM's native reflection methods are needed during early startup, before
>> the method-handle mechanism is initialized. That happens soon after
>> System::initPhase1 and before System::initPhase2, after which we switch to
>> using method handles exclusively.
>>
>> The core reflection and method handle implementation are updated to handle
>> chained caller-sensitive method calls [1] properly. A caller-sensitive
>> method can define with a caller-sensitive adapter method that will take an
>> additional caller class parameter and the adapter method will be annotated
>> with `@CallerSensitiveAdapter` for better auditing. See the detailed
>> description from [2].
>>
>> Ran tier1-tier8 tests.
>>
>> [1] https://bugs.openjdk.java.net/browse/JDK-8013527
>> [2]
>> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430
>
> Mandy Chung has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Fix left-over assignment
Marked as reviewed by plevart (Reviewer).
Hi Mandy,
I think this is good as first slab. I don't have anything to add at this point.
Some optimization ideas to try as followups. I ran benchmarks myself and the
results are similar to yours. Some seem like a regression, but don't have big
impact on real-world use case such as Jackson (de)serialization. "Const" class
is pretty much the same as with recently improved variant with @Stable fields.
I say Go!
src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
line 137:
> 135: {
> 136: if (isReadOnly()) {
> 137: ensureObj(obj); // throw NPE if obj is null on instance
> field
I think ensureObj(obj) must go before if statement in setChar
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Tue, 12 Oct 2021 17:44:08 GMT, Peter Levart wrote:
>> src/java.base/share/classes/jdk/internal/reflect/MethodHandleCharacterFieldAccessorImpl.java
>> line 137:
>>
>>> 135: {
>>> 136: if (isReadOnly()) {
>>> 137: ensureObj(obj); // throw NPE if obj is null on
>>> instance field
>>
>> I think ensureObj(obj) must go before if statement in setChar
>
> No, it's OK. You are relying on `setter.invokeExact(obj, c)` to throw NPE
> later...
Yup. This `ensureObj(obj)` call on a Field with no-write access is to ensure
NPE is thrown before IAE consistent with the current behavior.
-
PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v12]
On Fri, 8 Oct 2021 23:31:32 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request incrementally with one additional > commit since the last revision: > > Fix left-over assignment Thanks, Peter. JEP 417 is also updated to reflect this new implementation. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8274879: Replace uses of StringBuffer with StringBuilder within java.base classes [v2]
> StringBuffer is a legacy synchronized class. There are more modern > alternatives which perform better: > 1. Plain String concatenation should be preferred > 2. StringBuilder is a direct replacement to StringBuffer which generally have > better performance > > In [JDK-8264029](https://bugs.openjdk.java.net/browse/JDK-8264029) I > migrated only usages which were automatically detected by IDEA. It turns out > there are more usages which can be migrated. Andrey Turbanov has updated the pull request incrementally with one additional commit since the last revision: 8274879: Replace uses of StringBuffer with StringBuilder within java.base classes revert changes in spec to avoid CSR - Changes: - all: https://git.openjdk.java.net/jdk/pull/5432/files - new: https://git.openjdk.java.net/jdk/pull/5432/files/14005d1d..c8d68c2a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5432=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5432=00-01 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5432.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5432/head:pull/5432 PR: https://git.openjdk.java.net/jdk/pull/5432
Re: RFR: 8274879: Replace uses of StringBuffer with StringBuilder within java.base classes [v2]
On Mon, 11 Oct 2021 21:05:46 GMT, Andrey Turbanov wrote:
>> src/java.base/share/classes/java/lang/Character.java line 123:
>>
>>> 121: * than U+ are called supplementary characters. The Java
>>> 122: * platform uses the UTF-16 representation in {@code char} arrays and
>>> 123: * in the {@code String} and {@code StringBuilder} classes. In
>>
>> Not sure simple replacement applies here, as `StringBuffer` still uses
>> `UTF-16` representation. You may add `StringBuilder` as well here, but I
>> think you might want to file a CSR to clarify.
>
> reverted changes in this spec.
Did you modify the PR? I am unable to locate the revert.
-
PR: https://git.openjdk.java.net/jdk/pull/5432
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v7]
On Tue, 12 Oct 2021 18:11:56 GMT, Cheng Jin wrote:
> Just tried with `System.load()` but still ended up with pretty much the same
> failure given both of them eventually invokes `ClassLoader.loadLibrary` to
> load the library in which case there is no big difference at this point.
Yes and no. System::loadLibrary wants a library name (no extension). It will
add a library prefix (e.g. `lib` on linux) and a library suffix (e.g. `.so` on
linux). So, if you do:
System.loadLibrary("c")
You will end up with `libc.so`. The `System::loadLibrary` logic will then try
to find that file in any of the known library paths.
`System.load` avoids this by accepting the full path of the library. So there's
no guessing the path, nor guessing of prefix/suffix. But it seems like loading
still fails, likely because we try to load this library with `dlopen` but this
is a static library, so for `dlopen` it just doesn't make sense.
-
PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8274879: Replace uses of StringBuffer with StringBuilder within java.base classes [v2]
On Tue, 12 Oct 2021 20:39:13 GMT, Andrey Turbanov wrote: >> StringBuffer is a legacy synchronized class. There are more modern >> alternatives which perform better: >> 1. Plain String concatenation should be preferred >> 2. StringBuilder is a direct replacement to StringBuffer which generally >> have better performance >> >> In [JDK-8264029](https://bugs.openjdk.java.net/browse/JDK-8264029) I >> migrated only usages which were automatically detected by IDEA. It turns out >> there are more usages which can be migrated. > > Andrey Turbanov has updated the pull request incrementally with one > additional commit since the last revision: > > 8274879: Replace uses of StringBuffer with StringBuilder within java.base > classes > revert changes in spec to avoid CSR Marked as reviewed by naoto (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/5432
Re: RFR: 8275063: Implementation of Foreign Function & Memory API (Second incubator) [v2]
> This PR contains the API and implementation changes for JEP-419 [1]. A more > detailed description of such changes, to avoid repetitions during the review > process, is included as a separate comment. > > [1] - https://openjdk.java.net/jeps/419 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Fix TestLinkToNativeRBP test - Changes: - all: https://git.openjdk.java.net/jdk/pull/5907/files - new: https://git.openjdk.java.net/jdk/pull/5907/files/be0dd36e..23f69054 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5907=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5907=00-01 Stats: 7 lines in 1 file changed: 1 ins; 4 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/5907.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5907/head:pull/5907 PR: https://git.openjdk.java.net/jdk/pull/5907
Re: RFR: 8274879: Replace uses of StringBuffer with StringBuilder within java.base classes [v2]
On Tue, 12 Oct 2021 20:33:20 GMT, Naoto Sato wrote: >> reverted changes in this spec. > > Did you modify the PR? I am unable to locate the revert. Oops. Forgot to push - PR: https://git.openjdk.java.net/jdk/pull/5432
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v7]
On Thu, 3 Jun 2021 20:49:44 GMT, Maurizio Cimadamore wrote: >> This patch overhauls the library loading mechanism used by the Foreign >> Linker API. We realized that, while handy, the *default* lookup abstraction >> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. >> >> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` >> abstraction, a functional interface. Crucially, `SymbolLookup` does not >> concern with library loading, only symbol lookup. For this reason, two >> factories are added: >> >> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to >> lookup symbols in libraries loaded by current loader >> * `CLinker::systemLookup` - a more stable replacement for the *default* >> lookup, which looks for symbols in libc.so (or its equivalent in other >> platforms). The contents of this lookup are unspecified. >> >> Both factories are *restricted*, so they can only be called when >> `--enable-native-access` is set. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments on shim lib makefile If so, I am wondering whether there is anything else left (inherited from JDK16) in JDK17 we can leverage to support `libc.a` on AIX except the way similar to Windows. - PR: https://git.openjdk.java.net/jdk/pull/4316
Re: RFR: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries [v7]
On Thu, 3 Jun 2021 20:49:44 GMT, Maurizio Cimadamore wrote: >> This patch overhauls the library loading mechanism used by the Foreign >> Linker API. We realized that, while handy, the *default* lookup abstraction >> (`LibraryLookup::ofDefault`) was behaving inconsistentlt across platforms. >> >> This patch replaces `LibraryLookup` with a simpler `SymbolLookup` >> abstraction, a functional interface. Crucially, `SymbolLookup` does not >> concern with library loading, only symbol lookup. For this reason, two >> factories are added: >> >> * `SymbolLookup::loaderLookup` - which obtains a lookup that can be used to >> lookup symbols in libraries loaded by current loader >> * `CLinker::systemLookup` - a more stable replacement for the *default* >> lookup, which looks for symbols in libc.so (or its equivalent in other >> platforms). The contents of this lookup are unspecified. >> >> Both factories are *restricted*, so they can only be called when >> `--enable-native-access` is set. > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Address review comments on shim lib makefile I think the reason it worked in JDK 16 is because all the symbols in the JVM were visible through the system lookup, and the JVM statically links the standard library (AFAIU). So, just because the VM code depended on something, it could be loaded by the system lookup. But, that would mean that not all symbols in the standard library were visible... and also, being able to find _any_ symbol in the JVM was a bug. I think the only solution here - assuming that libc is not available as a dynamic library on typical AIX systems - is to figure out how to repackage these static libraries as a dynamic library, retaining all the symbols, and then bundle that dynamic library with the JDK. - PR: https://git.openjdk.java.net/jdk/pull/4316
