February 2016 - core-libs-dev

RFR: JDK-8133206 - Higher native memory usage caused by upgrade to zlib 1.2.8
by Alex Kashchenko 14 Mar '16

14 Mar '16

Please review the following fix to JDK-8133206: - bug: https://bugs.openjdk.java.net/browse/JDK-8133206 - webrev for jdk9: http://cr.openjdk.java.net/~akasko/jdk9/8133206/webrev.00/ This fix is a forward-port of the patch to jdk7u created by Nikolay Gorshkov: - webrev for jdk7u: http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/ - jdk7u review thread with the issue details: http://mail.openjdk.java.net/pipermail/jdk7u-dev/2015-October/010420.html - the same thread on core-libs-dev: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-October/035884.ht… Forward-port to jdk9 is almost the same as original patch (with changed paths), one additional inflate(...) call is changed in zip_util.c . jdk9 with this patch passes all the regression tests from java/util/jar, java/util/zip, tools/jar and tools/pack200. I created a jtreg reproducer that works with jdk7u and jdk9: - web link: http://cr.openjdk.java.net/~akasko/jdk9/8133206/reproducer/ - download: http://cr.openjdk.java.net/~akasko/jdk9/8133206/reproducer.zip This reproducer is Linux-only, slow (takes more than a minute) and requires Valgrind and zlib-debuginfo (if system zlib is used). It runs java.util.zip.Inflater under the Memcheck multiple times and parses the output to compare the number of zlib-specific memory allocations. I am not sure whether such reproducer should be included into the jdk regression tests tree, so publishing it as a standalone jtreg test (not a webrev). Please let me know if I need to create a new bug for jdk9 (in addition to JDK-8133206). -- -Alex

2 1

Re: JDK 9 RFR of JDK-8149896: Remove unnecessary values in FloatConsts and DoubleConsts
by Mike Duigou 13 Mar '16

13 Mar '16

> From: joe darcy <joe.darcy(a)oracle.com> > Hello, > > The the FloatConsts and DoubleConsts classes, while moved to an > internal > package recently (JDK-8145990), contain constants now available via the > public API. All such uses of the redundant values should be removed as > well as the redundant constants themselves. > > http://cr.openjdk.java.net/~darcy/8149896.0/ The changes look good to me and it is very tempting to suggest just retiring DoubleConstants and FloatConstants altogether and move the remaining quite useful constants in to Double and Float. Mike

2 1

RFR: 8072727 - add variation of Stream.iterate() that's finite
by Tagir F. Valeev 11 Mar '16

11 Mar '16

Hello! I wanted to work on foldLeft, but Brian asked me to take this issue instead. So here's webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8072727/r1/ I don't like iterator-based Stream source implementations, so I made them AbstractSpliterator-based. I also implemented manually forEachRemaining as, I believe, this improves the performance in non-short-circuiting cases. I also decided to keep two flags (started and finished) to track the state. Currently existing implementation of infinite iterate() does not use started flag, but instead reads one element ahead for primitive streams. This seems wrong to me and may even lead to unexpected exceptions (*). I could get rid of "started" flag for Stream.iterate() using Streams.NONE, but this would make object implementation different from primitive implementations. It would also be possible to keep single three-state variable (byte or int, NOT_STARTED, STARTED, FINISHED), but I doubt that this would improve the performance or footprint. Having two flags looks more readable to me. Currently existing two-arg iterate methods can now be expressed as a partial case of the new method: public static<T> Stream<T> iterate(final T seed, final UnaryOperator<T> f) { return iterate(seed, x -> true, f); } (same for primitive streams). I may do this if you think it's reasonable. I created new test class and added new iterate sources to existing data providers. Please review and sponsor! With best regards, Tagir Valeev. (*) Consider the following code: int[] data = {1,2,3,4,-1}; IntStream.iterate(0, x -> data[x]) .takeWhile(x -> x >= 0) .forEach(System.out::println); Currently this unexpectedly throws an AIOOBE, because IntStream.iterate unnecessarily tries to read one element ahead.

7 21

RFR(XS): 8147844: new method j.l.Runtime.onSpinWait()
by Ivan Krylov 10 Mar '16

10 Mar '16

Hello, Earlier there was a discussion on this mail alias about the spin loop hint proposal [1]. Based on the feedback from that discussion some changes were incorporated and the JEP has been filed [2]. There seems to be a consensus on the API side. The JEP is now in a draft state and I hope this JEP will get targeted for java 9 shortly. Please review the upcoming API changes: http://cr.openjdk.java.net/~ikrylov/8147844.jdk.00/ For the reference, the new generated JavaDoc for j.l.Runtime class and the new method http://ivankrylov.github.io/onspinwait/api/java/lang/Runtime.html#onSpinWai… Thanks, Ivan 1 - http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-October/thread.ht… 2 - https://bugs.openjdk.java.net/browse/JDK-8147832.

8 19

Re: RFR [7] 8133206: "java.lang.OutOfMemoryError: unable to create new native thread" caused by upgrade to zlib 1.2.8
by Dmitry Cherepanov 09 Mar '16

09 Mar '16

On Feb 19, 2016, at 1:55 PM, Nikolay Gorshkov <nikolay(a)azulsystems.com<mailto:nikolay@azulsystems.com>> wrote: Hi Sherman, Sean, Could you please help with making progress on this code review request? This fix is waiting for review since October. Webrev for jdk7u: http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/ Original mail thread: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-October/035884.ht… I’m not expert in this area but the changes look reasonable to me. +1 for pushing this into JDK9. Thanks Dmitry Webrev for jdk9 (contributed by Alex Kashchenko): http://cr.openjdk.java.net/~akasko/jdk9/8133206/webrev.00/ Original mail thread: http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html Thanks, Nikolay On 11.12.2015 15:09, Nikolay Gorshkov wrote: Hi Sean, Thank you for your attention to this matter! Actually, the code review request was sent to core-libs-dev alias a month ago: http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-November/036463.h… Unfortunately, we haven't got any feedback yet. Thanks, Nikolay On 11.12.2015 14:34, Seán Coffey wrote: Hi Alex, I'm dropping jdk7u-dev mailing list for the moment. core-libs-dev is the mailing list where this review should happen. This fix would be required in JDK 9 first as per process. I think Sherman would be best to review if possible. Once it's soaked in JDK 9 for a few weeks, we could consider jdk8u and 7u backports. Regards, Sean. On 10/11/15 10:57, Alex Kashchenko wrote: Hi, On 10/21/2015 04:39 PM, Nikolay Gorshkov wrote: Hi Sherman, Thank you for your reply! My answers are inlined. > Can you be more specific about the "class loading cases" above? Sounds > more like we have a memory leaking here (the real root cause)? for example > the inflateEnd() never gets called? I agree, the real root cause is probably the following issue that exists since the end of 2002: https://bugs.openjdk.java.net/browse/JDK-4797189 "Finalizers not called promptly enough" And it is "the absence of a general solution to the non-heap resource exhaustion problem". zlib's inflateEnd() function is called by void java.util.zip.Inflater.end(long addr) native method only, and this method, in turn, is called only by void java.util.zip.Inflater.end() and void java.util.zip.Inflater.finalize() methods. According to the experiments, the typical stack trace for instantiating java.util.zip.Inflater is: java.util.zip.Inflater.<init>(Inflater.java:116) java.util.zip.ZipFile.getInflater(ZipFile.java:450) java.util.zip.ZipFile.getInputStream(ZipFile.java:369) java.util.jar.JarFile.getInputStream(JarFile.java:412) org.jboss.virtual.plugins.context.zip.ZipFileWrapper.openStream(ZipFileWrapper.java:222) <more jboss frames> org.jboss.classloader.spi.base.BaseClassLoader$2.run(BaseClassLoader.java:592) java.security.AccessController.doPrivileged(Native Method) org.jboss.classloader.spi.base.BaseClassLoader.loadClassLocally(BaseClassLoader.java:591) <more jboss frames> org.jboss.classloader.spi.base.BaseClassLoader.loadClass(BaseClassLoader.java:447) java.lang.ClassLoader.loadClass(ClassLoader.java:358) java.lang.Class.forName0(Native Method) java.lang.Class.forName(Class.java:278) org.jboss.deployers.plugins.annotations.WeakClassLoaderHolder.loadClass(WeakClassLoaderHolder.java:72) <more jboss and other frames> It's quite hard to understand who is responsible for not calling Inflater.end() method explicitly; probably, it is the jboss/application's code. Unfortunately, we were in "it worked before and is broken now" customer situation here, so needed to fix it anyway. > From the doc/impl in inflate() it appears the proposed change should be > fine, though it's a little hacky, as you never know if it starts to return > Z_OK from some future release(s). Since the "current" implementation > never returns Z_OK, it might be worth considering to keep the Z_OK logic > asis in Inflater.c, together with the Z_BUF_ERROR, just in case? OK, I added handling of Z_OK code back. > I would be desired to add some words in Inflater.c to remind the > future maintainer why we switched from partial to finish and why to > check z_buf_error. I agree, added a comment. The updated webrev is available here: http://cr.openjdk.java.net/~nikgor/8133206/jdk7u-dev/webrev.01/ The change looks good to me (not a Reviewer/Committer). Patched jdk7u also passes JCK-7 on RHEL 7.1. I forward-ported this patch to jdk9 (consulted with Nikolay Gorshkov first), jtreg reproducer for jdk9 also works with jdk7u - http://mail.openjdk.java.net/pipermail/jdk9-dev/2015-November/003036.html

3 2

RFR-8148748: ArrayList.subList().spliterator() is not late-binding
by Tagir F. Valeev 04 Mar '16

04 Mar '16

Hello! Here's the webrev: http://cr.openjdk.java.net/~tvaleev/webrev/8148748/r1/ I noticed that list == null checks are never true, thus redundant, so I removed them. The list field is final and always assigned to the outer ArrayList instance. So it seems quite natural to make ArrayListSpliterator non-static instead. Also I added one more field into ArrayListSpliterator to hold the SubList reference. Hopefully such footprint increase is acceptable. The offset field is initialized eagerly as it does not change during the SubList lifetime. Also I replaced "if (action == null)" checks with "Objects.requireNonNull". Some more thoughts about forEachRemaining: To me it seems that if ((a = lst.elementData) != null) is a redundant check as well as in current ArrayList implementation elementData is never null. So it can be replaced with simple assignment. Also this check: (i = index) >= 0 I don't see the case when i could be negative. Should it be removed as well? Or probably it's added to aid JIT to elimitate bounds-checking? With best regards, Tagir Valeev. PS> Hi Tagir, PS> Last-binding while adding some complexity to the implementation PS> is an important property, as it is to fail-fast on a best effort basis. PS> When one has mutable collections that can be structurally PS> modified and lazy views over those collections (Streams/Iterators) PS> then those views really need to act on the most up to date PS> structure and not a snapshot (structural or otherwise) when the PS> view was created. Otherwise, it is a potential source of bugs PS> producing incorrect results or exceptions, or another form of PS> complexity for the implementation to maintain integrity under such conditions. PS> We took a short-cut here to reuse the existing top-level PS> spliterator in a non-late-binding manner by specifying the PS> absolute fence. We should fix it. PS> Glancing at the code i believe there is a simple fix. When -1 is PS> passed as the fence to the ArrayList.ArrayListSpliterator then the PS> fence is created lazily and should be the result of offset + PS> list.size e.g. in the constructor: PS> hi = fence = offset + lst.size; PS> The forEach impl also needs updating. PS> — PS> Sublists are also tricky beasts. See documentation on List.subList: PS> * The semantics of the list returned by this method become undefined if PS> * the backing list (i.e., this list) is <i>structurally modified</i> in PS> * any way other than via the returned list. (Structural modifications are PS> * those that change the size of this list, or otherwise perturb it in such PS> * a fashion that iterations in progress may yield incorrect results.) PS> Paul. >> On 29 Jan 2016, at 05:32, Tagir F. Valeev <amaembo(a)gmail.com> wrote: >> >> Hello! >> >> When reviewing 8079136 I noticed that >> ArrayList.subList().spliterator() is not late-binding: >> >> List<Integer> list = new ArrayList<>(Arrays.asList(1,2,3,4)).subList(0, 3); >> Stream<Integer> s = list.stream(); >> list.add(5); >> s.findFirst(); >> --> Exception in thread "main" java.util.ConcurrentModificationException >> >> This works correctly for other list implementation (for example, >> replacing ArrayList with LinkedList makes this code working). As >> Collection.spliterator() spec says: >> >>> In order to preserve expected laziness behavior for the stream() and >>> parallelStream() methods, spliterators should either have the >>> characteristic of IMMUTABLE or CONCURRENT, or be late-binding. If >>> none of these is practical, the overriding class should describe the >>> spliterator's documented policy of binding and structural >>> interference, and should override the stream() and parallelStream() >>> methods to create streams using a Supplier of the spliterator >> >> None of these is true for ArrayList.subList().spliterator(): it's not >> IMMUTABLE, not CONCURRENT, not late-binding, the binding policy is not >> documented and stream()/parallelStream() methods are not overridden. >> >> Is this an issue? >> >> Actually to my opinion late-binding spliterator behavior is a mistake. >> Nobody cares about it as nobody modifies the source between stream >> creation and terminal operation call. On the other hand, this >> requirement adds implementation complexity and I already found the >> second late-binding problem in existing code (the first one was with >> Pattern.splitAsStream). Also it's not documented that Stream.concat >> may bind the late-binding source, but nobody cares anyways. Life would >> be easier without late-binding spliterators. >> >> With best regards, >> Tagir Valeev. >> ===8<===========End of original message text=========== -- Best regards, Tagir mailto:amaembo@gmail.com

3 11

RFR:JDK-8032051:"ZonedDateTime" class "parse" method fails with short time zone offset ("+01")
by nadeesh tv 04 Mar '16

04 Mar '16

Hi all, Please review a fix for Bug Id https://bugs.openjdk.java.net/browse/JDK-8032051 webrev http://cr.openjdk.java.net/~ntv/8032051/webrev.01/ -- Thanks and Regards, Nadeesh TV

3 16

RFR: jsr166 jdk9 integration wave 5
by Martin Buchholz 03 Mar '16

03 Mar '16

A very boring jsr166 integration, focused on reliability. This one has the promised "even more unnecessarily robust" ThreadLocalRandom. http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

3 6

Custom security policy without replacing files in the OpenJDK?
by Marcus Lagergren 03 Mar '16

03 Mar '16

Hi! Is it possible to override lib/security/local_policy on app level without patching jdk distro? i.e. -Duse.this.policy.jar= … or something? Can’t find a way to do it Regards Marcus

3 2

RFR(s): 8150460: (linux|bsd|aix)_close.c: file descriptor table may become large or may not work at all
by Thomas Stüfe 03 Mar '16

03 Mar '16

Hi all, please take a look at this proposed fix. The bug: https://bugs.openjdk.java.net/browse/JDK-8150460 The Webrev: http://cr.openjdk.java.net/~stuefe/webrevs/8150460-linux_close-fdTable/webr… Basically, the file descriptor table implemented in linux_close.c may not work for RLIMIT_NO_FILE=infinite or may grow very large (I saw a 50MB table) for high values for RLIMIT_NO_FILE. Please see details in the bug description. The proposed solution is to implement the file descriptor table not as plain array, but as a twodimensional sparse array, which grows on demand. This keeps the memory footprint small and fixes the corner cases described in the bug description. Please note that the implemented solution is kept simple, at the cost of somewhat higher (some kb) memory footprint for low values of RLIMIT_NO_FILE. This can be optimized, if we even think it is worth the trouble. Please also note that the proposed implementation now uses a mutex lock for every call to getFdEntry() - I do not think this matters, as this is all in preparation for an IO system call, which are usually way more expensive than a pthread mutex. But again, this could be optimized. This is an implementation proposal for Linux; the same code found its way to BSD and AIX. Should you approve of this fix, I will modify those files too. Thank you and Kind Regards, Thomas

7 20