June 2018 - core-libs-dev

Re: RFR(S): 8205194: Improve the Array Comparison when there is a mismatch at first element.
by Roger Riggs 19 Jun '18

19 Jun '18

Hi Vivek, Do you need to concerned that the arrays are zero length? Are there any tests that test the zero length case. (I assume you ran the existing tests). Thanks, Roger On 6/18/2018 7:52 PM, Deshpande, Vivek R wrote: > Hi All > > Forgot to add the links: > https://bugs.openjdk.java.net/browse/JDK-8205194 > webrev: > http://cr.openjdk.java.net/~vdeshpande/vectorizedMismatch_jdk/webrev.00/ > > Regards. > Vivek > > From: Deshpande, Vivek R > Sent: Monday, June 18, 2018 4:50 PM > To: 'jdk-dev(a)openjdk.java.net' <jdk-dev(a)openjdk.java.net> > Cc: 'Paul Sandoz' <paul.sandoz(a)oracle.com>; Viswanathan, Sandhya <sandhya.viswanathan(a)intel.com> > Subject: RFR(S): 8205194: Improve the Array Comparison when there is a mismatch at first element. > > Hi All > > I would like to contribute a patch which improves the array comparison when there is a mismatch for the first element. > This avoids call to vectorizedMismatch method and gives ~80x speed up. > Please review and sponsor the patch. > > Regards, > Vivek >

1 0

RFR 8199435 : Unsafe publication of java.util.Properties.map
by Brent Christian 19 Jun '18

19 Jun '18

Hi, In JDK 9, 8029891[1] refactored java.util.Properties to store its values in an internal ConcurrentHashMap, and removed synchronization from "reader" methods in order to avoid potential hangs/deadlocks during classloading. Claes has noticed that there is the possibility of the new 'map' field being observed with its default value (null), before being set. After looking at the JSR 133 FAQ[2], I agree with Claes that we should make 'map' a field final. Please review my change to do this: Webrev: http://cr.openjdk.java.net/~bchristi/8199435/webrev/ Issue: https://bugs.openjdk.java.net/browse/JDK-8199435 Thanks, -Brent 1. https://bugs.openjdk.java.net/browse/JDK-8029891 2. https://www.cs.umd.edu/~pugh/java/memoryModel/jsr-133-faq.html#finalRight

7 22

RFR 8198669: Refactor annotation array value parsing to reduce duplication
by Liam Miller-Cushon 19 Jun '18

19 Jun '18

Hello, This change is a follow-up to JDK-7183985 which replaces the interior of parseClassArray, parseEnumArray, and parseAnnotationArray with a shared method that is passed a lambda for the type-specific parsing logic. bug: https://bugs.openjdk.java.net/browse/JDK-8198669 webrev: http://cr.openjdk.java.net/~cushon/8198669/webrev.00/ Liam

3 4

RFR(s): 8060192: Add default method Collection.toArray(generator)
by Stuart Marks 19 Jun '18

19 Jun '18

Hi all, I wanted to restart review of the changeset for bug JDK-8060192 [1]. The latest webrev is here: [2]. The previous review was from December 2017: [3]. The only difference between the current changeset and the previous one is the removal of the overriding implementations (see explanation below). Much of the discussion in the previous review thread was around performance and the shape of the API. # Performance I previously had done some benchmarking on this and reported the results: [4]. (I've recently re-done the benchmarks and conclusions are essentially the same.) The upshot is that implementations that use Arrays.copyOf() are the fastest, probably because it's an intrinsic. It can avoid zero-filling the freshly allocated array because it knows the entire array contents will be overwritten. This is true regardless of what the public API looks like. The default implementation calls generator.apply(0) to get a zero-length array and then simply calls toArray(T[]). This goes through the Arrays.copyOf() fast path, so it's essentially the same speed as toArray(new T[0]). The overrides I had previously provided in specific implementation classes like ArrayList actually are slower, because the allocation of the array is done separately from filling it. This necessitates the extra zero-filling step. Thus, I've removed the overrides. # API Shape There was some discussion about whether it would be preferable to have a Class parameter as a type token for the array component type or the array type itself, instead of using an IntFunction generator. Most of this boils down to what people are comfortable with. However, there are a few points that make the generator function approach preferable. One point in favor of using a generator is that Stream already has a similar toArray(generator) method. Comparing this to the type token alternatives, for simple tasks like converting Collection<String> to String[], things are about equal: toArray(String[]::new) toArray(String.class) toArray(String[].class) However, things are hairier if the element type of the collection is generic, such as Collection<List<String>>. The result type is a generic array List<String>[]. Using class literals or array constructor references will necessitate using an unchecked cast, because none of these can be used to represent a generic type. However, it's possible to use a method reference to provide a properly generic IntFunction. This would enable the toArray(generator) method to be used without any unchecked casts. (The method it refers to might need have an unchecked cast within it, though.) Such a method could also be reused for the corresponding Stream.toArray(generator) method. For these reasons I'd like to proceed with adding toArray(generator) API. Thanks, s'marks [1] https://bugs.openjdk.java.net/browse/JDK-8060192 [2] http://cr.openjdk.java.net/~smarks/reviews/8060192/webrev.3/ [3] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050325.h… [4] http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050585.h…

5 9

RFR: JDK-8205090 Update of the Reader:nullReader() spec for the mark() method
by Patrick Reinhart 18 Jun '18

18 Jun '18

Hi Everybody, In the process of solving the issue [1] a part of the proper solution should be a partially change of the Reader.nullReader() specification. I opened a new CSR to address this. The main change would be to change to following lines: |* <p> The {@code markSupported()} method returns {@code false}. The * {@code mark()} method does nothing, and the {@code reset()} method * throws {@code IOException}.| to this: |* <p> The {@code markSupported()} method returns {@code false}. The * {@code mark()} and {@code reset()} methods throw an {@code IOException}.| Side note: I got a additional comment from Krushnareddy Ganapureddy stating: > additional Note - spec says "ready()), skip(long), and transferTo() methods all behave as if end of stream has been reached" > But there is not clear say on these methods [ ready(), skip(), transferTo() ] what is the expected behavior "if end of stream reached" We may able to address the issue above too. Any ideas how to address this? -Patrick [1] https://bugs.openjdk.java.net/browse/JDK-8204930

1 0

[11] RFR: 8042131: DateTimeFormatterBuilder Mapped-values do not work for JapaneseDate
by Naoto Sato 18 Jun '18

18 Jun '18

Hello, Please review the fix to the following issue: https://bugs.openjdk.java.net/browse/JDK-8042131 The proposed change is located at: http://cr.openjdk.java.net/~naoto/8042131/webrev.00/ The fix is originally contributedy by Toshio Nakamura at IBM [1]. I am sponsoring the fix, with some trivial modifications to the test (diff is attached). Naoto [1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-June/053830.html

3 2

Re: RFR: 8196988 (Resolve disabled warnings for libjimage)
by Srinivas Dama 18 Jun '18

18 Jun '18

Thank you Paul for the review. >I am guessing the comment selectively disables the warning in this code (rather than using a diagnostic pragma). yes I have updated the patch on latest repo changes. http://cr.openjdk.java.net/~sdama/8196988/webrev.02/ pushing the above changeset. Note: I have run mach5 tests successfully. Regards, Srinivas ----- Original Message ----- From: paul.sandoz(a)oracle.com To: srinivas.dama(a)oracle.com Cc: core-libs-dev(a)openjdk.java.net, james.laskey(a)oracle.com Sent: Tuesday, 12 June, 2018 10:08:53 PM GMT +05:30 Chennai, Kolkata, Mumbai, New Delhi Subject: Re: RFR: 8196988 (Resolve disabled warnings for libjimage) Looks ok. Including Jim in case he is not listening in… I am guessing the comment selectively disables the warning in this code (rather than using a diagnostic pragma). Thanks, Paul. > On Jun 7, 2018, at 11:17 PM, Srinivas Dama <srinivas.dama(a)oracle.com> wrote: > > Hi, > > Please review http://cr.openjdk.java.net/~sdama/8196988/webrev.01/ > for https://bugs.openjdk.java.net/browse/JDK-8196988 > > > Note: Modified earlier fix to handle warning messages specific to > libjimage only. > > Regards, > Srinivas > > ----- Original Message ----- > From: srinivas.dama(a)oracle.com > To: core-libs-dev(a)openjdk.java.net > Sent: Monday, 4 June, 2018 11:08:08 PM GMT +05:30 Chennai, Kolkata, Mumbai, New Delhi > Subject: RFR: 8196988 (Resolve disabled warnings for implicit-fallthrough gcc option) > > Hi, > > Please review http://cr.openjdk.java.net/~sdama/8196988/webrev.00/ > for https://bugs.openjdk.java.net/browse/JDK-8196988 > > Note: gcc 7.3 compiler emits warnings for implicit fall-through switch cases,but > these warnings are disabled earlier.I have enabled these warnings and fixed in sources. > > Regards, > Srinivas

1 0

Durations in existing JDK APIs
by Kurt Alfred Kluever 16 Jun '18

16 Jun '18

*Hi core-libs-dev and concurrency-interest,Recently, we've been closely studying date/time code inside of Google, and found a surprising number of places with time unit mismatches due to overuse of primitives to represent date/time concepts. And unlike "off-by-one" errors, these are often "off-by-1000x" or worse...yikes! For internal APIs, we're strongly encouraging folks to use the appropriate java.time types (i.e., Duration or Instant) because this greatly reduces the occurrence of these problems.There are several improvements in the JDK we'd like to propose: 1. Rename ALL existing unitless primitive method parameters to include their time unit. At Google, we have static analysis tools to detect unit mismatches that work based on method names, variable names, etc. However, when methods and method parameters are unitless, the only way to know what correct units are is to read and understand the javadocs (which our tools obviously can't do). Many IDEs also show method signatures with parameter names only. There are a handful of examples in the JDK that we'd like to update. For example, Object.wait(long timeout) would become Object.wait(long timeoutMillis) (or similar). An incomplete list of these APIs is included below [1].2. Add a java.time overload to some APIs that currently represent date/time concepts using primitives. While static analysis helps find errors, ideally people wouldn't have to decompose their Duration instances to call these APIs. Adding a Duration overload of each of these APIs would remove the need to decompose durations, and would encourage developers to plumb durations through more layers of their application. The old primitive-accepting APIs would be softly discouraged. Note that new default implementations will have to delegate to the existing overloads, and will have to choose between losing precision or capping large values at e.g. 292 years (for long nanos), but it is hard to imagine this being a serious problem in practice for any of them.1. Note: it's probably not worth adding Duration overloads to legacy APIs (e.g., java.util.Timer) or low-level APIs (e.g., java.lang.Object.wait()). Determining which APIs make the cut is certainly open to discussion.3. Add a java.time overload to most APIs that currently accept a <long, TimeUnit> pair. Prior to Java8, the recommended advice for accepting a logical duration was to use a <long, TimeUnit> pair, as most <https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/locks/LockSu…> of java.util.concurrent currently does <https://docs.oracle.com/javase/9/docs/api/java/util/concurrent/class-use/Ti…>. Similarly, we've seen unit mismatch bugs with these APIs as well (e.g., future.get(timeout.toNanos(), MILLISECONDS)). Adding a Duration overload for each of these APIs would remove the need to decompose durations, and would encourage broader Duration adoption. The old APIs would be softly discouraged.1. Note: a few of the <long, TimeUnit> APIs are already overloaded, which would make this a 2x2 explosion of methods. Whether that sort of API explosion is acceptable is certainly open for discussion.2. Note: We've already started adding these overloads <https://github.com/google/guava/issues/2999> in Guava <https://github.com/google/guava/> and have plans to do so across the board. Caffeine <https://github.com/ben-manes/caffeine/> has also added <https://github.com/ben-manes/caffeine/issues/221> Duration overloads.4. Add APIs to convert between TimeUnit and Duration. As users transition to the new Duration-centric world, these APIs will come in handy. They will also be necessary for overload-implementers. We're proposing Duration.of(long, TimeUnit) and TimeUnit.convert(Duration).These recommendations, of course, should also apply to new APIs added in the future. Note that we are not expressing an opinion at this time on whether new APIs should or shouldn't also have a <long, TimeUnit> overload.Of course, even comprehensive adoption of Duration in a codebase will not eliminate every possible source of unit mismatch bugs. But it would confine them to only the places where Durations are created. This would reduce the risk in itself, but also makes it much easier for static analyses to detect any remaining bugs. It also lowers the cognitive burden faced by every developer interacting with logical durations.We realize that these are significant changes, so we'd love to hear your thoughts. We'd also be happy to work with Martin Buchholz to make these changes.Thanks,-Kurt Alfred Kluever(on behalf of the Java Core Libraries Team @ Google)Appendix[1] Unitless primitive parameters - Probably worth adding a Duration overload- java.lang.Thread.sleep(long millis)- java.lang.Thread.sleep(long millis, long nanos)- Note: this API is particularly weird since it accepts millis and nanos!- java.lang.Thread.join(long millis)- java.lang.Thread.join(long millis, long nanos)- Note: this API is particularly weird since it accepts millis and nanos!- java.nio.channel.Selector.select(long timeout)- Probably too low level to worry about adding a java.time overloads- java.lang.Object.wait(long timeout)- java.lang.Object.wait(long timeout, long nanos)- Note: this API is particularly weird since it accepts millis and nanos!- java.lang.ReferenceQueue.remove(long timeout)- java.util.concurrent.locks.LockSupport.parkUntil(long deadline)- Note: this parameter represents milliseconds since epoch (an Instant)- java.util.concurrent.locks.LockSupport.parkUntil(Object blocker, long deadline)- Note: this parameter represents milliseconds since epoch (an Instant)- java.util.concurrent.locks.LockSupport.parkNanos(long nanos)- java.util.concurrent.locks.LockSupport.parkNanos(Object blocker, long nanos)- java.util.concurrent.locks.AbstractQueuedSynchronizer.ConditionObject.awaitUntil(Date)- Note: this would be overloaded with an Instant- java.util.logging.LogRecord.setMillis(long millis)- "Legacy" APIs that are probably not worth adding a java.time overloads- java.util.Timer.schedule(TimerTask task, Date firstTime, long period)- java.util.Timer.schedule(TimerTask task, long delay)- java.util.Timer.schedule(TimerTask task, Date time)- java.util.Timer.schedule(TimerTask task, long delay, long period)- java.util.Timer.scheduleAtFixedRate(TimerTask task, Date firstTime, long period)- java.util.Timer.scheduleAtFixedRate(TimerTask task, long delay, long period)- java.sql.Connection.isValid(int timeout)- Note: this parameter is in seconds- java.sql.DriverManager.setLoginTimeout(int seconds)- Note: this parameter is in seconds- Networking APIs (perhaps not worth adding a Duration overload?)- java.net.InetAddress.isReachable(int timeout)- java.net.InetAddress.isReachable(NetworkInterface netif, int ttl, int timeout)- java.net.URLConnection.setConnectTimeout(int timeout)- java.net.URLConnection.setReadTimeout(int timeout)- java.net.Socket.setSoTimeout(int timeout)- java.net.Socket.connect(SocketAddress endpoint, int timeout)- java.net.ServerSocket.setSoTimeout(int timeout)- java.net.DatagramSocket.setSoTimeout(int timeout)*

6 18

RFR 8170159 Improve the performance of BitSet traversal
by Paul Sandoz 15 Jun '18

15 Jun '18

Hi, Please review this enhancement to improve the performance of BitSet traversal when using a stream. http://cr.openjdk.java.net/~psandoz/jdk/JDK-8170159-bitset-traverse/webrev/ <http://cr.openjdk.java.net/~psandoz/jdk/JDK-8170159-bitset-traverse/webrev/> The associated issue started out life referring to the BitSet’s spliterator reporting SIZED, and to report that the cardinality has to be computed. This has some cost but performance analysis (see issue for attached benchmark) has shown that the cost is small compared to the cost of traversal. It is recognized that the cost is higher for smaller BitSets but not unduly so. Thus it was concluded that it was reasonable for the spliterator to report SIZED. The issue was adjusted to focus on improving the performance of the BitSet’s spliterator forEachRemaining method. For large randomized BitSets a 1.6x speedup can be observed, and for smaller sizes a more modest speed up. The prior conclusion about reporting SIZED still holds. Thanks, Paul.

2 7

RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present
by Liam Miller-Cushon 15 Jun '18

15 Jun '18

Hello, Please consider this fix for JDK-7183985. bug: https://bugs.openjdk.java.net/browse/JDK-7183985 webrev: http://cr.openjdk.java.net/~cushon/7183985/webrev.01/ I started a CSR for the change: https://bugs.openjdk.java.net/browse/JDK-8198584 We have been using the fix at Google for about two years, and there has been no compatibility impact. I found very few places ArrayStoreException was being explicitly handled, and none that were depending on the current behaviour of getAnnotation(). There was some previous discussion of the bug on core-libs-dev: * http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-April/040590.html * http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-October/021798.ht… Thanks, Liam

6 13