Integrated: 8284579: Improve VarHandle checks for interpreter
On Fri, 8 Apr 2022 11:48:10 GMT, Claes Redestad wrote: > A few additional enhancements aiming to improve VH performance in the > interpreter: > > - Flatten `TypeAndInvokers`: adds a pointer to `VarHandle` (a small increase > 40->48) but removes an object and an indirection on any instance actually > used - and might avoid allocating the `MethodHandle[]` unnecessarily on some > instances > - Have `checkExactAccessMode` return the directness of the `VarHandle` so > that we can avoid some `isDirect` method calls. > > Baseline, `-Xint` > > Benchmark Mode CntScore Error Units > VarHandleExact.exact_exactInvocation avgt 30 478.324 ? 5.762 ns/op > VarHandleExact.generic_exactInvocationavgt 30 392.114 ? 1.644 ns/op > VarHandleExact.generic_genericInvocation avgt 30 822.484 ? 1.865 ns/op > > > Patched, `-Xint` > > Benchmark Mode CntScore Error Units > VarHandleExact.exact_exactInvocation avgt 30 437.704 ? 5.320 ns/op > VarHandleExact.generic_exactInvocationavgt 30 374.512 ? 3.154 ns/op > VarHandleExact.generic_genericInvocation avgt 30 757.054 ? 1.237 ns/op > > > No significant performance difference in normal mode. This pull request has now been integrated. Changeset: 280aa428 Author:Claes Redestad URL: https://git.openjdk.java.net/jdk/commit/280aa428800043f314b92ae88076d596cb4c2fe0 Stats: 244 lines in 4 files changed: 26 ins; 28 del; 190 mod 8284579: Improve VarHandle checks for interpreter Reviewed-by: mcimadamore, mchung - PR: https://git.openjdk.java.net/jdk/pull/8160
Re: RFR: 8283994: Make Xerces DatatypeException stackless [v2]
On Wed, 6 Apr 2022 07:48:04 GMT, Aleksey Shipilev wrote: >> Any other reviews? I would like someone else to confirm my investigation >> that we don't use the stack traces out of these exceptions too... > >> Hello @shipilev, do you think this stackless nature of this specific >> `DatatypeException` type should be noted in its javadoc, just to avoid any >> surprises when someone in future ends up using this exception type as the >> "cause" of some other exception? > > I don't think so. It seems to me the intent for these exceptions is to carry > error information without any stack trace info. > @shipilev > > > Any other reviews? I would like someone else to confirm my investigation > > that we don't use the stack traces out of these exceptions too... > > I opened [XERCESJ‑1742](https://issues.apache.org/jira/browse/XERCESJ-1742) > to get this change upstreamed into **Xerces**, the resolution of which should > also answer your question about whether the stack traces are really unused. Thanks! But I see there seem to be no upstream interest yet. So I am planning to integrate this to JDK first, and so asking for JDK-specific reviews again. - PR: https://git.openjdk.java.net/jdk/pull/8036
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Tue, 12 Apr 2022 13:02:44 GMT, Alan Bateman wrote:
>> src/java.base/share/classes/java/io/BufferedReader.java line 101:
>>
>>> 99: */
>>> 100: public BufferedReader(Reader in, int sz) {
>>> 101: Objects.requireNonNull(in);
>>
>> Not sure if that even matters - but there will be a slight change of
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`.
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on
>> `this`.
>> Now that I think of it - it probably does matter since even if
>> CAN_USE_INTERNAL_LOCK is true, untrusted subclasses of BufferedReader
>> calling this constructor might expect the locking to be performed on `in`?
>
>> Not sure if that even matters - but there will be a slight change of
>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`.
>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on
>> `this`.
>
> Good! We can change this so that depends on whether BufferedReader is
> extended and whether the given Reader is trusted. It's not clear if anyone
> could reliably depend on undocumented behavior like this but we have to be
> cautious at the same time.
Thanks - the same issue appears with `BufferedWriter`/`Writer`.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8283994: Make Xerces DatatypeException stackless [v2]
On Thu, 31 Mar 2022 17:54:28 GMT, Aleksey Shipilev wrote: >> See bug report for more details. This change improves >> SPECjvm2008:xml.validation for about +3%: >> >> >> baseline: 298.353 ± 1.008 ops/min >> patched: 309.912 ± 1.347 ops/min >> >> Of course, the real improvements might be even higher, as exception might be >> thrown from much deeper call hierarchy. >> >> Additional testing: >> - [x] Linux x86_64 fastdebug, `jaxp_all` >> - [x] Linux x86_64 fastdebug, `javax/xml` > > Aleksey Shipilev has updated the pull request incrementally with one > additional commit since the last revision: > > Also update LastModified Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/8036
Re: RFR: 8283994: Make Xerces DatatypeException stackless [v2]
On Wed, 13 Apr 2022 08:30:33 GMT, Aleksey Shipilev wrote: >>> Hello @shipilev, do you think this stackless nature of this specific >>> `DatatypeException` type should be noted in its javadoc, just to avoid any >>> surprises when someone in future ends up using this exception type as the >>> "cause" of some other exception? >> >> I don't think so. It seems to me the intent for these exceptions is to carry >> error information without any stack trace info. > >> @shipilev >> >> > Any other reviews? I would like someone else to confirm my investigation >> > that we don't use the stack traces out of these exceptions too... >> >> I opened [XERCESJ‑1742](https://issues.apache.org/jira/browse/XERCESJ-1742) >> to get this change upstreamed into **Xerces**, the resolution of which >> should also answer your question about whether the stack traces are really >> unused. > > Thanks! But I see there seem to be no upstream interest yet. So I am planning > to integrate this to JDK first, and so asking for JDK-specific reviews again. > > Hello @shipilev, do you think this stackless nature of this specific > > `DatatypeException` type should be noted in its javadoc, just to avoid any > > surprises when someone in future ends up using this exception type as the > > "cause" of some other exception? > > I don't think so. It seems to me the intent for these exceptions is to carry > error information without any stack trace info. Looks fine to me then. However, I'm not a Reviewer. - PR: https://git.openjdk.java.net/jdk/pull/8036
Re: RFR: 8275535: Retrying a failed authentication on multiple LDAP servers can lead to users blocked
On Wed, 20 Oct 2021 13:35:22 GMT, Martin Balao wrote: > I'd like to propose a fix for JDK-8275535. This fix reverts the behavior to > the state previous to JDK-8160768, where an authentication failure stops from > trying other LDAP servers with the same credentials [1]. After JDK-8160768 we > have 2 possible loops to stop: the one that iterates over different URLs and > the one that iterates over different endpoints (after a DNS query that > returns multiple values). > > No test regressions observed in jdk/com/sun/jndi/ldap. > > -- > [1] - https://hg.openjdk.java.net/jdk/jdk/rev/a609d549992a#l2.137 Look [here](https://github.com/spring-projects/spring-security/issues/10318) for spring based test case. - PR: https://git.openjdk.java.net/jdk/pull/6043
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Wed, 13 Apr 2022 09:49:04 GMT, Daniel Fuchs wrote:
>>> Not sure if that even matters - but there will be a slight change of
>>> behaviour here if `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`.
>>> Instead of synchronizing on `in`, the `BufferedReader` will synchronize on
>>> `this`.
>>
>> Good! We can change this so that depends on whether BufferedReader is
>> extended and whether the given Reader is trusted. It's not clear if anyone
>> could reliably depend on undocumented behavior like this but we have to be
>> cautious at the same time.
>
> Thanks - the same issue appears with `BufferedWriter`/`Writer`.
The solution is to add the `private` constructor:
private Reader(Object fallbackLock, Void internal) {
// assert fallbackLock != null;
// use InternalLock for trusted classes
Class clazz = getClass();
if (clazz == InputStreamReader.class
|| clazz == BufferedReader.class
|| clazz == FileReader.class
|| clazz == sun.nio.cs.StreamDecoder.class) {
this.lock = InternalLock.newLockOr(fallbackLock);
} else {
this.lock = fallbackLock;
}
}
to `Reader` (and an equivalent `private` constructor to `Writer`).
---
The `protected` `Reader` constructors can then be changed to:
protected Reader() {
this(this, null);
}
and
protected Reader(Object lock) {
this(Objects.requireNonNull(lock), null);
}
---
Doing so will allow this change to be reverted:
Suggestion:
super(in);
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Tue, 12 Apr 2022 19:09:32 GMT, Daniel Jeliński wrote:
>> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which
>> JDK version to target.
>>
>> We will refresh this PR periodically to pick up changes and fixes from the
>> loom repo.
>>
>> Most of the new mechanisms in the HotSpot VM are disabled by default and
>> require running with `--enable-preview` to enable.
>>
>> The patch has support for x64 and aarch64 on the usual operating systems
>> (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for
>> zero and some of the other ports. Additional ports can be contributed via
>> PRs against the fibers branch in the loom repo.
>>
>> There are changes in many areas. To reduce notifications/mails, the labels
>> have been trimmed down for now to hotspot, serviceability and core-libs.
>> We'll add the complete set of labels when the PR is further along.
>>
>> The changes include a refresh of java.util.concurrent and ForkJoinPool from
>> Doug Lea's CVS. These changes will probably be proposed and integrated in
>> advance of this PR.
>>
>> The changes include some non-exposed and low-level infrastructure to support
>> the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to
>> make life a bit easier and avoid having to separate VM changes and juggle
>> branches at this time.
>
> src/java.base/share/classes/sun/nio/cs/StreamEncoder.java line 110:
>
>> 108:
>> 109: public void flushBuffer() throws IOException {
>> 110: if (lock instanceof InternalLock locker) {
>
> now that StreamEncoder is final, we can drop the `else` branch
Actually, we can’t in case `InternalLock.CAN_USE_INTERNAL_LOCK` is ever `false`
(which it might be if it gets exposed as a system property).
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview)
On Tue, 12 Apr 2022 16:49:41 GMT, Daniel Jeliński wrote: > If it was, please update the javadoc - `NANOSECONDS.toMillis(-1)` = 0 implies > no waiting in Net.poll It's benign for the current usages but you are right, it was not intentional. - PR: https://git.openjdk.java.net/jdk/pull/8166
RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
Please review this small fix for JDK-8284642. The fix was tested by running Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux x64. Additionally, the modified test and the test in the bug report were run locally. Thanks, Harold - Commit messages: - 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0 Changes: https://git.openjdk.java.net/jdk/pull/8222/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8222&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284642 Stats: 7 lines in 2 files changed: 0 ins; 0 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/8222.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8222/head:pull/8222 PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold How long exactly has this been broken? I'd be worried about use cases where people use MaxDirectMemorySize=0 to prevent usage of direct memory altogether. - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup. src/jdk.compiler/share/classes/com/sun/tools/javac/code/Preview.java line 190: > 188: case CASE_NULL -> true; > 189: case PATTERN_SWITCH -> true; > 190: case TOTAL_PATTERN_IN_INSTACEOF -> true; small typo in `INSTANCEOF` - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8282664: Unroll by hand StringUTF16 and StringLatin1 polynomial hash loops [v9]
On Thu, 7 Apr 2022 07:01:23 GMT, Ludovic Henry wrote: >> Despite the hash value being cached for Strings, computing the hash still >> represents a significant CPU usage for applications handling lots of text. >> >> Even though it would be generally better to do it through an enhancement to >> the autovectorizer, the complexity of doing it by hand is trivial and the >> gain is sizable (2x speedup) even without the Vector API. The algorithm has >> been proposed by Richard Startin and Paul Sandoz [1]. >> >> Speedup are as follows on a `Intel(R) Xeon(R) E-2276G CPU @ 3.80GHz` >> >> >> Benchmark(size) Mode Cnt Score >>Error Units >> StringHashCode.Algorithm.scalarLatin1 0 avgt 25 2.111 >> ± 0.210 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 3.500 >> ± 0.127 ns/op >> StringHashCode.Algorithm.scalarLatin110 avgt 25 7.001 >> ± 0.099 ns/op >> StringHashCode.Algorithm.scalarLatin1 100 avgt 2561.285 >> ± 0.444 ns/op >> StringHashCode.Algorithm.scalarLatin1 1000 avgt 25 628.995 >> ± 0.846 ns/op >> StringHashCode.Algorithm.scalarLatin1 1 avgt 25 6307.990 >> ± 4.071 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 0 avgt 25 2.358 >> ± 0.092 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3.631 >> ± 0.159 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 10 avgt 25 7.049 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 100 avgt 2533.626 >> ± 1.218 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled161000 avgt 25 317.811 >> ± 1.225 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled16 1 avgt 25 3212.333 >> ± 14.621 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled80 avgt 25 2.356 >> ± 0.097 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3.630 >> ± 0.158 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 10 avgt 25 8.724 >> ± 0.065 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 100 avgt 2532.402 >> ± 0.019 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled8 1000 avgt 25 321.949 >> ± 0.251 ns/op >> StringHashCode.Algorithm.scalarLatin1Unrolled81 avgt 25 3202.083 >> ± 1.667 ns/op >> StringHashCode.Algorithm.scalarUTF16 0 avgt 25 2.135 >> ± 0.191 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 5.202 >> ± 0.362 ns/op >> StringHashCode.Algorithm.scalarUTF16 10 avgt 2511.105 >> ± 0.112 ns/op >> StringHashCode.Algorithm.scalarUTF16100 avgt 2575.974 >> ± 0.702 ns/op >> StringHashCode.Algorithm.scalarUTF16 1000 avgt 25 716.429 >> ± 3.290 ns/op >> StringHashCode.Algorithm.scalarUTF16 1 avgt 25 7095.459 >> ± 43.847 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled160 avgt 25 2.381 >> ± 0.038 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 5.268 >> ± 0.422 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 10 avgt 2511.248 >> ± 0.178 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 100 avgt 2552.966 >> ± 0.089 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled16 1000 avgt 25 450.912 >> ± 1.834 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled161 avgt 25 4403.988 >> ± 2.927 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 0 avgt 25 2.401 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 5.091 >> ± 0.396 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled810 avgt 2512.801 >> ± 0.189 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 100 avgt 2552.068 >> ± 0.032 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1000 avgt 25 453.270 >> ± 0.340 ns/op >> StringHashCode.Algorithm.scalarUTF16Unrolled8 1 avgt 25 4433.112 >> ± 2.699 ns/op >> >> >> At Datadog, we handle a great amount of text (through logs management for >> example), and hashing String represents a large part of our CPU usage. It's >> very unlikely that we are the only one as String.hashCode is such a core >> feature of the JVM-based languages with its use in HashMap for example. >> Having even only a 2x speedup would allow us to save thousands of CPU cores >> per month and improve correspondingly the energy/carbon impact. >> >> [1] >> https://static.rainfocus.com/oracle/oow18/sess/1525822677955001tLqU/PF/codeone18-vector-API-DEV5081_1540354883936001Q3Sv.pdf > > Ludovic Henry has updated the pull request incrementally with one additional > commit since t
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold If nothing else, I think the man page could be improved to say that JVM chooses the size for NIO direct-buffer allocations automatically when MaxDirectMemorySize is not set. - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v2]
> This is the implementation of JEP 425: Virtual Threads (Preview); TBD which > JDK version to target. > > We will refresh this PR periodically to pick up changes and fixes from the > loom repo. > > Most of the new mechanisms in the HotSpot VM are disabled by default and > require running with `--enable-preview` to enable. > > The patch has support for x64 and aarch64 on the usual operating systems > (Linux, macOS, and Windows). There are stubs (calling Unimplemented) for zero > and some of the other ports. Additional ports can be contributed via PRs > against the fibers branch in the loom repo. > > There are changes in many areas. To reduce notifications/mails, the labels > have been trimmed down for now to hotspot, serviceability and core-libs. > We'll add the complete set of labels when the PR is further along. > > The changes include a refresh of java.util.concurrent and ForkJoinPool from > Doug Lea's CVS. These changes will probably be proposed and integrated in > advance of this PR. > > The changes include some non-exposed and low-level infrastructure to support > the (in draft) JEPs for Structured Concurrency and Scope Locals. This is to > make life a bit easier and avoid having to separate VM changes and juggle > branches at this time. Alan Bateman has updated the pull request incrementally with one additional commit since the last revision: Refresh - Changes: - all: https://git.openjdk.java.net/jdk/pull/8166/files - new: https://git.openjdk.java.net/jdk/pull/8166/files/21972f45..0e12c206 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=8166&range=00-01 Stats: 1837 lines in 112 files changed: 786 ins; 531 del; 520 mod Patch: https://git.openjdk.java.net/jdk/pull/8166.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8166/head:pull/8166 PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v2]
On Wed, 13 Apr 2022 11:35:33 GMT, ExE Boss wrote: > Actually, we can’t in case `InternalLock.CAN_USE_INTERNAL_LOCK` is ever > `false` That's right, both StreamEncoder and StreamDecoder need the both cases. - PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284642: Unexpected behavior of -XX:MaxDirectMemorySize=0
On Wed, 13 Apr 2022 12:24:46 GMT, Harold Seigel wrote: > Please review this small fix for JDK-8284642. The fix was tested by running > Mach5 tiers 1-2 on Linux, Mac OS, and Windows, and Mach5 tiers 3-5 on Linux > x64. Additionally, the modified test and the test in the bug report were run > locally. > > Thanks, Harold Marked as reviewed by stuefe (Reviewer). I looked a little, and found no example on GH or Google which relied on MaxDirectMemorySize=0 to disable. I found some examples of the opposite, e.g. https://github.com/cloudfoundry/java-buildpack/issues/591. So this is fine I think. - PR: https://git.openjdk.java.net/jdk/pull/8222
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 22:19:18 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > add MIN_SKIP_BUFFER_SIZE This change may be problematic for servers with a large number connections and an input stream for each connection. It could add up to 2k to the footprint of each connection when skip is used. This change may be problematic for servers with a large number connections and an input stream for each connection. It could add up to 2k to the footprint of each connection when skip is used. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v6]
On Tue, 12 Apr 2022 15:00:29 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Adapted wording based on @AlanBateman's review, removed implementation note > on ZipFile::getInputStream and aligned wording for all ::read methods You've addressed my points, the updated javadoc looks good. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:36:17 GMT, Alan Bateman wrote: > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. @AlanBateman You are correct about this. But I wonder if this be a problem, why Reader class can afford store a skip buffer for each Reader. Is there anything different in the situations about skipBuffer in Reader and InputStream? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Tue, 12 Apr 2022 23:13:26 GMT, liach wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> add MIN_SKIP_BUFFER_SIZE
>
> src/java.base/share/classes/java/io/InputStream.java line 557:
>
>> 555:
>> 556: while (remaining > 0) {
>> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
>
> I recommend moving `nr` declaration from the beginning of the method to where
> it's actually used (here)
The check for `skipBuffer.length < size` makes it appear that the buffer can be
re-allocated.
If it is allocated once then only the `skipBuffer == null` is needed.
The code may be simpler if the 'size' variable is removed.
byte[] skipBuffer = this.skipBuffer;
if (skipBuffer == null) {
this.skipBuffer = skipBuffer =
new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
}
while (remaining > 0) {
int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
remaining));
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: moving nr declaration from the beginning of the method to where it's actually used - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/81e9ca49..85cec668 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=01-02 Stats: 2 lines in 1 file changed: 0 ins; 1 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:45:31 GMT, Roger Riggs wrote:
>> src/java.base/share/classes/java/io/InputStream.java line 557:
>>
>>> 555:
>>> 556: while (remaining > 0) {
>>> 557: nr = read(skipBuffer, 0, (int)Math.min(size, remaining));
>>
>> I recommend moving `nr` declaration from the beginning of the method to
>> where it's actually used (here)
>
> The check for `skipBuffer.length < size` makes it appear that the buffer can
> be re-allocated.
> If it is allocated once then only the `skipBuffer == null` is needed.
>
> The code may be simpler if the 'size' variable is removed.
>
> byte[] skipBuffer = this.skipBuffer;
> if (skipBuffer == null) {
> this.skipBuffer = skipBuffer =
> new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
> MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
> }
> while (remaining > 0) {
> int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
> remaining));
It indeed is reallocated when the existing one is not large enough.
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:51:34 GMT, liach wrote:
>> The check for `skipBuffer.length < size` makes it appear that the buffer can
>> be re-allocated.
>> If it is allocated once then only the `skipBuffer == null` is needed.
>>
>> The code may be simpler if the 'size' variable is removed.
>>
>> byte[] skipBuffer = this.skipBuffer;
>> if (skipBuffer == null) {
>> this.skipBuffer = skipBuffer =
>> new byte[(remaining < MIN_SKIP_BUFFER_SIZE) ?
>> MIN_SKIP_BUFFER_SIZE : MAX_SKIP_BUFFER_SIZE];
>> }
>> while (remaining > 0) {
>> int nr = read(skipBuffer, 0, (int)Math.min(skipBuffer.length,
>> remaining));
>
> It indeed is reallocated when the existing one is not large enough.
> I recommend moving `nr` declaration from the beginning of the method to where
> it's actually used (here)
@liach done.
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284161: Implementation of Virtual Threads (Preview) [v2]
On Wed, 13 Apr 2022 11:38:55 GMT, ExE Boss wrote:
>> Thanks - the same issue appears with `BufferedWriter`/`Writer`.
>
> The solution is to add the `private` constructor:
>
> private Reader(Object fallbackLock, Void internal) {
> // assert fallbackLock != null;
> // use InternalLock for trusted classes
> Class clazz = getClass();
> if (clazz == InputStreamReader.class
> || clazz == BufferedReader.class
> || clazz == FileReader.class
> || clazz == sun.nio.cs.StreamDecoder.class) {
> this.lock = InternalLock.newLockOr(fallbackLock);
> } else {
> this.lock = fallbackLock;
> }
> }
>
> to `Reader` (and an equivalent `private` constructor to `Writer`).
>
> ---
>
> The `protected` `Reader` constructors can then be changed to:
>
> protected Reader() {
> this(this, null);
> }
>
> and
>
> protected Reader(Object lock) {
> this(Objects.requireNonNull(lock), null);
> }
>
>
> ---
>
> Doing so will allow this change to be reverted:
> Suggestion:
>
> super(in);
> Thanks - the same issue appears with `BufferedWriter`/`Writer`.
Right. I think we can reduce this to the case where a BufferedReader is not
subclassed, and it wraps an InputStreamReader or FileReader that have not been
subclassed. Similarly the case where a BufferedWriter is not subclassed and it
wraps an OutputStreamWriter or FileWriter that have not been subclassed.
-
PR: https://git.openjdk.java.net/jdk/pull/8166
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:36:17 GMT, Alan Bateman wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> add MIN_SKIP_BUFFER_SIZE > > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. > @AlanBateman You are correct about this. But I wonder if this be a problem, > why Reader class can afford store a skip buffer for each Reader. > > Is there anything different in the situations about skipBuffer in Reader and > InputStream? Maybe the skip buffer in Reader should be looked at too, esp. as it couldpotentially grow to 16k bytes. The concern with changing InputStream.skip is that there may be a lot more input streams than readers in use, esp. if there is an input stream for every socket connection. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 14:52:20 GMT, XenoAmess wrote: >> It indeed is reallocated when the existing one is not large enough. > >> I recommend moving `nr` declaration from the beginning of the method to >> where it's actually used (here) > > @liach done. Sorry, I misunderstood your earlier comment: "Sounds reasonable and applied" as concurring with not re-allocating to avoid the overhead of gc and wasting the smaller buffer. The code can be harder to understand because `size` and `skipBuffer.length` may be different. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: > > @AlanBateman You are correct about this. But I wonder if this be a problem, > > why Reader class can afford store a skip buffer for each Reader. > > Is there anything different in the situations about skipBuffer in Reader > > and InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. Making a drop of storing skipBuffer in Reader sonds another solution that acceptable to me, as it makes the handling strategy same in Reader and InputStream classes. > The concern with changing InputStream.skip is that there may be a lot more > input streams than readers in use I don't think use frequency can influence this. In other words, I still think, if there be no special acceptable reason, we shall make the skipBuffer handle strategy same in Reader and InputStream classes. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: >> This change may be problematic for servers with a large number connections >> and an input stream for each connection. It could add up to 2k to the >> footprint of each connection when skip is used. > >> @AlanBateman You are correct about this. But I wonder if this be a problem, >> why Reader class can afford store a skip buffer for each Reader. >> >> Is there anything different in the situations about skipBuffer in Reader and >> InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. @AlanBateman Is the concern about holding more memory sufficient to retain the buffer using a SoftReference so it can be freed eagerly? These buffers are never read from, so are quite a waste, but at least they are only used when the underlying stream overrides skip. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used On a side note for unifying the skip buffer implementation of reader vs input stream: For the input stream subclasses in the JDK that have their own skip with buffering logic (as described in https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they almost always have only local-variable skip buffers (not kept as fields for reuse), and their buffers' max sizes are smaller that provided by the InputStream class. Imo we should check the usage of `skip` in other projects; in JDK it's like skipping 2 bytes in certain image formats, and I would expect usages like reading class file attribute name and size then skip by the read size. > This change may be problematic for servers with a large number connections > and an input stream for each connection. It could add up to 2k to the > footprint of each connection when skip is used. If per-object allocation is a problem, would it be feasible to allocate a static soft reference to a max-sized skip buffer? It can be potentially shared with the `Reader` class, too. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 15:23:16 GMT, XenoAmess wrote: > > On a side note for unifying the skip buffer implementation of reader vs > > input stream: For the input stream subclasses in the JDK that have their > > own skip with buffering logic (as described in > > https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they > > almost always have only local-variable skip buffers (not kept as fields for > > reuse), and their buffers' max sizes are smaller that provided by the > > InputStream class. > > > > Imo we should check the usage of `skip` in other projects; in JDK it's like > > skipping 2 bytes in certain image formats, and I would expect usages like > > reading class file attribute name and size then skip by the read size. > > > > > This change may be problematic for servers with a large number > > > connections and an input stream for each connection. It could add up to > > > 2k to the footprint of each connection when skip is used. > > > > If per-object allocation is a problem, would it be feasible to allocate a > > static soft reference to a max-sized skip buffer? It can be potentially > > shared with the `Reader` class, too. > > No as security reason. Any subclass can read this buffer using read function， > thus might cause secure data leak. see https://github.com/openjdk/jdk/pull/5855 - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 15:20:06 GMT, liach wrote: > On a side note for unifying the skip buffer implementation of reader vs input > stream: For the input stream subclasses in the JDK that have their own skip > with buffering logic (as described in > https://github.com/openjdk/jdk/pull/5872#discussion_r848950065), they almost > always have only local-variable skip buffers (not kept as fields for reuse), > and their buffers' max sizes are smaller that provided by the InputStream > class. > > Imo we should check the usage of `skip` in other projects; in JDK it's like > skipping 2 bytes in certain image formats, and I would expect usages like > reading class file attribute name and size then skip by the read size. > > > This change may be problematic for servers with a large number connections > > and an input stream for each connection. It could add up to 2k to the > > footprint of each connection when skip is used. > > If per-object allocation is a problem, would it be feasible to allocate a > static soft reference to a max-sized skip buffer? It can be potentially > shared with the `Reader` class, too. No as security reason. Any subclass can read this buffer using read function， thus might cause secure data leak. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> moving nr declaration from the beginning of the method to where it's
> actually used
Yeah forgot about that part. Thus I guess using soft reference would be a
better way to deal with this extra memory use issue. So something like
private SoftReference skipBuffer;
private byte[] skipBuffer(long remaining) {
int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);
SoftReference ref = this.skipBuffer;
byte[] buffer;
if (ref == null || (buffer = ref.get()) == null || buffer.length < size) {
buffer = new byte[size];
this.skipBuffer = new SoftReference(buffer);
}
return buffer;
}
This should be thread-safe, and we can just call `byte[] skipBuffer =
skipBuffer(remaining);`
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> moving nr declaration from the beginning of the method to where it's
> actually used
> Yeah forgot about that part. Thus I guess using soft reference would be a
> better way to deal with this extra memory use issue. So something like
>
> ```java
> private SoftReference skipBuffer;
>
> private byte[] skipBuffer(long remaining) {
> int size = (int) Math.min(MAX_SKIP_BUFFER_SIZE, remaining);
> SoftReference ref = this.skipBuffer;
> byte[] buffer;
> if (ref == null || (buffer = ref.get()) == null || buffer.length < size) {
> buffer = new byte[size];
> this.skipBuffer = new SoftReference(buffer);
> }
> return buffer;
> }
> ```
>
> This should be thread-safe, and we can just call `byte[] skipBuffer =
> skipBuffer(remaining);`
LGTM
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used I'd be a quite cautious about the slippery slope of incremental change with very little measurable benefit. Most applications reading InputStreams or Readers are using the buffered versions that already handle skip. These are the fallback versions. Except for the buffered versions, the lower level InputStreams can do a better job of skip then at the higher level built on read(). FileInputStream directly uses seek, other direct streams can probably do a more efficient skip() by reading into private buffers. For example low level I/O for networking already reads into private buffers. But the dominate case is still the buffered streams and readers. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 15:03:22 GMT, Alan Bateman wrote: >> This change may be problematic for servers with a large number connections >> and an input stream for each connection. It could add up to 2k to the >> footprint of each connection when skip is used. > >> @AlanBateman You are correct about this. But I wonder if this be a problem, >> why Reader class can afford store a skip buffer for each Reader. >> >> Is there anything different in the situations about skipBuffer in Reader and >> InputStream? > > Maybe the skip buffer in Reader should be looked at too, esp. as it > couldpotentially grow to 16k bytes. The concern with changing > InputStream.skip is that there may be a lot more input streams than readers > in use, esp. if there is an input stream for every socket connection. > @AlanBateman Is the concern about holding more memory sufficient to retain > the buffer using a SoftReference so it can be freed eagerly? > These buffers are never read from, so are quite a waste, but at least they > are only used when > the underlying stream overrides skip. Maybe but I think it needs some benchmarks to know if caching a byte[] will help or just bloat memory. If the InputStream is to a file or socket then maybe skip is dominated by the I/O rather than the array allocation and zero'ing. - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8186958: Need method to create pre-sized HashMap [v15]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with three additional commits since the last revision: - Add apiNote to appropriate constructors of HM, LHM, and WHM. - Add test cases for static factory methods. - Minor adjustment to test cases of WhiteBoxResizeTest - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/7adc89c1..ab8fbb83 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=14 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=13-14 Stats: 85 lines in 4 files changed: 77 ins; 1 del; 7 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
On Tue, 12 Apr 2022 10:24:47 GMT, Maurizio Cimadamore wrote: >> This PR contains the API and implementation changes for JEP-424 [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/424 > > Maurizio Cimadamore has updated the pull request incrementally with one > additional commit since the last revision: > > Add @ForceInline annotation on session accessor > Beef up UnrolledAccess benchmark src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052: > 1050: public MemorySegment map(MapMode mode, long offset, long size, > 1051: MemorySession session) > 1052: throws IOException, UnsupportedOperationException Just a minor here is that UOE is a runtime exception so probably shouldn't be in the throws. - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: revert changes in: src/java.desktop src/java.management src/jdk.internal.vm.ci src/jdk.jfr src/jdk.management.jfr src/jdk.management src/utils/IdealGraphVisualizer - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/ab8fbb83..2f5617bb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=15 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=14-15 Stats: 36 lines in 20 files changed: 0 ins; 0 del; 36 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v14]
On Wed, 13 Apr 2022 04:24:41 GMT, Stuart Marks wrote: > I've done some work on add test cases for these new static factory methods, > and I've also added API notes to the capacity-based constructors to link to > the new factory methods. Note that even though these are javadoc changes, the > API notes are non-normative and don't require an update to the CSR. See the > last few commits on this branch: > > https://github.com/stuart-marks/jdk/commits/JDK-8186958-presized-HashMap > > If you think they're good, please pull them in. > > Regarding the scope of changes, the number of call sites that are changed is > indeed spread rather too widely across the JDK. In order to keep the number > of required reviewers small, I think we should trim down the call sites to a > more manageable set. Specifically, I'd suggest **removing** from this PR the > changes from the files in the following areas: > > * src/java.desktop > * src/java.management > * src/jdk.internal.vm.ci > * src/jdk.jfr > * src/jdk.management.jfr > * src/jdk.management > * src/utils/IdealGraphVisualizer @stuart-marks done. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v6]
On Tue, 12 Apr 2022 18:12:03 GMT, Lance Andersen wrote:
>> Volker Simonis has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Adapted wording based on @AlanBateman's review, removed implementation
>> note on ZipFile::getInputStream and aligned wording for all ::read methods
>
> src/java.base/share/classes/java/util/zip/GZIPInputStream.java line 103:
>
>> 101: * elements {@code buf[off+}n{@code ]} through {@code
>> buf[off+}len{@code -1]}
>> 102: * are undefined (an implementation is free to change them during
>> the inflate
>> 103: * operation). If the return value is -1 or an exception is thrown
>> then {@code buf[off]}
>
> perhaps tweak "... is thrown then"
>
> to
>
> "... is thrown, then the contents of..."
Updated as requested (also in `ZipInputStream` and `InflaterInputStream`).
-
PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v7]
> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` to > highlight that it might write more bytes than the returned number of > inflated bytes into the buffer `b`. > > The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, > int len)` will leave the content beyond the last read byte in the read buffer > `b` unaffected. However, the overridden `read` method in > `InflaterInputStream` passes the read buffer `b` to `Inflater::inflate(byte[] > b, int off, int len)` which doesn't provide this guarantee. Depending on > implementation details, `Inflater::inflate` might write more than the > returned number of inflated bytes into the buffer `b`. > > ### TL;DR > > `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater > functionality. `Inflater::inflate(byte[] output, int off, int len)` currently > calls zlib's native `inflate(..)` function and passes the address of > `output[off]` and `len` to it via JNI. > > The specification of zlib's `inflate(..)` function (i.e. the [API > documentation in the original zlib > implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) > doesn't give any guarantees with regard to usage of the output buffer. It > only states that upon completion the function will return the number of bytes > that have been written (i.e. "inflated") into the output buffer. > > The original zlib implementation only wrote as many bytes into the output > buffer as it inflated. However, this is not a hard requirement and newer, > more performant implementations of the zlib library like > [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) > or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes > of the output buffer than they actually inflate as a scratch buffer. See > https://github.com/simonis/zlib-chromium for a more detailed description of > their approach and its performance benefit. > > These new zlib versions can still be used transparently from Java (e.g. by > putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because > they still fully comply to specification of `Inflater::inflate(..)`. However, > we might run into problems when using the `Inflater` functionality from the > `InflaterInputStream` class. `InflaterInputStream` is derived from from > `InputStream` and as such, its `read(byte[] b, int off, int len)` method is > quite constrained. It specifically specifies that if *k* bytes have been > read, then "these bytes will be stored in elements `b[off]` through > `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through `b[off+len-1]` > **unaffected**". But `InflaterInputStream::read(byte[] b, int off, int len)` > (which is constrained by `InputStream::read(..)`'s specification) calls > `Inflater::inflate(byte[] b, int off, int len)` and directly passes its > output buffer down to the native zlib `inflate(..)` method which is free to > change the bytes beyond `b[off+`* k*`]` (where *k* is the number of inflated bytes). > > From a practical point of view, I don't see this as a big problem, because > callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never > know how many bytes will be written into the output buffer `b` (and in fact > its content can always be completely overwritten). It therefore makes no > sense to depend on any data there being untouched after the call. Also, > having used zlib-cloudflare productively for about two years, we haven't seen > real-world issues because of this behavior yet. However, from a specification > point of view it is easy to artificially construct a program which violates > `InflaterInputStream::read(..)`'s postcondition if using one of the > alterantive zlib implementations. A recently integrated JTreg test > (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails > with zlib-chromium but can fixed easily to run with alternative > implementations as well (see > [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). Volker Simonis has updated the pull request incrementally with one additional commit since the last revision: Updated wording based on @LanceAndersen's review - Changes: - all: https://git.openjdk.java.net/jdk/pull/7986/files - new: https://git.openjdk.java.net/jdk/pull/7986/files/52524353..c9b60404 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7986&range=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7986&range=05-06 Stats: 9 lines in 3 files changed: 1 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/7986.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7986/head:pull/7986 PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v7]
On Wed, 13 Apr 2022 17:42:57 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Updated wording based on @LanceAndersen's review Hi Volker, The updates look good. Thank you. Next up is updating the CSR with the changes and craft the release-note as a subtask. Thank you for your efforts here - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v3]
On Sun, 3 Apr 2022 19:04:21 GMT, Alan Bateman wrote: >> Volker Simonis has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Extended API-doc based on reviewer feedback > > A suggestion for the structure is to start the new paragraph by saying it > reads n bytes of uncompressed data into b[off] to b[off+n-1]. Then follow to > say that the contents of b[n] to b[off+len-1] after the read are are > undefined, then say that an implementation is free to ... Finally just > finish it by saying that the contents are also undefined when the method > fails by throwing an exception. Thanks @AlanBateman, @LanceAndersen. I've just updated the CSR with the latest wording from this PR. Please feel free to review the CSR as well so I can finalize it. - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/0053820b...14980605 Here, I argue for the case of splitting different types of access flags into distinct enums, such as `MemberAccessFlag`, `ModuleRequiresAccessFlag`, `ModuleExportsAccessFlag` implementing a sealed interface `AccessFlag` that keeps all its existing methods, since most of those access flags will never be returned in the same collection. The `accessFlags()` method should return a `Set`, which is 1. safe as the set is read-only; 2. can be overridden with `Set` (For forward compability, we can make `MemberAccessFlag` an interface with static final fields, implemented by some package-private enum in case we want to split it into more specific types down the road). But you may ask, what about `SYNTHETIC`, `MANDATED`, `FINAL`? Aren't they shared by almost all of the locations? What if users test with incorrect enum instance, such as looking for the `MemberAccessFlag.SYNTHETIC` in `moduleDescriptor.accessFlags()`? This problem can be prevented: 1. by making `ModuleDescriptor.accessFlags()` return `Set`, thus the IDE can easily detect misuse when they insert the access flag of a different type; 2. In the current hodgepodge `AccessFlag`, we have `STATIC` and `STATIC_PHASE`, and the incorrect `ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC)` call is much more subtle, especially to new users of this class. Arguably, this misuse would be way worse than that in the distinct enum case. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Reviewers for i18n, net, nio, and security, please review call site changes in your areas. Thanks. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for changes in i18n related call sites, assuming that the copyright years will be updated. Should we need some additions/modifications to the hashmap optimal capacity utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? - Marked as reviewed by naoto (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
RFR: 8284853: Fix varios 'expected' typo
Found various typos of expected: `exepected`, `exept`, `epectedly`, `expeced`, `Unexpeted`, etc. - Commit messages: - [PATCH] Fix 'expected' typo - [PATCH] Fix 'expected' typo - [PATCH] Fix 'expected' typo Changes: https://git.openjdk.java.net/jdk/pull/8231/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8231&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8284853 Stats: 65 lines in 28 files changed: 0 ins; 0 del; 65 mod Patch: https://git.openjdk.java.net/jdk/pull/8231.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8231/head:pull/8231 PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer Looks good for the changes in java.xml, like Naoto, assuming copyright years and the LastModified tag are updated. I generally prefer for the source level to stay at 8 as the upstream source is maintained at an older JDK level, but these changes seem to be small enough to tolerate. - Marked as reviewed by joehw (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Sat, 5 Mar 2022 19:54:44 GMT, Joe Darcy wrote: >> This is an early review of changes to better model JVM access flags, that is >> "modifiers" like public, protected, etc. but explicitly at a VM level. >> >> Language level modifiers and JVM level access flags are closely related, but >> distinct. There are concepts that overlap in the two domains (public, >> private, etc.), others that only have a language-level modifier (sealed), >> and still others that only have an access flag (synthetic). >> >> The existing java.lang.reflect.Modifier class is inadequate to model these >> subtleties. For example, the bit positions used by access flags on different >> kinds of elements overlap (such as "volatile" for fields and "bridge" for >> methods. Just having a raw integer does not provide sufficient context to >> decode the corresponding language-level string. Methods like >> Modifier.methodModifiers() were introduced to cope with this situation. >> >> With additional modifiers and flags on the horizon with projects like >> Valhalla, addressing the existent modeling deficiency now ahead of time is >> reasonable before further strain is introduced. >> >> This PR in its current form is meant to give the overall shape of the API. >> It is missing implementations to map from, say, method modifiers to access >> flags, taking into account overlaps in bit positions. >> >> The CSR https://bugs.openjdk.java.net/browse/JDK-8281660 will be filled in >> once the API is further along. > > Joe Darcy has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 26 additional commits since > the last revision: > > - Respond to review feedback. > - Merge branch 'master' into JDK-8266670 > - Make workding changes suggested in review feedback. > - Merge branch 'master' into JDK-8266670 > - Typo fix; add implSpec to Executable. > - Appease jcheck. > - Fix some bugs found by inspection, docs cleanup. > - Merge branch 'master' into JDK-8266670 > - Initial support for accessFlags methods > - Add mask to access flag functionality. > - ... and 16 more: > https://git.openjdk.java.net/jdk/compare/364bd126...14980605 src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167: > 165: * but is optional in the dynamic phase, during execution. > 166: */ > 167: STATIC(AccessFlag.STATIC.mask()), This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not `AccessFlag.STATIC` (`0x0008`): Suggestion: STATIC(AccessFlag.STATIC_PHASE.mask()), - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: JDK-8266670: Better modeling of access flags in core reflection [v17]
On Wed, 13 Apr 2022 21:12:40 GMT, ExE Boss wrote: >> Joe Darcy has updated the pull request with a new target base due to a merge >> or a rebase. The incremental webrev excludes the unrelated changes brought >> in by the merge/rebase. The pull request contains 26 additional commits >> since the last revision: >> >> - Respond to review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Make workding changes suggested in review feedback. >> - Merge branch 'master' into JDK-8266670 >> - Typo fix; add implSpec to Executable. >> - Appease jcheck. >> - Fix some bugs found by inspection, docs cleanup. >> - Merge branch 'master' into JDK-8266670 >> - Initial support for accessFlags methods >> - Add mask to access flag functionality. >> - ... and 16 more: >> https://git.openjdk.java.net/jdk/compare/afd02683...14980605 > > src/java.base/share/classes/java/lang/module/ModuleDescriptor.java line 167: > >> 165: * but is optional in the dynamic phase, during execution. >> 166: */ >> 167: STATIC(AccessFlag.STATIC.mask()), > > This is actually `AccessFlag.STATIC_PHASE` (`0x0040`), and not > `AccessFlag.STATIC` (`0x0008`): > Suggestion: > > STATIC(AccessFlag.STATIC_PHASE.mask()), > In the current hodgepodge AccessFlag, we have STATIC and STATIC_PHASE, and > the incorrect ModuleDescriptor.accessFlags().contains(AccessFlag.STATIC) call > is much more subtle, especially to new users of this class. Arguably, this > misuse would be way worse than that in the distinct enum case. Oops, didn't know this already happened. Good spot right there. - PR: https://git.openjdk.java.net/jdk/pull/7445
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 16:29:11 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > revert changes in: > src/java.desktop > src/java.management > src/jdk.internal.vm.ci > src/jdk.jfr > src/jdk.management.jfr > src/jdk.management > src/utils/IdealGraphVisualizer The ZipFS and Jar changes seem OK - Marked as reviewed by lancea (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v27]
On Wed, 13 Apr 2022 16:12:31 GMT, Alan Bateman wrote: >> Maurizio Cimadamore has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Add @ForceInline annotation on session accessor >> Beef up UnrolledAccess benchmark > > src/java.base/share/classes/java/nio/channels/FileChannel.java line 1052: > >> 1050: public MemorySegment map(MapMode mode, long offset, long size, >> 1051: MemorySession session) >> 1052: throws IOException, UnsupportedOperationException > > Just a minor here is that UOE is a runtime exception so probably shouldn't be > in the throws. whoops - good catch - will fix! - PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v28]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Drop `UnsupportedOperationException` from throws clause in FileChannel/FileChannelImpl - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/66cebe77..3a87df5e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=27 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=26-27 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8282191: Implementation of Foreign Function & Memory API (Preview) [v29]
> This PR contains the API and implementation changes for JEP-424 [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/424 Maurizio Cimadamore has updated the pull request incrementally with one additional commit since the last revision: Remove whitespaces - Changes: - all: https://git.openjdk.java.net/jdk/pull/7888/files - new: https://git.openjdk.java.net/jdk/pull/7888/files/3a87df5e..8637379e Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=28 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7888&range=27-28 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7888.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7888/head:pull/7888 PR: https://git.openjdk.java.net/jdk/pull/7888
Re: RFR: 8284638: store skip buffers in InputStream Object [v4]
> @jmehrens what about this then? > I think it safe now(actually this mechanism is learned from Reader) XenoAmess has updated the pull request incrementally with one additional commit since the last revision: add jmh - Changes: - all: https://git.openjdk.java.net/jdk/pull/5872/files - new: https://git.openjdk.java.net/jdk/pull/5872/files/85cec668..7a9844cb Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=5872&range=02-03 Stats: 288 lines in 1 file changed: 288 ins; 0 del; 0 mod Patch: https://git.openjdk.java.net/jdk/pull/5872.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5872/head:pull/5872 PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
On Wed, 13 Apr 2022 16:02:10 GMT, Alan Bateman wrote: >>> @AlanBateman You are correct about this. But I wonder if this be a problem, >>> why Reader class can afford store a skip buffer for each Reader. >>> >>> Is there anything different in the situations about skipBuffer in Reader >>> and InputStream? >> >> Maybe the skip buffer in Reader should be looked at too, esp. as it >> couldpotentially grow to 16k bytes. The concern with changing >> InputStream.skip is that there may be a lot more input streams than readers >> in use, esp. if there is an input stream for every socket connection. > >> @AlanBateman Is the concern about holding more memory sufficient to retain >> the buffer using a SoftReference so it can be freed eagerly? >> These buffers are never read from, so are quite a waste, but at least they >> are only used when >> the underlying stream overrides skip. > > Maybe but I think it needs some benchmarks to know if caching a byte[] will > help or just bloat memory. If the InputStream is to a file or socket then > maybe skip is dominated by the I/O rather than the array allocation and > zero'ing. @AlanBateman jmh test added - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8284638: store skip buffers in InputStream Object [v3]
On Wed, 13 Apr 2022 14:56:12 GMT, XenoAmess wrote: >> @jmehrens what about this then? >> I think it safe now(actually this mechanism is learned from Reader) > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > moving nr declaration from the beginning of the method to where it's > actually used # JMH version: 1.34 # VM version: JDK 19-internal, OpenJDK 64-Bit Server VM, 19-internal-adhoc..jdk # VM invoker: F:\workspace\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.exe # VM options: -Djava.library.path=f:\workspace\jdk\build\windows-x86_64-server-release\images\test\micro\native -Xms1g -Xmx1g # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 10 iterations, 500 ms each # Measurement: 10 iterations, 500 ms each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0 # Parameters: (inputStreamSize = 100, skipLength = 1) # Run progress: 0.00% complete, ETA 00:14:00 # Fork: 1 of 3 # Warmup Iteration 1: 3188201.250 ns/op # Warmup Iteration 2: 2445386.603 ns/op # Warmup Iteration 3: 2395206.481 ns/op # Warmup Iteration 4: 2391797.196 ns/op # Warmup Iteration 5: 2518698.039 ns/op # Warmup Iteration 6: 2414542.254 ns/op # Warmup Iteration 7: 2419808.019 ns/op # Warmup Iteration 8: 2430526.540 ns/op # Warmup Iteration 9: 2430806.635 ns/op # Warmup Iteration 10: 2414558.173 ns/op Iteration 1: 2426910.427 ns/op Iteration 2: 2429144.811 ns/op Iteration 3: 2574322.500 ns/op Iteration 4: 2396079.907 ns/op Iteration 5: 2425966.509 ns/op Iteration 6: 2461450.239 ns/op Iteration 7: 2527480.392 ns/op Iteration 8: 2523890.640 ns/op Iteration 9: 2511460.488 ns/op Iteration 10: 2532332.843 ns/op # Run progress: 1.19% complete, ETA 00:14:36 # Fork: 2 of 3 # Warmup Iteration 1: 3174534.969 ns/op # Warmup Iteration 2: 2499138.235 ns/op # Warmup Iteration 3: 2421553.521 ns/op # Warmup Iteration 4: 2416571.698 ns/op # Warmup Iteration 5: 2384638.889 ns/op # Warmup Iteration 6: 2377448.387 ns/op # Warmup Iteration 7: 2395746.512 ns/op # Warmup Iteration 8: 2384657.209 ns/op # Warmup Iteration 9: 2403249.302 ns/op # Warmup Iteration 10: 2397511.163 ns/op Iteration 1: 2386731.481 ns/op Iteration 2: 2418383.491 ns/op Iteration 3: 2394599.535 ns/op Iteration 4: 2436117.536 ns/op Iteration 5: 2399048.357 ns/op Iteration 6: 2483871.498 ns/op Iteration 7: 2496279.612 ns/op Iteration 8: 2496743.204 ns/op Iteration 9: 2491237.811 ns/op Iteration 10: 2493390.338 ns/op # Run progress: 2.38% complete, ETA 00:14:25 # Fork: 3 of 3 # Warmup Iteration 1: 3677115.714 ns/op # Warmup Iteration 2: 2423365.877 ns/op # Warmup Iteration 3: 2480975.962 ns/op # Warmup Iteration 4: 2412990.610 ns/op # Warmup Iteration 5: 2433951.185 ns/op # Warmup Iteration 6: 2389266.190 ns/op # Warmup Iteration 7: 2407245.933 ns/op # Warmup Iteration 8: 2375563.721 ns/op # Warmup Iteration 9: 2388543.981 ns/op # Warmup Iteration 10: 2393794.419 ns/op Iteration 1: 2403104.306 ns/op Iteration 2: 2414766.355 ns/op Iteration 3: 2408296.667 ns/op Iteration 4: 2509554.412 ns/op Iteration 5: 2402212.440 ns/op Iteration 6: 2436674.272 ns/op Iteration 7: 2466500.000 ns/op Iteration 8: 2472919.712 ns/op Iteration 9: 2462019.139 ns/op Iteration 10: 2453551.905 ns/op Result "org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0": 2457834.694 ±(99.9%) 33354.477 ns/op [Average] (min, avg, max) = (2386731.481, 2457834.694, 2574322.500), stdev = 49923.415 CI (99.9%): [2424480.217, 2491189.172] (assumes normal distribution) # JMH version: 1.34 # VM version: JDK 19-internal, OpenJDK 64-Bit Server VM, 19-internal-adhoc..jdk # VM invoker: F:\workspace\jdk\build\windows-x86_64-server-release\images\jdk\bin\java.exe # VM options: -Djava.library.path=f:\workspace\jdk\build\windows-x86_64-server-release\images\test\micro\native -Xms1g -Xmx1g # Blackhole mode: compiler (auto-detected, use -Djmh.blackhole.autoDetect=false to disable) # Warmup: 10 iterations, 500 ms each # Measurement: 10 iterations, 500 ms each # Timeout: 10 min per iteration # Threads: 1 thread, will synchronize iterations # Benchmark mode: Average time, time/op # Benchmark: org.openjdk.bench.java.io.InputStreamSkipBenchmark.testSkip0 # Parameters: (inputStreamSize = 100, skipLength = 8) # Run progress: 3.57% complete, ETA 00:14:13 # Fork: 1 of 3 # Warmup Iteration 1: 432960.511 ns/op # Warmup Iteration 2: 336604.734 ns/op # Warmup Iteration 3: 330185.843 ns/op # Warmup Iteration 4: 325184.675 ns/op # Warmup Iteration 5: 328446.641 ns/op # Warmup Iteration 6: 327902.621 ns/op # Warmup Iteration 7: 330808.786 ns/op # Warmup Iteration 8: 347259.824 ns/op # Warmup Iteration 9: 325685.010 ns/op # Warmu
Re: RFR: 8279185: Support for IsoFields in JapaneseDate/MinguoDate/ThaiBuddhistDate [v6]
> Supporting `IsoFields` temporal fields in chronologies that are similar to > ISO chronology. Corresponding CSR has also been drafted. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Modified class desc of `IsoFields` - Changes: - all: https://git.openjdk.java.net/jdk/pull/7683/files - new: https://git.openjdk.java.net/jdk/pull/7683/files/7f596789..474dc85a Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7683&range=05 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7683&range=04-05 Stats: 6 lines in 2 files changed: 5 ins; 0 del; 1 mod Patch: https://git.openjdk.java.net/jdk/pull/7683.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7683/head:pull/7683 PR: https://git.openjdk.java.net/jdk/pull/7683
Re: RFR: 8186958: Need method to create pre-sized HashMap [v17]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: Copyright latest year to 2022 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/2f5617bb..4476c761 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=16 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=15-16 Stats: 24 lines in 24 files changed: 0 ins; 0 del; 24 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284853: Fix varios 'expected' typo
On Wed, 13 Apr 2022 20:36:48 GMT, Andrey Turbanov wrote: > Found various typos of expected: `exepected`, `exept`, `epectedly`, > `expeced`, `Unexpeted`, etc. Expect the Unexpeted. - Marked as reviewed by bpb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
> 8186958: Need method to create pre-sized HashMap XenoAmess has updated the pull request incrementally with one additional commit since the last revision: update LastModified - Changes: - all: https://git.openjdk.java.net/jdk/pull/7928/files - new: https://git.openjdk.java.net/jdk/pull/7928/files/4476c761..d110ecfd Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=17 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7928&range=16-17 Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7928.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7928/head:pull/7928 PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj @JoeWang-Java copyright updated to 2022. LastModified updated to 2022. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/share/classes/java/lang/Character.java line 8574:
> 8572: private static final HashMap
> aliases;
> 8573: static {
> 8574: aliases = HashMap.newHashMap(162);
@naotoj Seems like this magic number is likely to go out of date. Should there
be a test for it like the one you updated for NUM_ENTITIES?
[JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284853: Fix varios 'expected' typo
On Wed, 13 Apr 2022 20:36:48 GMT, Andrey Turbanov wrote:
> Found various typos of expected: `exepected`, `exept`, `epectedly`,
> `expeced`, `Unexpeted`, etc.
test/jdk/java/lang/StackWalker/StackStreamTest.java line 218:
> 216: private static void equalsOrThrow(String label, List list,
> List expected) {
> 217: System.out.println("List: " + list);
> 218: System.out.println("Expected: " + list);
I think there is a per-existing bug here. Shouldn't the second println print
`expected` instead of `list`?
-
PR: https://git.openjdk.java.net/jdk/pull/8231
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:40:38 GMT, Stuart Marks wrote:
>> XenoAmess has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> update LastModified
>
> src/java.base/share/classes/java/lang/Character.java line 8574:
>
>> 8572: private static final HashMap
>> aliases;
>> 8573: static {
>> 8574: aliases = HashMap.newHashMap(162);
>
> @naotoj Seems like this magic number is likely to go out of date. Should
> there be a test for it like the one you updated for NUM_ENTITIES?
> [JDK-8283465](https://bugs.openjdk.java.net/browse/JDK-8283465).
Good point! Filed an issue: https://bugs.openjdk.java.net/browse/JDK-8284856
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote: >> 8186958: Need method to create pre-sized HashMap > > XenoAmess has updated the pull request incrementally with one additional > commit since the last revision: > > update LastModified src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java line 171: > 169: _current = 0; > 170: _size = size; > 171: _references = HashMap.newHashMap(_size); Not `_size+2` ? - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284638: store skip buffers in InputStream Object [v4]
On Wed, 13 Apr 2022 21:58:06 GMT, XenoAmess wrote:
>> @jmehrens what about this then?
>> I think it safe now(actually this mechanism is learned from Reader)
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> add jmh
test/micro/org/openjdk/bench/java/io/InputStreamSkipBenchmark.java line 54:
> 52: @Benchmark
> 53: public long testSkip0(Data data) throws IOException {
> 54: TestBaseInputStream0 testBaseInputStream = new
> TestBaseInputStream0(data.inputStreamSize);
Instead of creating 3 methods with identical bodies, I recommend using an enum
to represent the type of buffers. An example at
https://github.com/openjdk/jdk/blob/7a9844cb1cd18c18ce097741cba7db1148c83de0/test/micro/org/openjdk/bench/java/util/HashMapBench.java#L65-L71
For enum values, you can name them like `LOCAL_VARIABLE`, `FIELD`,
`SOFT_REFERENCE`, which is more descriptive than current "0, 1, 2," etc.
test/micro/org/openjdk/bench/java/io/InputStreamSkipBenchmark.java line 127:
> 125:
> 126: @Override
> 127: public int read(byte[] b, int off, int len) throws IOException {
I suggest we actually write into the byte array to better simulate overheads
(maybe by generating a number with `ThreadLocalRandom`). Otherwise, the
benchmark may have exaggerated the performance gains of large skips.
To simulate overhead on each read call, you can perform some extra activity
consumed by the blackhole (possibly pass jmh blackhole through input stream
constructor)
-
PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
line 1819:
> 1817: Map items;
> 1818: LargeContainer(int size) {
> 1819: items = HashMap.newHashMap(size*2+1);
I'm wondering if we should change this to just `newHashMap(size)` since it
looks like this container is intended to hold up to `size` items. @JoeWang-Java
? I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
correct capacity for `size` mappings.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:20:14 GMT, XenoAmess wrote:
>> 8186958: Need method to create pre-sized HashMap
>
> XenoAmess has updated the pull request incrementally with one additional
> commit since the last revision:
>
> update LastModified
src/java.base/unix/classes/java/lang/ProcessEnvironment.java line 102:
> 100: /* Only for use by Runtime.exec(...String[]envp...) */
> 101: static Map emptyEnvironment(int capacity) {
> 102: return new StringEnvironment(HashMap.newHashMap(capacity));
This change is correct, since this is called with the length of an array that's
used to populate the environment. It really should be named `size` instead of
`capacity`. However the windows version of this code simply calls
`super(capacity)` as it's a subclass of `HashMap`, which is wrong. Well,
probably not worth changing this now. We may need to revisit this later to do
some cleaning up. (And also the strange computation in the static initializer
at line 71.)
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 20:06:34 GMT, Naoto Sato wrote: >> XenoAmess has updated the pull request incrementally with one additional >> commit since the last revision: >> >> revert changes in: >> src/java.desktop >> src/java.management >> src/jdk.internal.vm.ci >> src/jdk.jfr >> src/jdk.management.jfr >> src/jdk.management >> src/utils/IdealGraphVisualizer > > Looks good for changes in i18n related call sites, assuming that the > copyright years will be updated. > > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? @naotoj wrote: > Should we need some additions/modifications to the hashmap optimal capacity > utility `test/lib/jdk/test/lib/util/OptimalCapacity.java`? The only thing that uses this utility now is `test/jdk/java/lang/Enum/ConstantDirectoryOptimalCapacity.java`, which is on the problem list. The cleanup is covered by [JDK-8282120](https://bugs.openjdk.java.net/browse/JDK-8282120). After this PR gets integrated, the Enum ConstantDirectory will be initialized with `HashMap.newHashMap(universe.length)` so the OptimalCapacity test won't really be testing anything. I'll take another look at it and the library utility, but I suspect the cleanup may simply be removing them entirely. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v16]
On Wed, 13 Apr 2022 23:48:06 GMT, Stuart Marks wrote: > but I suspect the cleanup may simply be removing them entirely. +1 for removing it. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Wed, 13 Apr 2022 22:57:33 GMT, Stuart Marks wrote: > Not `_size+2` ? I don't have a idea here why he original use the + 2. Is there any guy more familiar with this code tell me why? Thanks! > I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the > correct capacity for `size` mappings. I looked the codes and don't think so.. If I'm wrong, I'm glad to fix. - PR: https://git.openjdk.java.net/jdk/pull/7928
RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
Modify native multi-byte read-write code used by the `java.io` classes to limit the size of the allocated native buffer thereby decreasing off-heap memory footprint and increasing throughput. - Commit messages: - 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available Changes: https://git.openjdk.java.net/jdk/pull/8235/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=8235&range=00 Issue: https://bugs.openjdk.java.net/browse/JDK-6478546 Stats: 93 lines in 2 files changed: 52 ins; 8 del; 33 mod Patch: https://git.openjdk.java.net/jdk/pull/8235.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/8235/head:pull/8235 PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
On Thu, 14 Apr 2022 01:40:50 GMT, Brian Burkhalter wrote: > Modify native multi-byte read-write code used by the `java.io` classes to > limit the size of the allocated native buffer thereby decreasing off-heap > memory footprint and increasing throughput. Currently for `java.io.FileInputStream.read(byte[],int,int)` and `java.io.FileOutputStream.write(byte[],int,int)`, for example, if the number of bytes respectively to be read or written exceeds 8192, an array of the total length of the read or write is allocated. For large reads or writes this could be significant. It is proposed to limit the maximum allocation size to 1 MB and perform the read or write by looping with this buffer. For reading or writing less than 1 MB, there is no effective change in the implementation. This change both saves off-heap memory and increases throughput. An allocation of 1 MB is only 0.42% the size of the buffer in the JBS issue, 501 x 501 x 501 x 2 (= 251,503,002), so for this case the memory reduction is drastic. Reading throughput is almost doubled and writing throughput improved by about 50%. As measured on macOS, the throughput of the methods mentioned above before the change was: Benchmark Mode Cnt Score Error Units ReadWrite.read thrpt5 10.108 ± 0.264 ops/s ReadWrite.write thrpt5 7.188 ± 0.431 ops/s and that after is: Benchmark Mode Cnt Score Error Units ReadWrite.read thrpt5 20.112 ± 0.262 ops/s ReadWrite.write thrpt5 10.644 ± 4.568 ops/s - PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 8282274: Compiler implementation for Pattern Matching for switch (Third Preview) [v3]
On Tue, 12 Apr 2022 13:18:14 GMT, Jan Lahoda wrote: >> This is a (preliminary) patch for javac implementation for the third preview >> of pattern matching for switch (type patterns in switches). >> >> Draft JLS: >> http://cr.openjdk.java.net/~gbierman/PatternSwitchPlusRecordPatterns/PatternSwitchPlusRecordPatterns-20220407/specs/patterns-switch-jls.html >> >> The changes are: >> -there are no guarded patterns anymore, guards are not bound to the >> CaseElement (JLS 15.28) >> -a new contextual keyword `when` is used to add a guard, instead of `&&` >> -`null` selector value is handled on switch level (if a switch has `case >> null`, it is used, otherwise a NPE is thrown), rather than on pattern >> matching level. >> -total patterns are allowed in `instanceof` >> -`java.lang.MatchException` is added for the case where a switch is >> exhaustive (due to sealed types) at compile-time, but not at runtime. >> >> Feedback is welcome! >> >> Thanks! > > Jan Lahoda has updated the pull request incrementally with one additional > commit since the last revision: > > Cleanup. src/jdk.compiler/share/classes/com/sun/tools/javac/code/Source.java line 242: > 240: PATTERN_SWITCH(JDK17, Fragments.FeaturePatternSwitch, > DiagKind.PLURAL), > 241: REDUNDANT_STRICTFP(JDK17), > 242: TOTAL_PATTERN_IN_INSTACEOF(JDK17, > Fragments.FeatureTotalPatternsInInstanceof, DiagKind.PLURAL), shouldn't be JDK19? - PR: https://git.openjdk.java.net/jdk/pull/8182
Re: RFR: 8284638: store skip buffers in InputStream Object [v2]
If you consider doing benchmarks in detail maybe consider a static buffer, too? (Especially if it can be used in multiple implementations?) Gruss Bernd -- http://bernd.eckenfels.net Von: core-libs-dev im Auftrag von XenoAmess Gesendet: Wednesday, April 13, 2022 11:58:06 PM An: core-libs-dev@openjdk.java.net Betreff: Re: RFR: 8284638: store skip buffers in InputStream Object [v2] On Wed, 13 Apr 2022 16:02:10 GMT, Alan Bateman wrote: >>> @AlanBateman You are correct about this. But I wonder if this be a problem, >>> why Reader class can afford store a skip buffer for each Reader. >>> >>> Is there anything different in the situations about skipBuffer in Reader >>> and InputStream? >> >> Maybe the skip buffer in Reader should be looked at too, esp. as it >> couldpotentially grow to 16k bytes. The concern with changing >> InputStream.skip is that there may be a lot more input streams than readers >> in use, esp. if there is an input stream for every socket connection. > >> @AlanBateman Is the concern about holding more memory sufficient to retain >> the buffer using a SoftReference so it can be freed eagerly? >> These buffers are never read from, so are quite a waste, but at least they >> are only used when >> the underlying stream overrides skip. > > Maybe but I think it needs some benchmarks to know if caching a byte[] will > help or just bloat memory. If the InputStream is to a file or socket then > maybe skip is dominated by the I/O rather than the array allocation and > zero'ing. @AlanBateman jmh test added - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 8282648: Problems due to conflicting specification of Inflater::inflate(..) and InflaterInputStream::read(..) [v7]
On Wed, 13 Apr 2022 17:42:57 GMT, Volker Simonis wrote: >> Add an API note to `InflaterInputStream::read(byte[] b, int off, int len)` >> to highlight that it might write more bytes than the returned number of >> inflated bytes into the buffer `b`. >> >> The superclass `java.io.InputStream` specifies that `read(byte[] b, int off, >> int len)` will leave the content beyond the last read byte in the read >> buffer `b` unaffected. However, the overridden `read` method in >> `InflaterInputStream` passes the read buffer `b` to >> `Inflater::inflate(byte[] b, int off, int len)` which doesn't provide this >> guarantee. Depending on implementation details, `Inflater::inflate` might >> write more than the returned number of inflated bytes into the buffer `b`. >> >> ### TL;DR >> >> `java.util.zip.Inflater` is the Java wrapper class for zlib's inflater >> functionality. `Inflater::inflate(byte[] output, int off, int len)` >> currently calls zlib's native `inflate(..)` function and passes the address >> of `output[off]` and `len` to it via JNI. >> >> The specification of zlib's `inflate(..)` function (i.e. the [API >> documentation in the original zlib >> implementation](https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/zlib.h#L400)) >> doesn't give any guarantees with regard to usage of the output buffer. It >> only states that upon completion the function will return the number of >> bytes that have been written (i.e. "inflated") into the output buffer. >> >> The original zlib implementation only wrote as many bytes into the output >> buffer as it inflated. However, this is not a hard requirement and newer, >> more performant implementations of the zlib library like >> [zlib-chromium](https://chromium.googlesource.com/chromium/src/third_party/zlib/) >> or [zlib-cloudflare](https://github.com/cloudflare/zlib) can use more bytes >> of the output buffer than they actually inflate as a scratch buffer. See >> https://github.com/simonis/zlib-chromium for a more detailed description of >> their approach and its performance benefit. >> >> These new zlib versions can still be used transparently from Java (e.g. by >> putting them into the `LD_LIBRARY_PATH` or by using `LD_PRELOAD`), because >> they still fully comply to specification of `Inflater::inflate(..)`. >> However, we might run into problems when using the `Inflater` functionality >> from the `InflaterInputStream` class. `InflaterInputStream` is derived from >> from `InputStream` and as such, its `read(byte[] b, int off, int len)` >> method is quite constrained. It specifically specifies that if *k* bytes >> have been read, then "these bytes will be stored in elements `b[off]` >> through `b[off+`*k*`-1]`, leaving elements `b[off+`*k*`]` through >> `b[off+len-1]` **unaffected**". But `InflaterInputStream::read(byte[] b, int >> off, int len)` (which is constrained by `InputStream::read(..)`'s >> specification) calls `Inflater::inflate(byte[] b, int off, int len)` and >> directly passes its output buffer down to the native zlib `inflate(..)` >> method which is free to change the bytes beyond `b[off+` *k*`]` (where *k* is the number of inflated bytes). >> >> From a practical point of view, I don't see this as a big problem, because >> callers of `InflaterInputStream::read(byte[] b, int off, int len)` can never >> know how many bytes will be written into the output buffer `b` (and in fact >> its content can always be completely overwritten). It therefore makes no >> sense to depend on any data there being untouched after the call. Also, >> having used zlib-cloudflare productively for about two years, we haven't >> seen real-world issues because of this behavior yet. However, from a >> specification point of view it is easy to artificially construct a program >> which violates `InflaterInputStream::read(..)`'s postcondition if using one >> of the alterantive zlib implementations. A recently integrated JTreg test >> (test/jdk/jdk/nio/zipfs/ZipFSOutputStreamTest.java) "unintentionally" fails >> with zlib-chromium but can fixed easily to run with alternative >> implementations as well (see >> [JDK-8283756](https://bugs.openjdk.java.net/browse/JDK-8283756)). > > Volker Simonis has updated the pull request incrementally with one additional > commit since the last revision: > > Updated wording based on @LanceAndersen's review Marked as reviewed by jpai (Committer). - PR: https://git.openjdk.java.net/jdk/pull/7986
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:15:05 GMT, XenoAmess wrote: >> src/java.xml/share/classes/com/sun/org/apache/xalan/internal/xsltc/dom/DocumentCache.java >> line 171: >> >>> 169: _current = 0; >>> 170: _size = size; >>> 171: _references = HashMap.newHashMap(_size); >> >> Not `_size+2` ? > >> Not `_size+2` ? > > I don't have a idea here why he original use the + 2. > Is there any guy more familiar with this code tell me why? > Thanks! size, not size + 2, same situation as size*2+1 below. - PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8186958: Need method to create pre-sized HashMap [v18]
On Thu, 14 Apr 2022 01:13:18 GMT, XenoAmess wrote:
>> src/java.xml/share/classes/com/sun/org/apache/xerces/internal/impl/xs/traversers/XSAttributeChecker.java
>> line 1819:
>>
>>> 1817: Map items;
>>> 1818: LargeContainer(int size) {
>>> 1819: items = HashMap.newHashMap(size*2+1);
>>
>> I'm wondering if we should change this to just `newHashMap(size)` since it
>> looks like this container is intended to hold up to `size` items.
>> @JoeWang-Java ? I suspect the `size*2+1` was a failed attempt at allocating
>> a HashMap of the correct capacity for `size` mappings.
>
>> I suspect the `size*2+1` was a failed attempt at allocating a HashMap of the
>> correct capacity for `size` mappings.
>
> I looked the codes and don't think so..
> If I'm wrong, I'm glad to fix.
Stuart's right, I looked at the code, it's as you said a failed attempt, "size"
would be good. So HashMap.newHashMap(size) would actually be a small
improvement.
It's an interesting impl the way it used HashMap, but it's 20 year code.
-
PR: https://git.openjdk.java.net/jdk/pull/7928
Re: RFR: 8284638: store skip buffers in InputStream Object
On Thu, 14 Apr 2022 02:21:23 GMT, Bernd Eckenfels wrote: > If you consider doing benchmarks in detail maybe consider a static buffer, > too? (Especially if it can be used in multiple implementations?) Why as it already be a unacceptable option for security reason? - PR: https://git.openjdk.java.net/jdk/pull/5872
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
On Thu, 14 Apr 2022 01:40:50 GMT, Brian Burkhalter wrote: > Modify native multi-byte read-write code used by the `java.io` classes to > limit the size of the allocated native buffer thereby decreasing off-heap > memory footprint and increasing throughput. The benchmark results are quite unexpected. Would we benefit from reducing the buffer size even further? - PR: https://git.openjdk.java.net/jdk/pull/8235
Re: RFR: 6478546: FileInputStream.read() throws OutOfMemoryError when there is plenty available
On Thu, 14 Apr 2022 01:40:50 GMT, Brian Burkhalter wrote: > Modify native multi-byte read-write code used by the `java.io` classes to > limit the size of the allocated native buffer thereby decreasing off-heap > memory footprint and increasing throughput. src/java.base/share/native/libjava/io_util.c line 133: > 131: if (nread == 0) > 132: nread = -1; > 133: break; Can you prototype doing the loop in Java rather than in native code so that there is less native code to maintain? - PR: https://git.openjdk.java.net/jdk/pull/8235
