Re: RFR: 8261847: performace of java.lang.Record::toString should be improved
On Tue, 16 Nov 2021 05:24:38 GMT, Vicente Romero wrote: > Please review this PR which aims to optimize the implementation of the > `toString` method we provide for records. A benchmark comparing the > implementation we are providing for records with lombok found out that lombok > is much faster mainly because our implementation uses `String::format`. This > fix is basically delegating on StringConcatFactory::makeConcatWithConstants > which is faster. > > TIA > > This is the result of the benchmark comparing records to lombok with vanilla > JDK: > > Benchmark Mode CntScoreError Units > MyBenchmark.base avgt40.849 ± 0.111 ns/op > MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op > MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op > MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op > MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op > <-- Before > MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op > MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op > > > after this patch: > > Benchmark Mode Cnt Score Error Units > MyBenchmark.base avgt4 0.680 ± 0.185 ns/op > MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op > MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op > MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op > MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op > <--- After > MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op > MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op There is a limit on the number of arguments that you can feed into `SCF`, what happens if we reach it? E.g. what if we have the Record with too many components? - PR: https://git.openjdk.java.net/jdk/pull/6403
RFR: 8261847: performace of java.lang.Record::toString should be improved
Please review this PR which aims to optimize the implementation of the `toString` method we provide for records. A benchmark comparing the implementation we are providing for records with lombok found out that lombok is much faster mainly because our implementation uses `String::format`. This fix is basically delegating on StringConcatFactory::makeConcatWithConstants which is faster. TIA This is the result of the benchmark comparing records to lombok with vanilla JDK: Benchmark Mode CntScoreError Units MyBenchmark.base avgt40.849 ± 0.111 ns/op MyBenchmark.equals_record avgt47.343 ± 2.740 ns/op MyBenchmark.equals_value avgt46.644 ± 1.920 ns/op MyBenchmark.record_hash_code avgt45.763 ± 3.882 ns/op MyBenchmark.record_to_string avgt4 262.626 ± 12.574 ns/op <-- Before MyBenchmark.value_class_to_string avgt4 30.325 ± 21.389 ns/op MyBenchmark.value_hash_codeavgt45.048 ± 3.936 ns/op after this patch: Benchmark Mode Cnt Score Error Units MyBenchmark.base avgt4 0.680 ± 0.185 ns/op MyBenchmark.equals_record avgt4 5.599 ± 1.348 ns/op MyBenchmark.equals_value avgt4 5.718 ± 4.633 ns/op MyBenchmark.record_hash_code avgt4 4.628 ± 4.368 ns/op MyBenchmark.record_to_string avgt4 26.791 ± 1.817 ns/op <--- After MyBenchmark.value_class_to_string avgt4 35.473 ± 2.626 ns/op MyBenchmark.value_hash_codeavgt4 6.152 ± 5.101 ns/op - Commit messages: - 8261847: Suboptimal java.lang.record's methods generation Changes: https://git.openjdk.java.net/jdk/pull/6403/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6403=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8261847 Stats: 56 lines in 1 file changed: 20 ins; 15 del; 21 mod Patch: https://git.openjdk.java.net/jdk/pull/6403.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6403/head:pull/6403 PR: https://git.openjdk.java.net/jdk/pull/6403
Re: RFR: 8276970: Default charset for PrintWriter that wraps PrintStream
On Mon, 15 Nov 2021 22:43:37 GMT, Naoto Sato wrote: > Fixing the default charset for PrintWriter/OutputStreamWriter that wraps a > PrintStream to its charset. This issue was raised during the conversations in > https://github.com/openjdk/jdk/pull/5771 > A corresponding CSR has also been drafted: > https://bugs.openjdk.java.net/browse/JDK-8277078 src/java.base/share/classes/java/io/PrintStream.java line 71: > 69: private boolean trouble = false; > 70: private Formatter formatter; > 71: private Charset charset; Hello Naoto, should this be formally marked as `final`? - PR: https://git.openjdk.java.net/jdk/pull/6401
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v3]
On Mon, 15 Nov 2021 17:28:44 GMT, Naoto Sato wrote: >> @naotoj , sorry for bothering you again. >> Still I got exception. It seems value "jnuEncoding" should be overwritten or >> set "fastEncoding" to "FAST_UTF_8" on jnu_util.c. >> Could you check this part again ? >> >> $ env LC_ALL=kk_KZ.pt154 >> ./build/linux-x86_64-server-release/images/jdk/bin/java Hello.java >> WARNING: The encoding of the underlying platform's file system is not >> supported: PT154 >> Error: A JNI error has occurred, please check your installation and try again >> Exception in thread "main" java.util.ServiceConfigurationError: >> java.nio.charset.spi.CharsetProvider: Provider >> sun.nio.cs.ext.ExtendedCharsets not found >> at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593) >> at >> java.base/java.util.ServiceLoader.loadProvider(ServiceLoader.java:875) >> at >> java.base/java.util.ServiceLoader$ModuleServicesLookupIterator.hasNext(ServiceLoader.java:1084) >> at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309) >> at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393) >> at >> java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:428) >> at >> java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:422) >> at >> java.base/java.security.AccessController.doPrivileged(AccessController.java:318) >> at >> java.base/java.nio.charset.Charset$ExtendedProviderHolder.extendedProviders(Charset.java:422) >> at >> java.base/java.nio.charset.Charset$ExtendedProviderHolder.(Charset.java:418) >> at >> java.base/java.nio.charset.Charset.lookupExtendedCharset(Charset.java:442) >> at java.base/java.nio.charset.Charset.lookup2(Charset.java:472) >> at java.base/java.nio.charset.Charset.lookup(Charset.java:460) >> at java.base/java.nio.charset.Charset.isSupported(Charset.java:501) >> at java.base/jdk.internal.loader.NativeLibraries.findBuiltinLib(Native >> Method) >> at >> java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:157) >> at >> java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:322) >> at >> java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:289) >> at >> java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:274) >> at >> java.base/jdk.internal.loader.BootLoader.loadLibrary(BootLoader.java:149) >> at >> java.base/sun.nio.fs.UnixNativeDispatcher.(UnixNativeDispatcher.java:667) >> at java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:65) >> at java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39) >> at >> java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:46) >> at >> java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:39) >> at >> java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.java:55) >> at >> java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvider.java:41) >> at >> java.base/sun.nio.fs.DefaultFileSystemProvider.(DefaultFileSystemProvider.java:35) >> at >> java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.getDefaultProvider(FileSystems.java:114) >> at >> java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:103) >> at >> java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:101) >> at >> java.base/java.security.AccessController.doPrivileged(AccessController.java:318) >> at >> java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.defaultFileSystem(FileSystems.java:101) >> at >> java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.(FileSystems.java:94) >> at java.base/java.nio.file.FileSystems.getDefault(FileSystems.java:183) >> at java.base/java.nio.file.Path.of(Path.java:147) >> at java.base/java.nio.file.Paths.get(Paths.java:69) >> at >> java.base/jdk.internal.jimage.ImageReaderFactory.(ImageReaderFactory.java:51) >> at >> java.base/jdk.internal.module.SystemModuleFinders$SystemImage.(SystemModuleFinders.java:385) >> at >> java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.findImageLocation(SystemModuleFinders.java:429) >> at >> java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.read(SystemModuleFinders.java:483) >> at >> java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:809) >> at >> java.base/jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(BuiltinClassLoader.java:741) >> at >> java.base/jdk.internal.loader.BuiltinClassLoader.findClass(BuiltinClassLoader.java:621) >> at java.base/java.lang.ClassLoader.loadClass(ClassLoader.java:633) >> at java.base/java.lang.Class.forName(Class.java:577) >>
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Wed, 27 Oct 2021 20:16:54 GMT, Mandy Chung wrote: >> This reimplements core reflection with method handles. >> >> For `Constructor::newInstance` and `Method::invoke`, the new implementation >> uses `MethodHandle`. For `Field` accessor, the new implementation uses >> `VarHandle`.For the first few invocations of one of these reflective >> methods on a specific reflective object we invoke the corresponding method >> handle directly. After that we spin a dynamic bytecode stub defined in a >> hidden class which loads the target `MethodHandle` or `VarHandle` from its >> class data as a dynamically computed constant. Loading the method handle >> from a constant allows JIT to inline the method-handle invocation in order >> to achieve good performance. >> >> The VM's native reflection methods are needed during early startup, before >> the method-handle mechanism is initialized. That happens soon after >> System::initPhase1 and before System::initPhase2, after which we switch to >> using method handles exclusively. >> >> The core reflection and method handle implementation are updated to handle >> chained caller-sensitive method calls [1] properly. A caller-sensitive >> method can define with a caller-sensitive adapter method that will take an >> additional caller class parameter and the adapter method will be annotated >> with `@CallerSensitiveAdapter` for better auditing. See the detailed >> description from [2]. >> >> Ran tier1-tier8 tests. >> >> [1] https://bugs.openjdk.java.net/browse/JDK-8013527 >> [2] >> https://bugs.openjdk.java.net/browse/JDK-8271820?focusedCommentId=14439430=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14439430 > > Mandy Chung has updated the pull request with a new target base due to a > merge or a rebase. The pull request now contains 43 commits: > > - fix copyright header and typo > - improve documentation of AccessorUtils > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Fall back to the VM native reflection support if method handle cannot be > created > - fix bug id in test > - Merge > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Merge branch 'master' of https://github.com/openjdk/jdk into > reimplement-method-invoke > - Separate paramFlgas into paramCount and flags fields > - Minor cleanup. Improve javadoc in CallerSensitiveAdapter > - ... and 33 more: > https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b I further looked at the specification and implementation of `Field::set` and adding `java.lang.reflect` to the trusted final field package list does not relate to this issue. I was confused with the `isTrustedFinalField` check which is supposed to check if the given `Field` object has read-only access. A `Field` object has write access if and only if - setAccessible(true) has succeeded for this Field object; - the field is non-static; and - the field's declaring class is not a hidden class; and - the field's declaring class is not a record class. The hack dropping `final` from a static final `Field` make a `Field` object which has only read-only access to get write access. With JEP 416, this hack no longer works because the read-only access is set according to the original field modifiers but not the `Field` object. - PR: https://git.openjdk.java.net/jdk/pull/5027
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
> The ZipOutputStream class may create bogus zip data which cannot be opened by > the ZipFile. The root cause is how the comment field is stored by the > ZipOutputStream. According to the zip specification, the comment field should > not be longer than 0x bytes, and we try to validate the length of the > comment, but unfortunately, we do this after the comment was assigned > already. So if the application saves the comment based on the user's input > and then gets an exception from the ZipOutputStream.setComment() it may > assume that the comment is too long and it will be ignored, but it will be > saved as-is to the file. > > Please take a look at > [this](https://github.com/openjdk/jdk/commit/c435a0905dfae827687ed46015269f9c1b36c239#diff-736e247f0ec294323891a77e16a9f0dba8bc1872131c857edf18c3f349e750eeL117) > refactoring, and note: > * The comment field is assigned before the length check > * The null comment is ignored > > The current fix will move the length validation before being assigned and > will use the null comment as an empty text. Note that the behavior of the > null parameter is not specified in the method/class/package so we are free > here to implement it in any way, any thoughts/suggestions on which is better? Sergey Bylokhov has updated the pull request incrementally with one additional commit since the last revision: Update EmptyComment.java - Changes: - all: https://git.openjdk.java.net/jdk/pull/6380/files - new: https://git.openjdk.java.net/jdk/pull/6380/files/5198d657..33617622 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6380=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6380=00-01 Stats: 50 lines in 1 file changed: 27 ins; 11 del; 12 mod Patch: https://git.openjdk.java.net/jdk/pull/6380.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6380/head:pull/6380 PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Sun, 14 Nov 2021 16:14:54 GMT, Martin Buchholz wrote: >> Sergey Bylokhov has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Update EmptyComment.java > > test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 88: > >> 86: byte[] data = baos.toByteArray(); >> 87: // Since the comment is empty this will be two last bytes >> 88: int pos = data.length - ZipFile.ENDHDR + ZipFile.ENDCOM; > > I would probably check that I could find Phil Katz' initials at data.length - > ZipFile.ENDHDR Done. - PR: https://git.openjdk.java.net/jdk/pull/6380
Re: RFR: 8277087: ZipException: zip END header not found at ZipFile#Source.findEND [v2]
On Sun, 14 Nov 2021 14:44:08 GMT, Lance Andersen wrote:
>> Sergey Bylokhov has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Update EmptyComment.java
>
> test/jdk/java/util/zip/ZipOutputStream/EmptyComment.java line 35:
>
>> 33: * @summary Verifies various use cases when the zip comment should be
>> empty
>> 34: */
>> 35: public final class EmptyComment {
>
> Please consider converting this to use TestNG and a DataProvider as it will
> simply the code even further
Done.
-
PR: https://git.openjdk.java.net/jdk/pull/6380
RFR: 8276970: Default charset for PrintWriter that wraps PrintStream
Fixing the default charset for PrintWriter/OutputStreamWriter that wraps a PrintStream to its charset. This issue was raised during the conversations in https://github.com/openjdk/jdk/pull/5771 A corresponding CSR has also been drafted: https://bugs.openjdk.java.net/browse/JDK-8277078 - Commit messages: - 8276970: Default charset for PrintWriter that wraps PrintStream Changes: https://git.openjdk.java.net/jdk/pull/6401/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6401=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276970 Stats: 129 lines in 4 files changed: 121 ins; 0 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/6401.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6401/head:pull/6401 PR: https://git.openjdk.java.net/jdk/pull/6401
Integrated: 8271515: Integration of JEP 417: Vector API (Third Incubator)
On Fri, 8 Oct 2021 21:25:26 GMT, Paul Sandoz wrote: > This PR improves the performance of vector operations that accept masks on > architectures that support masking in hardware, specifically Intel AVX512 and > ARM SVE. > > On architectures that do not support masking in hardware the same technique > as before is applied to most operations, specifically composition using blend. > > Masked loads/stores are a special form of masked operation that require > additional care to ensure out-of-bounds access throw exceptions. The range > checking has not been fully optimized and will require further work. > > No API enhancements were required and only a few additional tests were needed. This pull request has now been integrated. Changeset: a59c9b2a Author:Paul Sandoz URL: https://git.openjdk.java.net/jdk/commit/a59c9b2ac277d6ff6be1700d91ff389f137e61ca Stats: 21982 lines in 104 files changed: 16217 ins; 2087 del; 3678 mod 8271515: Integration of JEP 417: Vector API (Third Incubator) Co-authored-by: Sandhya Viswanathan Co-authored-by: Jatin Bhateja Co-authored-by: Ningsheng Jian Co-authored-by: Xiaohong Gong Co-authored-by: Eric Liu Co-authored-by: Jie Fu Co-authored-by: Vladimir Ivanov Co-authored-by: John R Rose Co-authored-by: Paul Sandoz Co-authored-by: Rado Smogura Reviewed-by: kvn, sviswanathan, ngasson - PR: https://git.openjdk.java.net/jdk/pull/5873
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
On Mon, 15 Nov 2021 19:30:54 GMT, Roger Riggs wrote: >> Roman Kennke has updated the pull request incrementally with two additional >> commits since the last revision: >> >> - Use ForceGC instead of System.gc() >> - Convert test to testng > > test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 55: > >> 53: con = null; >> 54: assert myOwnClassLoaderWeakReference.get() != null; >> 55: > > It is preferable is to write (new) tests using TestNG. > Relying on Assert to be enabled is not reliable. > It is preferable to make the checks explicit and throw RuntimeExceptions on > failure. Ok, I've changed to TestNG. Even though I often find that it's easier to debug a problem with a simple main method, instead of figuring out how to run the test in TestNG. > test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 57: > >> 55: >> 56: gc(); >> 57: > > Is the dependency on ParallelGC necessary? > To may understanding invoking System.gc() is only a request to gc and does > not reflect any idea that it has completed. > There is a function in the test library util/ForceGC to ensure gc has > completed. ParallelGC is not necessary, this was a left-over from my own testing. I've removed it. I've also changed to use ForceGC, I did not know that it exists :-) - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
On Mon, 15 Nov 2021 16:20:07 GMT, iaroslavski wrote: >> Sorting: >> >> - adopt radix sort for sequential and parallel sorts on >> int/long/float/double arrays (almost random and length > 6K) >> - fix tryMergeRuns() to better handle case when the last run is a single >> element >> - minor javadoc and comment changes >> >> Testing: >> - add new data inputs in tests for sorting >> - add min/max/infinity values to float/double testing >> - add tests for radix sort > > iaroslavski has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) > > Updated comments for partitioning As I am not an official openjdk reviewer, I can not approve, but I do support - PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive [v2]
> The caches in ObjectStreamClass basically map WeakReference to > SoftReference, where the ObjectStreamClass also references > the same Class. That means that the cache entry, and thus the class and its > class-loader, will not get reclaimed, unless the GC determines that memory > pressure is very high. > > However, this seems bogus, because that unnecessarily keeps ClassLoaders and > all its classes alive much longer than necessary: as soon as a ClassLoader > (and all its classes) become unreachable, there is no point in retaining the > stuff in OSC's caches. > > The proposed change is to use WeakReference instead of SoftReference for the > values in caches. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 > - [ ] tier4 Roman Kennke has updated the pull request incrementally with two additional commits since the last revision: - Use ForceGC instead of System.gc() - Convert test to testng - Changes: - all: https://git.openjdk.java.net/jdk/pull/6375/files - new: https://git.openjdk.java.net/jdk/pull/6375/files/430f54e5..6f099c9c Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6375=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6375=00-01 Stats: 21 lines in 1 file changed: 8 ins; 7 del; 6 mod Patch: https://git.openjdk.java.net/jdk/pull/6375.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6375/head:pull/6375 PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Mon, 1 Nov 2021 16:10:26 GMT, Ichiroh Takiguchi wrote: >> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. >> After JDK18-b13, javac and some other langtool command's usage were garbled >> on Japanese Windows. >> These commands use PrintWriter instead of standard out/err with PrintStream. > > Ichiroh Takiguchi has updated the pull request with a new target base due to > a merge or a rebase. The pull request now contains five commits: > > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - Langtools command's usage were grabled on Japanese Windows The gist of the issue is that `PrintWriter` (w/o explicit charset) uses the default charset, ignoring `PrintStream`'s charset. So it basically is irrelevant of JEP400, but apparently, JEP400 made it visible as it keeps the System.out/err encoding in `native.encoding` while changing the default to `UTF-8`. I am now in the process of creating a PR for the issue JDK-8276970, and will submit it shortly. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
On Mon, 15 Nov 2021 16:20:07 GMT, iaroslavski wrote: >> Sorting: >> >> - adopt radix sort for sequential and parallel sorts on >> int/long/float/double arrays (almost random and length > 6K) >> - fix tryMergeRuns() to better handle case when the last run is a single >> element >> - minor javadoc and comment changes >> >> Testing: >> - add new data inputs in tests for sorting >> - add min/max/infinity values to float/double testing >> - add tests for radix sort > > iaroslavski has updated the pull request incrementally with one additional > commit since the last revision: > > JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) > > Updated comments for partitioning Hi reviewers! @jhorstmann @tarsa @bourgesl @richardstartin @AlanBateman @neliasso I applied and tested all ideas/comments from Laurent and Alan, the sorting sources (3 classes) are in final state. Could you please review and approve the PR, if you don't have comments? - PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8274544: Langtools command's usage were garbled on Japanese Windows [v5]
On Mon, 1 Nov 2021 16:10:26 GMT, Ichiroh Takiguchi wrote: >> JEP-400 (UTF-8 by Default) was eabled on JDK18-b13. >> After JDK18-b13, javac and some other langtool command's usage were garbled >> on Japanese Windows. >> These commands use PrintWriter instead of standard out/err with PrintStream. > > Ichiroh Takiguchi 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 five additional > commits since the last revision: > > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - 8274544: Langtools command's usage were garbled on Japanese Windows > - Langtools command's usage were grabled on Japanese Windows Generally, I think it would be a good goal for JEP-400 to not require any source-code changes to any use-sites, at least for this common idiom of wrapping a `PrintStream` in a `PrintWriter`. While we may have the ability to change JDK use-sites, we do not have the ability to change any usages outside of JDK, and we should try not to break those usages in the way that the JDK tools have been broken. - PR: https://git.openjdk.java.net/jdk/pull/5771
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Mon, 15 Nov 2021 18:23:16 GMT, Philippe Marschall wrote: > > ``` > > 3. I made many methods just return `this` after checking for operated on > > and closed: > > ``` > > Reading the Javadoc again I'm not sure this is allowed. The method Javadoc > doesn't clearly say it but the package Javadoc for intermediate operations > says a new stream is returned. Yes, I also returned "this" initially to avoid object allocation, but fortunately escape analysis helps get rid of them if they are indeed unnecessary. My object allocation for the new empty streams is minimal. The one thing that is still hindering me is the lazy creation of streams. $ jshell | Welcome to JShell -- Version 11.0.12 | For an introduction type: /help intro jshell> var list = new ArrayList() list ==> [] jshell> var stream = list.stream() stream ==> java.util.stream.ReferencePipeline$Head@cb5822 jshell> Collections.addAll(list, 1,2,3) $3 ==> true jshell> stream.forEach(System.out::print) 123 jshell> Does anyone use this "feature"? I hope not, but unfortunately it's the current behavior and we probably have to support it :-( - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Mon, 15 Nov 2021 18:08:22 GMT, Philippe Marschall wrote: > I have a similar project at > [empty-streams](https://github.com/marschall/empty-streams). A couple of > notes: > > 1. I found the need for streams to be stateful. I had need for the following > state: > >1. closed >2. ordered >3. parallel >4. sorted I found the need for ALL the Spliterator characteristics, plus some others that I interleaved between the Spliterator bits. That way I could fit them into a single int. Parallel was too tricky though, so in that case I return a new stream built with the standard StreamSupport(spliterator, true). >5. closeHandler Similarly when someone calls closeHandler(), I also return a new stream built with StreamSupport. >6. comparator (on EmptyStream) Yes, we need this for TreeSet and ConcurrentSkipListSet. I was hoping to avoid it, but there is no way around if we want to be completely correct. > A shared instance can not be used because of `#close`. Agreed. I fixed that. > 2. I have a `PrimitiveIterator` that short circuits `#next` and > `#forEachRemaining` as well. Oh that's a good suggestion - thanks. > 3. I made many methods just return `this` after checking for operated on and > closed: > >1. `#filter` `#map`, `#flatMap`, `#mapMulti`, `#distinct`, `#peek`, > `#limit`, `#skip`, `#dropWhile`, `#takeWhile`. I now always return a new EmptyStream. Fortunately escape analysis gets rid of a lot of objects. >2. These do a state change state as well `#sequential`, `#parallel`, > `#unordered`, `#sorted`, `#onClose`. unordered() only does a state change if it currently ORDERED, otherwise it is correct to return this. The logic is convoluted and weird though. I have a more thorough test case that I need to incorporate into my EmptyStreamTest and which tests all the JDK collections, including the wrapper and immutable collections. > 4. I made my `EmptyBaseStream` implement `BaseStream` and make > `EmptyIntLongDoubleStream` extend from this class as `IntLongDoubleStream` > all extend `BaseStream`. This allowed me to move the following methods up in > the hierarchy `#isParallel` , `#onClose`, `#sequential`, `#parallel`, > `#unordered`. Interesting idea. I'm not sure if that would work for me. I will have a look if I can also do this, but I doubt it. A lot of the methods, especially the primitive ones, are repeats of each other. It might be really nice to put that into a common superclass. > 5. Is there any reason why you make `#parallel` "bail out" to `StreamSupport` > rather than just do a state change? I tried to keep the characteristics identical to what we had before and parallel seemed particularly challenging to get right. I might attempt this again at a later stage, but for now I want to keep it like it is. I don't think parallel() is the most common use case, except with very large collections perhaps. Even though I thought I was careful with the characteristics, I still have a few edge cases I need to iron out. - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Fri, 12 Nov 2021 21:43:42 GMT, Roman Kennke wrote: > The caches in ObjectStreamClass basically map WeakReference to > SoftReference, where the ObjectStreamClass also references > the same Class. That means that the cache entry, and thus the class and its > class-loader, will not get reclaimed, unless the GC determines that memory > pressure is very high. > > However, this seems bogus, because that unnecessarily keeps ClassLoaders and > all its classes alive much longer than necessary: as soon as a ClassLoader > (and all its classes) become unreachable, there is no point in retaining the > stuff in OSC's caches. > > The proposed change is to use WeakReference instead of SoftReference for the > values in caches. > > Testing: > - [x] tier1 > - [x] tier2 > - [x] tier3 > - [ ] tier4 test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 55: > 53: con = null; > 54: assert myOwnClassLoaderWeakReference.get() != null; > 55: It is preferable is to write (new) tests using TestNG. Relying on Assert to be enabled is not reliable. It is preferable to make the checks explicit and throw RuntimeExceptions on failure. test/jdk/java/io/ObjectStreamClass/TestOSCClassLoaderLeak.java line 57: > 55: > 56: gc(); > 57: Is the dependency on ParallelGC necessary? To may understanding invoking System.gc() is only a request to gc and does not reflect any idea that it has completed. There is a function in the test library util/ForceGC to ensure gc has completed. - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 4511638: Double.toString(double) sometimes produces incorrect results [v5]
> Hello, > > here's a PR for a patch submitted on March 2020 > [1](https://cr.openjdk.java.net/~bpb/4511638/webrev.04/) when Mercurial was a > thing. > > The patch has been edited to adhere to OpenJDK code conventions about > multi-line (block) comments. Nothing in the code proper has changed, except > for the addition of redundant but clarifying parentheses in some expressions. > > > Greetings > Raffaello Raffaello Giulietti has updated the pull request incrementally with one additional commit since the last revision: 4511638: Double.toString(double) sometimes produces incorrect results Enhanced intervals in MathUtils. Updated references to Schubfach v4. - Changes: - all: https://git.openjdk.java.net/jdk/pull/3402/files - new: https://git.openjdk.java.net/jdk/pull/3402/files/37acd52a..45881cd4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3402=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3402=03-04 Stats: 26 lines in 3 files changed: 0 ins; 0 del; 26 mod Patch: https://git.openjdk.java.net/jdk/pull/3402.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3402/head:pull/3402 PR: https://git.openjdk.java.net/jdk/pull/3402
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Mon, 15 Nov 2021 18:01:04 GMT, Joe Darcy wrote: > If the intent of this change is to alter the lifetimes of the objects in > question in a meaningful way, I recommend a CSR for the behavioral > compatibility impact. It would be hard for application code to observe this change: before the change, a ClassLoader and its classes could be lingering in the cache longer than necessary, even if otherwise not reachable. With the change, they would be reclaimed as soon as they become unreachable. This could only be observed, if application code holds onto ClassLoader or Class instances via Weak or PhantomReference, and even then I am not sure if that qualifies as 'meaningful'. - PR: https://git.openjdk.java.net/jdk/pull/6375
Withdrawn: 8273660: ObjectInputStream.GetField.get returns null instead of handling ClassNotFoundException
On Wed, 20 Oct 2021 21:57:29 GMT, Roger Riggs wrote: > The ObjectInputStream.GetField method `get(String name, Object val)` should > have been throwing > a ClassNotFoundException if the class was not found. Instead the > implementation was returning null. > A design error does not allow the `get(String name, Object val)` method to > throw CNFE as it should. > However, an exception must be thrown to prevent invalid data from being > returned. > Wrapping the CNFE in IOException allows it to be thrown and the exception > handled. > The call to `get(String name, Object val)` is always from within a > `readObject` method > so the deserialization logic can catch the IOException and unwrap it to > handle the CNFE. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/6053
Re: RFR: 8273660: ObjectInputStream.GetField.get returns null instead of handling ClassNotFoundException [v2]
On Fri, 29 Oct 2021 15:35:50 GMT, Roger Riggs wrote: >> The ObjectInputStream.GetField method `get(String name, Object val)` should >> have been throwing >> a ClassNotFoundException if the class was not found. Instead the >> implementation was returning null. >> A design error does not allow the `get(String name, Object val)` method to >> throw CNFE as it should. >> However, an exception must be thrown to prevent invalid data from being >> returned. >> Wrapping the CNFE in IOException allows it to be thrown and the exception >> handled. >> The call to `get(String name, Object val)` is always from within a >> `readObject` method >> so the deserialization logic can catch the IOException and unwrap it to >> handle the CNFE. > > Roger Riggs has updated the pull request incrementally with one additional > commit since the last revision: > > Correct comment on the handling of ClassNotFoundException This workaround is not the best solution for the main line. A cleaner and more robust change is proposed in JDK-8276665. - PR: https://git.openjdk.java.net/jdk/pull/6053
RFR: 8276764: Enable deterministic file content ordering for Jar and Jmod
Both jar and jmod utilise java.io file operations whose methods define no ordering of the return file lists, and in fact rely on OS query file ordering, which can differ by underlying OS architecture. This PR adds sort processing to the creation of such jar's and jmod's to enable a deterministic content ordering. Signed-off-by: Andrew Leonard - Commit messages: - 8276764: Enable deterministic file content ordering for Jar and Jmod Changes: https://git.openjdk.java.net/jdk/pull/6395/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6395=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8276764 Stats: 209 lines in 4 files changed: 199 ins; 2 del; 8 mod Patch: https://git.openjdk.java.net/jdk/pull/6395.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6395/head:pull/6395 PR: https://git.openjdk.java.net/jdk/pull/6395
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Mon, 15 Nov 2021 18:08:22 GMT, Philippe Marschall wrote: > 3. I made many methods just return `this` after checking for operated on > and closed: Reading the Javadoc again I'm not sure this is allowed. The method Javadoc doesn't clearly say it but the package Javadoc for intermediate operations says a new stream is returned. - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote: > This is a draft proposal for how we could improve stream performance for the > case where the streams are empty. Empty collections are common-place. If we > iterate over them with an Iterator, we would have to create one small > Iterator object (which could often be eliminated) and if it is empty we are > done. However, with Streams we first have to build up the entire pipeline, > until we realize that there is no work to do. With this example, we change > Collection#stream() to first check if the collection is empty, and if it is, > we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream > and EmptyDoubleStream. We have taken great care for these to have the same > characteristics and behaviour as the streams returned by Stream.empty(), > IntStream.empty(), etc. > > Some of the JDK tests fail with this, due to ClassCastExceptions (our > EmptyStream is not an AbstractPipeline) and AssertionError, since we can call > some methods repeatedly on the stream without it failing. On the plus side, > creating a complex stream on an empty stream gives us upwards of 50x increase > in performance due to a much smaller object allocation rate. This PR includes > the code for the change, unit tests and also a JMH benchmark to demonstrate > the improvement. I have a similar project at [empty-streams](https://github.com/marschall/empty-streams). A couple of notes: 1. I found the need for streams to be stateful. I had need for the following state: 1. closed 2. ordered 3. parallel 4. sorted 5. closeHandler 5. comparator (on EmptyStream) A shared instance can not be used because of `#close`. 2. I have a `PrimitiveIterator` that short circuits `#next` and `#forEachRemaining` as well. 3. I made many methods just return `this` after checking for operated on and closed: 1. `#filter` `#map`, `#flatMap`, `#mapMulti`, `#distinct`, `#peek`, `#limit`, `#skip`, `#dropWhile`, `#takeWhile`. 2. These do a state change state as well `#sequential`, `#parallel`, `#unordered`, `#sorted`, `#onClose`. 4. I made my `EmptyBaseStream` implement `BaseStream` and make `EmptyIntLongDoubleStream` extend from this class as `IntLongDoubleStream` all extend `BaseStream`. This allowed me to move the following methods up in the hierarchy `#isParallel` , `#onClose`, `#sequential`, `#parallel`, `#unordered`. 5. Is there any reason why you make `#parallel` "bail out" to `StreamSupport` rather than just do a state change? - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
On Fri, 12 Nov 2021 21:43:42 GMT, Roman Kennke wrote: > The caches in ObjectStreamClass basically map WeakReference to > SoftReference, where the ObjectStreamClass also references > the same Class. That means that the cache entry, and thus the class and its > class-loader, will not get reclaimed, unless the GC determines that memory > pressure is very high. > > However, this seems bogus, because that unnecessarily keeps ClassLoaders and > all its classes alive much longer than necessary: as soon as a ClassLoader > (and all its classes) become unreachable, there is no point in retaining the > stuff in OSC's caches. > > The proposed change is to use WeakReference instead of SoftReference for the > values in caches. > > Testing: > - [x] tier1 > - [x] tier2 > - [ ] tier3 > - [ ] tier4 If the intent of this change is to alter the lifetimes of the objects in question in a meaningful way, I recommend a CSR for the behavioral compatibility impact. - PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On Mon, 15 Nov 2021 07:33:00 GMT, Martin Grigorov wrote: >> Mandy Chung has updated the pull request with a new target base due to a >> merge or a rebase. The pull request now contains 43 commits: >> >> - fix copyright header and typo >> - improve documentation of AccessorUtils >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Fall back to the VM native reflection support if method handle cannot be >> created >> - fix bug id in test >> - Merge >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Merge branch 'master' of https://github.com/openjdk/jdk into >> reimplement-method-invoke >> - Separate paramFlgas into paramCount and flags fields >> - Minor cleanup. Improve javadoc in CallerSensitiveAdapter >> - ... and 33 more: >> https://git.openjdk.java.net/jdk/compare/9a3e9542...46cb306b > > I have already fixed our build with > https://github.com/apache/wicket/commit/191de985e22b9e0801d5783fbcfa151a25d29ec8 > and > https://github.com/apache/wicket/commit/128125f25c33a4d886386291f24ffe2d195007ac > Depending on your decision whether to make it possible again to drop `final` > for `static` fields I will either revert these changes or not. > The main purpose of my report is to let you know about this "regression". @martin-g Thanks for reporting this. Appreciated. JEP 416 makes `java.lang.reflect` classes *trusted* that reveals this another attempt to change the value of the private final `Field::modifiers` field via reflection. Since JDK 12 after https://bugs.openjdk.java.net/browse/JDK-8210496, all security-sensitive fields in `Field` and other java.lang.reflect classes are filtered from reflective access. In other words, since Java 12, `Field::modifiers` cannot be found through reflection and hence it can't be used to change the value of the modifiers of a field. The implementation of JEP 416 hardens the restriction further. To drop `final` from the modifiers, one should look into using an instrumentation agent, as Alan suggests. - PR: https://git.openjdk.java.net/jdk/pull/5027
Integrated: 8274856: Failing jpackage tests with fastdebug/release build
On Fri, 5 Nov 2021 19:58:01 GMT, Alexey Semenyuk wrote: > The fix is to isolate C++ calls in the separate forked child process on > Linux. > This change requires the passing of JLI command line arguments and values of > environment variables between two processes. This pull request has now been integrated. Changeset: fe45835f Author:Alexey Semenyuk URL: https://git.openjdk.java.net/jdk/commit/fe45835f7cebfccd4544ae19d88bdc7f07560fbe Stats: 278 lines in 8 files changed: 232 ins; 20 del; 26 mod 8274856: Failing jpackage tests with fastdebug/release build Reviewed-by: almatvee, herrick - PR: https://git.openjdk.java.net/jdk/pull/6283
Integrated: 8276084: Linux DEB Bundler: release number in outputted .deb file should be optional
On Thu, 11 Nov 2021 04:07:18 GMT, Alexey Semenyuk wrote: > 8276084: Linux DEB Bundler: release number in outputted .deb file should be > optional This pull request has now been integrated. Changeset: 9046077f Author:Alexey Semenyuk URL: https://git.openjdk.java.net/jdk/commit/9046077fe6ce7bb042fbd0fa1a80537cb4a60581 Stats: 65 lines in 7 files changed: 52 ins; 1 del; 12 mod 8276084: Linux DEB Bundler: release number in outputted .deb file should be optional Reviewed-by: almatvee, herrick - PR: https://git.openjdk.java.net/jdk/pull/6345
Supporting charset GB18030-2005
Hi there, OpenJDK currently supports version 2000 of the GB18030 (https://en.wikipedia.org/wiki/GB_18030) character set viz. GB18030-2000. The character mappings corresponding to Unicode codepoints '\u1E3F' and '\uE7C7' were swapped in a new version of the character set named GB18030-2005. I learn that this corrected a mistake in version 2000. OpenJDK does not support version 2005 as yet. Can someone please help me with reasons for the same, if any? We do have users requesting for 2005 support. While Linux (RHEL 7/8) has moved to supporting GB18030-2005 via glibc, Windows 10 and AIX 7.2 still have GB18030-2000 base. That means OpenJDK cannot move to GB18030-2005 base as yet. However, we can support both the versions until all the supported platforms move to GB18030-2005 base. Would that be an acceptable proposition? If we can have an enhancement request opened, I'd be glad to contribute the GB18030-2005 charset implementation. Thanks! Pushkar N Kulkarni, Developer, IBM Runtimes Simplicity is prerequisite for reliability - Edsger W. Dijkstra
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v3]
On Mon, 15 Nov 2021 02:40:14 GMT, Ichiroh Takiguchi wrote: >> I reproduced those failures on Debian Linux. Corrected the issue by >> replacing unsupported jnu encoding with `UTF-8` in `System::initPhase1`. > > @naotoj , sorry for bothering you again. > Still I got exception. It seems value "jnuEncoding" should be overwritten or > set "fastEncoding" to "FAST_UTF_8" on jnu_util.c. > Could you check this part again ? > > $ env LC_ALL=kk_KZ.pt154 > ./build/linux-x86_64-server-release/images/jdk/bin/java Hello.java > WARNING: The encoding of the underlying platform's file system is not > supported: PT154 > Error: A JNI error has occurred, please check your installation and try again > Exception in thread "main" java.util.ServiceConfigurationError: > java.nio.charset.spi.CharsetProvider: Provider > sun.nio.cs.ext.ExtendedCharsets not found > at java.base/java.util.ServiceLoader.fail(ServiceLoader.java:593) > at > java.base/java.util.ServiceLoader.loadProvider(ServiceLoader.java:875) > at > java.base/java.util.ServiceLoader$ModuleServicesLookupIterator.hasNext(ServiceLoader.java:1084) > at java.base/java.util.ServiceLoader$2.hasNext(ServiceLoader.java:1309) > at java.base/java.util.ServiceLoader$3.hasNext(ServiceLoader.java:1393) > at > java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:428) > at > java.base/java.nio.charset.Charset$ExtendedProviderHolder$1.run(Charset.java:422) > at > java.base/java.security.AccessController.doPrivileged(AccessController.java:318) > at > java.base/java.nio.charset.Charset$ExtendedProviderHolder.extendedProviders(Charset.java:422) > at > java.base/java.nio.charset.Charset$ExtendedProviderHolder.(Charset.java:418) > at > java.base/java.nio.charset.Charset.lookupExtendedCharset(Charset.java:442) > at java.base/java.nio.charset.Charset.lookup2(Charset.java:472) > at java.base/java.nio.charset.Charset.lookup(Charset.java:460) > at java.base/java.nio.charset.Charset.isSupported(Charset.java:501) > at java.base/jdk.internal.loader.NativeLibraries.findBuiltinLib(Native > Method) > at > java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:157) > at > java.base/jdk.internal.loader.NativeLibraries.findFromPaths(NativeLibraries.java:322) > at > java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:289) > at > java.base/jdk.internal.loader.NativeLibraries.loadLibrary(NativeLibraries.java:274) > at > java.base/jdk.internal.loader.BootLoader.loadLibrary(BootLoader.java:149) > at > java.base/sun.nio.fs.UnixNativeDispatcher.(UnixNativeDispatcher.java:667) > at java.base/sun.nio.fs.UnixFileSystem.(UnixFileSystem.java:65) > at java.base/sun.nio.fs.LinuxFileSystem.(LinuxFileSystem.java:39) > at > java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:46) > at > java.base/sun.nio.fs.LinuxFileSystemProvider.newFileSystem(LinuxFileSystemProvider.java:39) > at > java.base/sun.nio.fs.UnixFileSystemProvider.(UnixFileSystemProvider.java:55) > at > java.base/sun.nio.fs.LinuxFileSystemProvider.(LinuxFileSystemProvider.java:41) > at > java.base/sun.nio.fs.DefaultFileSystemProvider.(DefaultFileSystemProvider.java:35) > at > java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.getDefaultProvider(FileSystems.java:114) > at > java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:103) > at > java.base/java.nio.file.FileSystems$DefaultFileSystemHolder$1.run(FileSystems.java:101) > at > java.base/java.security.AccessController.doPrivileged(AccessController.java:318) > at > java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.defaultFileSystem(FileSystems.java:101) > at > java.base/java.nio.file.FileSystems$DefaultFileSystemHolder.(FileSystems.java:94) > at java.base/java.nio.file.FileSystems.getDefault(FileSystems.java:183) > at java.base/java.nio.file.Path.of(Path.java:147) > at java.base/java.nio.file.Paths.get(Paths.java:69) > at > java.base/jdk.internal.jimage.ImageReaderFactory.(ImageReaderFactory.java:51) > at > java.base/jdk.internal.module.SystemModuleFinders$SystemImage.(SystemModuleFinders.java:385) > at > java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.findImageLocation(SystemModuleFinders.java:429) > at > java.base/jdk.internal.module.SystemModuleFinders$SystemModuleReader.read(SystemModuleFinders.java:483) > at > java.base/jdk.internal.loader.BuiltinClassLoader.defineClass(BuiltinClassLoader.java:809) > at > java.base/jdk.internal.loader.BuiltinClassLoader.findClassInModuleOrNull(BuiltinClassLoader.java:741) > at > java.base/jdk.internal.loader.BuiltinClassLoader.findClass(BuiltinClassLoader.java:621) > at
Re: RFR: 8275007: Java fails to start with null charset if LC_ALL is set to certain locales [v7]
> Please review the subject fix. In light of JEP400, Java runtime can/should > start in UTF-8 charset if the underlying native encoding is not supported. Naoto Sato has updated the pull request incrementally with one additional commit since the last revision: Replace jnuEncoding in jni_util.c with UTF-8, if platform encoding is not supported - Changes: - all: https://git.openjdk.java.net/jdk/pull/6282/files - new: https://git.openjdk.java.net/jdk/pull/6282/files/3428cdf4..c241e8da Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=6282=06 - incr: https://webrevs.openjdk.java.net/?repo=jdk=6282=05-06 Stats: 70 lines in 1 file changed: 20 ins; 37 del; 13 mod Patch: https://git.openjdk.java.net/jdk/pull/6282.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6282/head:pull/6282 PR: https://git.openjdk.java.net/jdk/pull/6282
RFR: 8277072: ObjectStreamClass caches keep ClassLoaders alive
The caches in ObjectStreamClass basically map WeakReference to SoftReference, where the ObjectStreamClass also references the same Class. That means that the cache entry, and thus the class and its class-loader, will not get reclaimed, unless the GC determines that memory pressure is very high. However, this seems bogus, because that unnecessarily keeps ClassLoaders and all its classes alive much longer than necessary: as soon as a ClassLoader (and all its classes) become unreachable, there is no point in retaining the stuff in OSC's caches. The proposed change is to use WeakReference instead of SoftReference for the values in caches. Testing: - [x] tier1 - [x] tier2 - [ ] tier3 - [ ] tier4 - Commit messages: - Fix indentation of new testcase - 8277072: ObjectStreamClass caches keep ClassLoaders alive Changes: https://git.openjdk.java.net/jdk/pull/6375/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6375=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277072 Stats: 112 lines in 2 files changed: 107 ins; 1 del; 4 mod Patch: https://git.openjdk.java.net/jdk/pull/6375.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6375/head:pull/6375 PR: https://git.openjdk.java.net/jdk/pull/6375
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v10]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request 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 10 additional commits since the last revision: - Merge branch 'openjdk:master' into JDK-8193682 - 8193682: Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682 : Infinite loop in ZipOutputStream.close() - 8193682: Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/c315ab21..c66535fc Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=09 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=08-09 Stats: 937207 lines in 2953 files changed: 489188 ins; 435324 del; 12695 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) [v9]
> Sorting: > > - adopt radix sort for sequential and parallel sorts on int/long/float/double > arrays (almost random and length > 6K) > - fix tryMergeRuns() to better handle case when the last run is a single > element > - minor javadoc and comment changes > > Testing: > - add new data inputs in tests for sorting > - add min/max/infinity values to float/double testing > - add tests for radix sort iaroslavski has updated the pull request incrementally with one additional commit since the last revision: JDK-8266431: Dual-Pivot Quicksort improvements (Radix sort) Updated comments for partitioning - Changes: - all: https://git.openjdk.java.net/jdk/pull/3938/files - new: https://git.openjdk.java.net/jdk/pull/3938/files/e1b01cfb..4baa9a39 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=3938=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=3938=07-08 Stats: 54 lines in 1 file changed: 0 ins; 0 del; 54 mod Patch: https://git.openjdk.java.net/jdk/pull/3938.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/3938/head:pull/3938 PR: https://git.openjdk.java.net/jdk/pull/3938
Re: RFR: 8193682: Infinite loop in ZipOutputStream.close() [v9]
> Hi all, > > Please review this fix for Infinite loop in ZipOutputStream.close(). > The main issue here is when ever there is an exception during close > operations on GZip we are not setting the deflator to a finished state which > is leading to an infinite loop when we try writing on the same GZip instance( > since we use while(!def.finished()) inside the write operation). > > Thanks, > Ravi Ravi Reddy has updated the pull request incrementally with one additional commit since the last revision: 8193682: Infinite loop in ZipOutputStream.close() - Changes: - all: https://git.openjdk.java.net/jdk/pull/5522/files - new: https://git.openjdk.java.net/jdk/pull/5522/files/b3d7fb74..c315ab21 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=5522=08 - incr: https://webrevs.openjdk.java.net/?repo=jdk=5522=07-08 Stats: 30 lines in 2 files changed: 16 ins; 0 del; 14 mod Patch: https://git.openjdk.java.net/jdk/pull/5522.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/5522/head:pull/5522 PR: https://git.openjdk.java.net/jdk/pull/5522
Re: RFR: 8227020: Unclosed input streams in URL Class Loader for JARs with META-INF/INDEX.LIST
On Mon, 15 Nov 2021 13:56:18 GMT, Alan Bateman wrote: > I can't quite tell if you have something that runs into the issue or whether > you just spotted the open issue Hello Alan, I just happened to notice this issue in JBS and thought of checking if there's any interest in this fix. From the inputs I've received so far both on this PR and in a private discussion, it looks like this fix may not be of interest in the mainline master branch? If that's the case, I don't mind closing this PR. As for backports, I think this fix (if it looks fine) does add value for Java 11 and 17 where jar indexing is enabled by default. However, I guess for this specific case, I'll have to directly open a fix request/PR against those repos instead of first fixing it here? - PR: https://git.openjdk.java.net/jdk/pull/6389
Re: RFR: 8227020: Unclosed input streams in URL Class Loader for JARs with META-INF/INDEX.LIST
On Mon, 15 Nov 2021 13:26:30 GMT, Jaikiran Pai wrote: > Hello Daniel, The issue is still reproducible (only) if the jar indexing is > enabled by setting the `-Djdk.net.URLClassPath.enableJarIndex=true` Just to add to Daniel's comment. JAR indexing has several issues, most going back 20+ years. We toyed with the idea of removing it in JDK 18 but decided to keep it disabled for now with a view to removing the code in a future release. It's not clear to me that it's worth trying to fix issues but maybe the motive is to back-port a fix to an older release? (I can't quite tell if you have something that runs into the issue or whether you just spotted the open issue). - PR: https://git.openjdk.java.net/jdk/pull/6389
Re: RFR: 8227020: Unclosed input streams in URL Class Loader for JARs with META-INF/INDEX.LIST
On Mon, 15 Nov 2021 13:25:01 GMT, Daniel Fuchs wrote: > Hi Jaikiran, is this still an issue now that Jar Index is disabled by > default? (see https://git.openjdk.java.net/jdk/pull/5524) Hello Daniel, The issue is still reproducible (only) if the jar indexing is enabled by setting the `-Djdk.net.URLClassPath.enableJarIndex=true` - PR: https://git.openjdk.java.net/jdk/pull/6389
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sun, 7 Nov 2021 06:53:12 GMT, kabutz wrote:
>>> The net effect of this change might depend on your workload. If you call
>>> stream() on empty collections that have cheap isEmpty(), this change will
>>> likely improve performance and reduce waste. However, this same change
>>> might do the opposite if some of your collections aren't empty or have
>>> costly isEmpty(). It would be good to have benchmarks for different
>>> workloads.
>>
>> Yes, I also thought about the cost of isEmpty() on concurrent collections.
>> There are four concurrent collections that have a linear time cost size()
>> method: CLQ, CLD, LTQ and CHM. However, in each of these cases, the
>> isEmpty() method has constant time cost. There might be collections defined
>> outside the JDK where this could be the case.
>>
>> However, I will extend the benchmark to include a few of those cases too, as
>> well as different sizes and collection sizes.
>>
>> Thank you so much for your input.
>
>> wouldn't this make streams no longer lazy if the collection is empty?
>>
>> ```java
>> List list = new ArrayList<>();
>> Stream stream = list.stream();
>>
>> list.addAll(List.of("one", "two", "three"));
>>
>> stream.forEach(System.out::println); // prints one two three
>> ```
>
> I did not consider this case, thank you for bringing it up. I have always
> found this behaviour a bit strange and have never used it "in the real
> world". It is also not consistent between collections. Here is an example
> with four collections: ArrayList, CopyOnWriteArrayList, ConcurrentSkipListSet
> and ArrayBlockingQueue:
>
>
> import java.util.ArrayList;
> import java.util.Arrays;
> import java.util.Collection;
> import java.util.List;
> import java.util.Objects;
> import java.util.concurrent.ArrayBlockingQueue;
> import java.util.concurrent.ConcurrentSkipListSet;
> import java.util.concurrent.CopyOnWriteArrayList;
> import java.util.function.Supplier;
> import java.util.stream.IntStream;
>
> public class LazyStreamDemo {
> public static void main(String... args) {
> List>> suppliers =
> List.of(ArrayList::new, // fast-fail
> CopyOnWriteArrayList::new, // snapshot
> ConcurrentSkipListSet::new, // weakly-consistent
> () -> new ArrayBlockingQueue<>(10) //
> weakly-consistent
> );
> for (Supplier> supplier : suppliers) {
> Collection c = supplier.get();
> System.out.println(c.getClass());
> IntStream stream = c.stream()
> .sorted()
> .filter(Objects::nonNull)
> .mapToInt(String::length)
> .sorted();
>
> c.addAll(List.of("one", "two", "three", "four", "five"));
> System.out.println("stream = " +
> Arrays.toString(stream.toArray()));
> }
> }
> }
>
>
> The output is:
>
>
> class java.util.ArrayList
> stream = [3, 3, 4, 4, 5]
> class java.util.concurrent.CopyOnWriteArrayList
> stream = []
> class java.util.concurrent.ConcurrentSkipListSet
> stream = []
> class java.util.concurrent.ArrayBlockingQueue
> stream = [3, 3, 4, 4, 5]
>
>
> At least with the EmptyStream we would have consistent output of always []
@kabutz I agree that i wouldn't consider it clean code to use a stream like i
put into the example. I only brought it up because it might break existing
code, since i think streams are expected to be lazy. Interesting to see that
they are in fact not lazy in all situations - i put that into my notes.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Wed, 10 Nov 2021 07:45:27 GMT, Anthony Vanelverdinghe
wrote:
>> @kabutz I agree that i wouldn't consider it clean code to use a stream like
>> i put into the example. I only brought it up because it might break existing
>> code, since i think streams are expected to be lazy. Interesting to see that
>> they are in fact not lazy in all situations - i put that into my notes.
>
> Edit: actually I think the implementation of `Collection::stream` could
> simply be changed to:
>
>
> default Stream stream() {
> var spliterator = spliterator();
> if(spliterator.hasCharacteristics(Spliterator.IMMUTABLE |
> Spliterator.CONCURRENT) && isEmpty()) {
> return Stream.empty();
> }
> return StreamSupport.stream(spliterator, false);
> }
>
>
> Note that the spliterators of methods such as `Collections::emptyList`,
> `List::of`, `List::copyOf`, `Set::of`, ... currently don't have the
> `IMMUTABLE` characteristic though, so they should be updated.
>
> ---
>
> The Javadoc for
> [java.util.stream](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/package-summary.html)
> is clear though:
>
>> Traversal of the pipeline source does not begin until the terminal operation
>> of the pipeline is executed.
>
> and further on says:
>
>> Spliterators for mutable data sources have an additional challenge; timing
>> of binding to the data, since the data could change between the time the
>> spliterator is created and the time the stream pipeline is executed.
>> Ideally, a spliterator for a stream would report a characteristic of
>> IMMUTABLE or CONCURRENT; if not it should be late-binding.
>
> which explains why the collections in `java.util.concurrent` (whose
> spliterators have one of those characteristics) need not be late-binding.
Thanks @anthonyvdotbe I believe that would satisfy the lazy binding, and then
we should increase the use of the IMMUTABLE characteristic where it makes sense.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sun, 7 Nov 2021 07:51:06 GMT, Michael Bien wrote:
>>> wouldn't this make streams no longer lazy if the collection is empty?
>>>
>>> ```java
>>> List list = new ArrayList<>();
>>> Stream stream = list.stream();
>>>
>>> list.addAll(List.of("one", "two", "three"));
>>>
>>> stream.forEach(System.out::println); // prints one two three
>>> ```
>>
>> I did not consider this case, thank you for bringing it up. I have always
>> found this behaviour a bit strange and have never used it "in the real
>> world". It is also not consistent between collections. Here is an example
>> with four collections: ArrayList, CopyOnWriteArrayList,
>> ConcurrentSkipListSet and ArrayBlockingQueue:
>>
>>
>> import java.util.ArrayList;
>> import java.util.Arrays;
>> import java.util.Collection;
>> import java.util.List;
>> import java.util.Objects;
>> import java.util.concurrent.ArrayBlockingQueue;
>> import java.util.concurrent.ConcurrentSkipListSet;
>> import java.util.concurrent.CopyOnWriteArrayList;
>> import java.util.function.Supplier;
>> import java.util.stream.IntStream;
>>
>> public class LazyStreamDemo {
>> public static void main(String... args) {
>> List>> suppliers =
>> List.of(ArrayList::new, // fast-fail
>> CopyOnWriteArrayList::new, // snapshot
>> ConcurrentSkipListSet::new, // weakly-consistent
>> () -> new ArrayBlockingQueue<>(10) //
>> weakly-consistent
>> );
>> for (Supplier> supplier : suppliers) {
>> Collection c = supplier.get();
>> System.out.println(c.getClass());
>> IntStream stream = c.stream()
>> .sorted()
>> .filter(Objects::nonNull)
>> .mapToInt(String::length)
>> .sorted();
>>
>> c.addAll(List.of("one", "two", "three", "four", "five"));
>> System.out.println("stream = " +
>> Arrays.toString(stream.toArray()));
>> }
>> }
>> }
>>
>>
>> The output is:
>>
>>
>> class java.util.ArrayList
>> stream = [3, 3, 4, 4, 5]
>> class java.util.concurrent.CopyOnWriteArrayList
>> stream = []
>> class java.util.concurrent.ConcurrentSkipListSet
>> stream = []
>> class java.util.concurrent.ArrayBlockingQueue
>> stream = [3, 3, 4, 4, 5]
>>
>>
>> At least with the EmptyStream we would have consistent output of always []
>
> @kabutz I agree that i wouldn't consider it clean code to use a stream like i
> put into the example. I only brought it up because it might break existing
> code, since i think streams are expected to be lazy. Interesting to see that
> they are in fact not lazy in all situations - i put that into my notes.
Edit: actually I think the implementation of `Collection::stream` could simply
be changed to:
default Stream stream() {
var spliterator = spliterator();
if(spliterator.hasCharacteristics(Spliterator.IMMUTABLE |
Spliterator.CONCURRENT) && isEmpty()) {
return Stream.empty();
}
return StreamSupport.stream(spliterator, false);
}
Note that the spliterators of methods such as `Collections::emptyList`,
`List::of`, `List::copyOf`, `Set::of`, ... currently don't have the `IMMUTABLE`
characteristic though, so they should be updated.
---
The Javadoc for
[java.util.stream](https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/stream/package-summary.html)
is clear though:
> Traversal of the pipeline source does not begin until the terminal operation
> of the pipeline is executed.
and further on says:
> Spliterators for mutable data sources have an additional challenge; timing of
> binding to the data, since the data could change between the time the
> spliterator is created and the time the stream pipeline is executed. Ideally,
> a spliterator for a stream would report a characteristic of IMMUTABLE or
> CONCURRENT; if not it should be late-binding.
which explains why the collections in `java.util.concurrent` (whose
spliterators have one of those characteristics) need not be late-binding.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sun, 7 Nov 2021 06:26:22 GMT, kabutz wrote:
>> (immutable collections could override stream() instead, since they don't
>> have that problem)
>
>> The net effect of this change might depend on your workload. If you call
>> stream() on empty collections that have cheap isEmpty(), this change will
>> likely improve performance and reduce waste. However, this same change might
>> do the opposite if some of your collections aren't empty or have costly
>> isEmpty(). It would be good to have benchmarks for different workloads.
>
> Yes, I also thought about the cost of isEmpty() on concurrent collections.
> There are four concurrent collections that have a linear time cost size()
> method: CLQ, CLD, LTQ and CHM. However, in each of these cases, the isEmpty()
> method has constant time cost. There might be collections defined outside the
> JDK where this could be the case.
>
> However, I will extend the benchmark to include a few of those cases too, as
> well as different sizes and collection sizes.
>
> Thank you so much for your input.
> wouldn't this make streams no longer lazy if the collection is empty?
>
> ```java
> List list = new ArrayList<>();
> Stream stream = list.stream();
>
> list.addAll(List.of("one", "two", "three"));
>
> stream.forEach(System.out::println); // prints one two three
> ```
I did not consider this case, thank you for bringing it up. I have always found
this behaviour a bit strange and have never used it "in the real world". It is
also not consistent between collections. Here is an example with four
collections: ArrayList, CopyOnWriteArrayList, ConcurrentSkipListSet and
ArrayBlockingQueue:
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Objects;
import java.util.concurrent.ArrayBlockingQueue;
import java.util.concurrent.ConcurrentSkipListSet;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.function.Supplier;
import java.util.stream.IntStream;
public class LazyStreamDemo {
public static void main(String... args) {
List>> suppliers =
List.of(ArrayList::new, // fast-fail
CopyOnWriteArrayList::new, // snapshot
ConcurrentSkipListSet::new, // weakly-consistent
() -> new ArrayBlockingQueue<>(10) // weakly-consistent
);
for (Supplier> supplier : suppliers) {
Collection c = supplier.get();
System.out.println(c.getClass());
IntStream stream = c.stream()
.sorted()
.filter(Objects::nonNull)
.mapToInt(String::length)
.sorted();
c.addAll(List.of("one", "two", "three", "four", "five"));
System.out.println("stream = " + Arrays.toString(stream.toArray()));
}
}
}
The output is:
class java.util.ArrayList
stream = [3, 3, 4, 4, 5]
class java.util.concurrent.CopyOnWriteArrayList
stream = []
class java.util.concurrent.ConcurrentSkipListSet
stream = []
class java.util.concurrent.ArrayBlockingQueue
stream = [3, 3, 4, 4, 5]
At least with the EmptyStream we would have consistent output of always []
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sun, 7 Nov 2021 04:26:13 GMT, Michael Bien wrote:
>> wouldn't this make streams no longer lazy if the collection is empty?
>>
>> List list = new ArrayList<>();
>> Stream stream = list.stream();
>>
>> list.addAll(List.of("one", "two", "three"));
>>
>> stream.forEach(System.out::println); // prints one two three
>
> (immutable collections could override stream() instead, since they don't have
> that problem)
> The net effect of this change might depend on your workload. If you call
> stream() on empty collections that have cheap isEmpty(), this change will
> likely improve performance and reduce waste. However, this same change might
> do the opposite if some of your collections aren't empty or have costly
> isEmpty(). It would be good to have benchmarks for different workloads.
Yes, I also thought about the cost of isEmpty() on concurrent collections.
There are four concurrent collections that have a linear time cost size()
method: CLQ, CLD, LTQ and CHM. However, in each of these cases, the isEmpty()
method has constant time cost. There might be collections defined outside the
JDK where this could be the case.
However, I will extend the benchmark to include a few of those cases too, as
well as different sizes and collection sizes.
Thank you so much for your input.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sat, 6 Nov 2021 22:03:26 GMT, Pavel Rappo wrote:
>> This is a draft proposal for how we could improve stream performance for the
>> case where the streams are empty. Empty collections are common-place. If we
>> iterate over them with an Iterator, we would have to create one small
>> Iterator object (which could often be eliminated) and if it is empty we are
>> done. However, with Streams we first have to build up the entire pipeline,
>> until we realize that there is no work to do. With this example, we change
>> Collection#stream() to first check if the collection is empty, and if it is,
>> we simply return an EmptyStream. We also have EmptyIntStream,
>> EmptyLongStream and EmptyDoubleStream. We have taken great care for these to
>> have the same characteristics and behaviour as the streams returned by
>> Stream.empty(), IntStream.empty(), etc.
>>
>> Some of the JDK tests fail with this, due to ClassCastExceptions (our
>> EmptyStream is not an AbstractPipeline) and AssertionError, since we can
>> call some methods repeatedly on the stream without it failing. On the plus
>> side, creating a complex stream on an empty stream gives us upwards of 50x
>> increase in performance due to a much smaller object allocation rate. This
>> PR includes the code for the change, unit tests and also a JMH benchmark to
>> demonstrate the improvement.
>
> src/java.base/share/classes/java/util/Collection.java line 743:
>
>> 741: */
>> 742: default Stream stream() {
>> 743: if (isEmpty()) return Stream.empty();
>
> The net effect of this change might depend on your workload. If you call
> stream() on empty collections that have cheap isEmpty(), this change will
> likely improve performance and reduce waste. However, this same change might
> do the opposite if some of your collections aren't empty or have costly
> isEmpty(). It would be good to have benchmarks for different workloads.
wouldn't this make streams no longer lazy if the collection is empty?
List list = new ArrayList<>();
Stream stream = list.stream();
list.addAll(List.of("one", "two", "three"));
stream.forEach(System.out::println); // prints one two three
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sun, 7 Nov 2021 03:53:29 GMT, Michael Bien wrote:
>> src/java.base/share/classes/java/util/Collection.java line 743:
>>
>>> 741: */
>>> 742: default Stream stream() {
>>> 743: if (isEmpty()) return Stream.empty();
>>
>> The net effect of this change might depend on your workload. If you call
>> stream() on empty collections that have cheap isEmpty(), this change will
>> likely improve performance and reduce waste. However, this same change might
>> do the opposite if some of your collections aren't empty or have costly
>> isEmpty(). It would be good to have benchmarks for different workloads.
>
> wouldn't this make streams no longer lazy if the collection is empty?
>
> List list = new ArrayList<>();
> Stream stream = list.stream();
>
> list.addAll(List.of("one", "two", "three"));
>
> stream.forEach(System.out::println); // prints one two three
(immutable collections could override stream() instead, since they don't have
that problem)
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sat, 13 Nov 2021 16:59:10 GMT, liach wrote:
>> This is a draft proposal for how we could improve stream performance for the
>> case where the streams are empty. Empty collections are common-place. If we
>> iterate over them with an Iterator, we would have to create one small
>> Iterator object (which could often be eliminated) and if it is empty we are
>> done. However, with Streams we first have to build up the entire pipeline,
>> until we realize that there is no work to do. With this example, we change
>> Collection#stream() to first check if the collection is empty, and if it is,
>> we simply return an EmptyStream. We also have EmptyIntStream,
>> EmptyLongStream and EmptyDoubleStream. We have taken great care for these to
>> have the same characteristics and behaviour as the streams returned by
>> Stream.empty(), IntStream.empty(), etc.
>>
>> Some of the JDK tests fail with this, due to ClassCastExceptions (our
>> EmptyStream is not an AbstractPipeline) and AssertionError, since we can
>> call some methods repeatedly on the stream without it failing. On the plus
>> side, creating a complex stream on an empty stream gives us upwards of 50x
>> increase in performance due to a much smaller object allocation rate. This
>> PR includes the code for the change, unit tests and also a JMH benchmark to
>> demonstrate the improvement.
>
> src/java.base/share/classes/java/util/Arrays.java line 5448:
>
>> 5446: public static Stream stream(T[] array, int startInclusive,
>> int endExclusive) {
>> 5447: var spliterator = spliterator(array, startInclusive,
>> endExclusive);
>> 5448: if (startInclusive == endExclusive) return Stream.empty();
>
> Can't we just add the `if` statement to before the `spliterator` is computed?
Thanks @liach for the suggestion. The spliterator serves the purpose of
checking the arguments. For example, if array is null, the method should throw
a NPE. Similarly, startInclusive and endExclusive have to be in the range of
the array length. If we swap around the lines, someone could call stream(null,
-100, -100) and it would happily return an empty stream.
Furthermore, the empty streams can inherit characteristics of a spliterator. In
the code you pointed out, I don't do that, but will change that. It makes it
much easier to keep the empty streams consistent with their previous behavior.
One might argue that it is pointless to call distinct() sorted() and
unordered() on an empty stream and expect it to change, but it was easy enough
to implement so that we can keep consistency FWIW.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote:
> This is a draft proposal for how we could improve stream performance for the
> case where the streams are empty. Empty collections are common-place. If we
> iterate over them with an Iterator, we would have to create one small
> Iterator object (which could often be eliminated) and if it is empty we are
> done. However, with Streams we first have to build up the entire pipeline,
> until we realize that there is no work to do. With this example, we change
> Collection#stream() to first check if the collection is empty, and if it is,
> we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream
> and EmptyDoubleStream. We have taken great care for these to have the same
> characteristics and behaviour as the streams returned by Stream.empty(),
> IntStream.empty(), etc.
>
> Some of the JDK tests fail with this, due to ClassCastExceptions (our
> EmptyStream is not an AbstractPipeline) and AssertionError, since we can call
> some methods repeatedly on the stream without it failing. On the plus side,
> creating a complex stream on an empty stream gives us upwards of 50x increase
> in performance due to a much smaller object allocation rate. This PR includes
> the code for the change, unit tests and also a JMH benchmark to demonstrate
> the improvement.
src/java.base/share/classes/java/util/Arrays.java line 5448:
> 5446: public static Stream stream(T[] array, int startInclusive,
> int endExclusive) {
> 5447: var spliterator = spliterator(array, startInclusive,
> endExclusive);
> 5448: if (startInclusive == endExclusive) return Stream.empty();
Can't we just add the `if` statement to before the `spliterator` is computed?
-
PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote: > This is a draft proposal for how we could improve stream performance for the > case where the streams are empty. Empty collections are common-place. If we > iterate over them with an Iterator, we would have to create one small > Iterator object (which could often be eliminated) and if it is empty we are > done. However, with Streams we first have to build up the entire pipeline, > until we realize that there is no work to do. With this example, we change > Collection#stream() to first check if the collection is empty, and if it is, > we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream > and EmptyDoubleStream. We have taken great care for these to have the same > characteristics and behaviour as the streams returned by Stream.empty(), > IntStream.empty(), etc. > > Some of the JDK tests fail with this, due to ClassCastExceptions (our > EmptyStream is not an AbstractPipeline) and AssertionError, since we can call > some methods repeatedly on the stream without it failing. On the plus side, > creating a complex stream on an empty stream gives us upwards of 50x increase > in performance due to a much smaller object allocation rate. This PR includes > the code for the change, unit tests and also a JMH benchmark to demonstrate > the improvement. I've added far more JMH tests to check for stream size of [0, 1, 10, 100], then different types of streams, from minimal to complex, then five different collections ArrayList, ConcurrentLinkedQueue, ConcurrentSkipListSet, CopyOnWriteArrayList, ConcurrentHashMap.newKeySet() and lastly with Stream.empty() that has the new behavior and StreamSupport.stream(...) that was the old behavior. With all this multiplication of test options, it will take a couple of days to run on my server. Will let you know if anything surprising pops up. Thanks for your excellent suggestions. It seemed that we cannot get away from making some objects. I've got a new version in the works that makes EmptyStream instances each time, with their own state. The performance is still good. For a simple stream pipeline, it is roughly twice as fast for an empty stream. There is no noticeable slow-down for non-empty streams. `EmptyStreams` now follow the same behavior as we would get if we created the stream with `StreamSupport.stream(collection.spliterator(), false)`. For example, we cannot call `filter()` / `map()` etc. twice on the same stream. We can call `unordered()` twice on the same stream, but only if it was not `ORDERED` to begin with. Streams are not created lazily yet in my updated version, but I'm hoping it is a step in the right direction. I'm running an extensive JMH suite overnight to compare object allocation rates and throughput for the streams. Here are the maximum throughput numbers for the JMH benchmark for various lengths, collections, type of stream decorations and whether it is the old style stream creation or the new EmptyStream. We also see the speedup with the new system. There are some strange effects where we see small differences once we get past 0 length, which is most likely to be explained because of noise in the benchmark. The speedup in the benchmark for empty streams seems to be from about 2x for minimal stream decoration through to about 9x for the more complex kind. a_length, b_typeOfCollection, c_typeOfStreamDecoration, d_streamCreation, max op/ms, speedup 0, ArrayList, minimal, old, 20972.400 0, ArrayList, minimal, new, 44866.020, 2.13x 0, ArrayList, basic, old, 19219.077 0, ArrayList, basic, new, 47574.102, 2.47x 0, ArrayList, complex, old, 9425.247 0, ArrayList, complex, new, 44850.655, 4.75x 0, ArrayList, crossover, old, 4749.495 0, ArrayList, crossover, new, 44550.110, 9.37x 0, ConcurrentLinkedQueue, minimal, old, 20146.717 0, ConcurrentLinkedQueue, minimal, new, 36154.586, 1.79x 0, ConcurrentLinkedQueue, basic, old, 18107.648 0, ConcurrentLinkedQueue, basic, new, 36094.319, 1.99x 0, ConcurrentLinkedQueue, complex, old, 9033.936 0, ConcurrentLinkedQueue, complex, new, 36089.520, 3.99x 0, ConcurrentLinkedQueue, crossover, old, 4555.928 0, ConcurrentLinkedQueue, crossover, new, 35932.132, 7.88x 0, ConcurrentSkipListSet, minimal, old, 20391.479 0, ConcurrentSkipListSet, minimal, new, 40259.694, 1.97x 0, ConcurrentSkipListSet, basic, old, 18616.301 0, ConcurrentSkipListSet, basic, new, 40914.132, 2.19x 0, ConcurrentSkipListSet, complex, old, 9853.569 0, ConcurrentSkipListSet, complex, new, 40150.606, 4.07x 0, ConcurrentSkipListSet, crossover, old, 4632.691 0, ConcurrentSkipListSet, crossover, new, 40151.534, 8.66x 0, CopyOnWriteArrayList, minimal, old, 19952.257 0, CopyOnWriteArrayList, minimal, new, 36884.796, 1.84x 0, CopyOnWriteArrayList, basic, old, 18228.390 0, CopyOnWriteArrayList, basic, new, 36993.146, 2.02x 0, CopyOnWriteArrayList, complex, old, 9635.404 0, CopyOnWriteArrayList, complex, new, 36862.714, 3.82x 0, CopyOnWriteArrayList,
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote: > This is a draft proposal for how we could improve stream performance for the > case where the streams are empty. Empty collections are common-place. If we > iterate over them with an Iterator, we would have to create one small > Iterator object (which could often be eliminated) and if it is empty we are > done. However, with Streams we first have to build up the entire pipeline, > until we realize that there is no work to do. With this example, we change > Collection#stream() to first check if the collection is empty, and if it is, > we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream > and EmptyDoubleStream. We have taken great care for these to have the same > characteristics and behaviour as the streams returned by Stream.empty(), > IntStream.empty(), etc. > > Some of the JDK tests fail with this, due to ClassCastExceptions (our > EmptyStream is not an AbstractPipeline) and AssertionError, since we can call > some methods repeatedly on the stream without it failing. On the plus side, > creating a complex stream on an empty stream gives us upwards of 50x increase > in performance due to a much smaller object allocation rate. This PR includes > the code for the change, unit tests and also a JMH benchmark to demonstrate > the improvement. Looks a good idea to reduce the pipeline overhead ! - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Sat, 6 Nov 2021 17:23:34 GMT, Pavel Rappo wrote: > Streams are closeable, and a terminal operation may be invoked on a given > stream only once. Thus, shouldn't the third line in both of the examples > below throw `IllegalStateException`? > > ``` > Stream empty = Stream.empty(); > System.out.println(empty.count()); > System.out.println(empty.count()); > > Stream empty = Stream.empty(); > empty.close(); > System.out.println(empty.count()); > ``` That would be fairly easy to solve by having two instances of the EmptyStream. The terminal operations would return the terminal operation that throws IllegalStateExceptions. - PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: JDK-8277095 : Empty streams create too many objects
On Fri, 5 Nov 2021 12:53:46 GMT, kabutz wrote:
> This is a draft proposal for how we could improve stream performance for the
> case where the streams are empty. Empty collections are common-place. If we
> iterate over them with an Iterator, we would have to create one small
> Iterator object (which could often be eliminated) and if it is empty we are
> done. However, with Streams we first have to build up the entire pipeline,
> until we realize that there is no work to do. With this example, we change
> Collection#stream() to first check if the collection is empty, and if it is,
> we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream
> and EmptyDoubleStream. We have taken great care for these to have the same
> characteristics and behaviour as the streams returned by Stream.empty(),
> IntStream.empty(), etc.
>
> Some of the JDK tests fail with this, due to ClassCastExceptions (our
> EmptyStream is not an AbstractPipeline) and AssertionError, since we can call
> some methods repeatedly on the stream without it failing. On the plus side,
> creating a complex stream on an empty stream gives us upwards of 50x increase
> in performance due to a much smaller object allocation rate. This PR includes
> the code for the change, unit tests and also a JMH benchmark to demonstrate
> the improvement.
Streams are closeable, and a terminal operation may be invoked on a given
stream only once. Thus, shouldn't the third line in both of the examples below
throw `IllegalStateException`?
Stream empty = Stream.empty();
System.out.println(empty.count());
System.out.println(empty.count());
Stream empty = Stream.empty();
empty.close();
System.out.println(empty.count());
I don't think that we can remove all the state from an empty stream, but we can
likely make it smaller.
src/java.base/share/classes/java/util/Collection.java line 743:
> 741: */
> 742: default Stream stream() {
> 743: if (isEmpty()) return Stream.empty();
The net effect of this change might depend on your workload. If you call
stream() on empty collections that have cheap isEmpty(), this change will
likely improve performance and reduce waste. However, this same change might do
the opposite if some of your collections aren't empty or have costly isEmpty().
It would be good to have benchmarks for different workloads.
-
PR: https://git.openjdk.java.net/jdk/pull/6275
RFR: JDK-8277095 : Empty streams create too many objects
This is a draft proposal for how we could improve stream performance for the case where the streams are empty. Empty collections are common-place. If we iterate over them with an Iterator, we would have to create one small Iterator object (which could often be eliminated) and if it is empty we are done. However, with Streams we first have to build up the entire pipeline, until we realize that there is no work to do. With this example, we change Collection#stream() to first check if the collection is empty, and if it is, we simply return an EmptyStream. We also have EmptyIntStream, EmptyLongStream and EmptyDoubleStream. We have taken great care for these to have the same characteristics and behaviour as the streams returned by Stream.empty(), IntStream.empty(), etc. Some of the JDK tests fail with this, due to ClassCastExceptions (our EmptyStream is not an AbstractPipeline) and AssertionError, since we can call some methods repeatedly on the stream without it failing. On the plus side, creating a complex stream on an empty stream gives us upwards of 50x increase in performance due to a much smaller object allocation rate. This PR includes the code for the change, unit tests and also a JMH benchmark to demonstrate the improvement. - Commit messages: - Fixed name typo - Update benchmark to have a variable mix of empty streams - New microbenchmark to test mixed length streams within a single run - Updated empty streams to contain Spliterator state and to prevent stream reuse - Grouped test results through the use of better field naming - Removed old JMH tests that were replaced with more thorough test - Increased warmup time to get more consistent results - Additional benchmarks to check different types of collections, lengths and workloads - Added test classes and minor improvements - Faster empty streams to reduce object allocation rates Changes: https://git.openjdk.java.net/jdk/pull/6275/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=6275=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8277095 Stats: 4269 lines in 15 files changed: 4259 ins; 0 del; 10 mod Patch: https://git.openjdk.java.net/jdk/pull/6275.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/6275/head:pull/6275 PR: https://git.openjdk.java.net/jdk/pull/6275
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 15/11/2021 8:14 pm, Alan Bateman wrote: On 15/11/2021 09:48, David Holmes wrote: I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. That's the outcome. To get there, they call a private method getDeclaredFields0 on j.l.Class and also change the value of the private "modifiers" field in jlr.Field. It's just not tenable to hack into private members like this. Martin seems to have done the right thing and removed it. Apologies - the misunderstanding was mine. David -Alan.
Integrated: 8277048: Tiny improvements to the specification text for java.util.Properties.load
On Fri, 12 Nov 2021 12:25:20 GMT, Pavel Rappo wrote: > Please review this simple two-hunk fix to the documentation comment of > java.util.Properties#load(java.io.Reader). The first hunk (line/lines) is a > suggestion. I believe it reads better since the plurality is more idiomatic; > native English speakers should correct me if I'm wrong. The second hunk picks > up what was overlooked in JDK-8274075 > (https://git.openjdk.java.net/jdk/pull/5610). This pull request has now been integrated. Changeset: fdcd16a3 Author:Pavel Rappo URL: https://git.openjdk.java.net/jdk/commit/fdcd16a38fb9a14a819d68682f9666ebfe7285db Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 mod 8277048: Tiny improvements to the specification text for java.util.Properties.load Reviewed-by: rriggs, iris, naoto - PR: https://git.openjdk.java.net/jdk/pull/6367
Re: RFR: 8277059: Use blessed modifier order in java.xml
On Fri, 12 Nov 2021 15:06:35 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source code in java.xml and > java.xml.crypto. This scripts verifies that modifiers are in the "blessed" > order, and fixes it otherwise. I have manually checked the changes made by > the script to make sure they are sound. I understand. I just checked the git history and saw that there were other code hygiene changes. I'll close this issue then. - PR: https://git.openjdk.java.net/jdk/pull/6369
Withdrawn: 8277059: Use blessed modifier order in java.xml
On Fri, 12 Nov 2021 15:06:35 GMT, Magnus Ihse Bursie wrote: > I ran bin/blessed-modifier-order.sh on source code in java.xml and > java.xml.crypto. This scripts verifies that modifiers are in the "blessed" > order, and fixes it otherwise. I have manually checked the changes made by > the script to make sure they are sound. This pull request has been closed without being integrated. - PR: https://git.openjdk.java.net/jdk/pull/6369
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
On 15/11/2021 09:48, David Holmes wrote: I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. That's the outcome. To get there, they call a private method getDeclaredFields0 on j.l.Class and also change the value of the private "modifiers" field in jlr.Field. It's just not tenable to hack into private members like this. Martin seems to have done the right thing and removed it. -Alan.
Re: RFR: 8271820: Implementation of JEP 416: Reimplement Core Reflection with Method Handle [v16]
Hi Alan, On 15/11/2021 5:11 pm, Alan Bateman wrote: On 14/11/2021 22:56, Claes Redestad wrote: : Alan: changing `Field.modifiers` still works, but dropping the final modifier is not enough for this to work in the new impl. It won't be hard to adapt to the new world. Users who relies on this today could for example opt-out of the new MH-based impl using `-Djdk.reflect.useDirectMethodHandle=false` and get the old behavior. I've checked using a minimal reproducer I extracted from the Wicket sources that this works. Sure, but I don't think that would be enough as Wicket would also need to open java.lang and java.lang.reflect to allow it continue to access private members of Class and Field. I think there may be a misunderstanding here, AFAICS they are using reflection to remove the final-ness of a field in their own classes, not modifying anything in Class or Field. Cheers, David - I assume the test started emitting "Illegal reflective access ..." warnings in JDK 9 and it stopped working in JDK 16, and somewhere along the line the maintainers must have added --add-opens to get it to work. It's just not tenable, hopefully the project will find a way to re-write that test. -Alan
Re: RFR (CSR): 8202555: Double.toString(double) sometimes produces incorrect results
Hello, the 2nd version of the article [1] accompanying this issue [2] and issue [3] has been kindly and thoroughly reviewed by the following renowned world-class researchers: * Guy Steele Jr [4], Oracle Labs * Paul Zimmermann [5], INRIA * Jean-Michel Muller [6], Ecole Normale Supérieure de Lyon Thanks to the improvements contributed by these fine reviewers, the newest 4th version [7] is now in "preview" form, the intent being to publish it on a more permanent platform (academic journal or similar). In addition, they also suggested and provided new hard tests which are included in the PR [8]. I hope this will help in progressing both the CSR and the PR. Greetings Raffaello [1] https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN [2] https://bugs.openjdk.java.net/browse/JDK-8202555 [3] https://bugs.openjdk.java.net/browse/JDK-4511638 [4] https://labs.oracle.com/pls/apex/f?p=labs%3Abio%3A0%3A120 [5] https://members.loria.fr/PZimmermann/ [6] https://perso.ens-lyon.fr/jean-michel.muller/ [7] https://drive.google.com/file/d/1IEeATSVnEE6TkrHlCYNY2GjaraBjOT4f [8] https://github.com/openjdk/jdk/pull/3402 On 2021-10-26 21:28, Raffaello Giulietti wrote: Hello, PR [1] and the accompanying article [2] have been subject to some positive reactions in the last couple of weeks. A fresh set of about 20 thousand additional hard test cases kindly provided by Paul Zimmermann of INRIA and other tests proposed by Guy Steele have been added to the code. The corresponding CSR [3] is in Finalized state for review. The proposed spec has been carefully written to uniquely define the outcomes and the code in the PR has been extensively tested to match the proposed spec. Behavioral backward compatibility has been a major goal for the CSR. In fact, in the vast majority of cases, the CSR and the current implementation agree on the outcomes. As the CSR is a prerequisite for the advancement of the PR, I beg everybody entitled (and interested) to review and approve it and/or discuss it further. Greetings Raffaello [1] https://github.com/openjdk/jdk/pull/3402 [2] https://drive.google.com/file/d/1luHhyQF9zKlM8yJ1nebU0OgVYhfC6CBN/view [3] https://bugs.openjdk.java.net/browse/JDK-8202555
