JDK 9 RFR of JDK-8148653: Problem list B5086147.java on windows

[joe darcy]

The test

java/net/URL/B5086147.java

has been failing on windows since the changesets for JDK-8147462 and 
JDK-8145104 were pushed. The test should be problem listed until the 
root cause of JDK-8148626 is found and fixed.


Please review the patch to do this below.

Thanks,

-Joe

diff -r 3bce90b8839e test/ProblemList.txt
--- a/test/ProblemList.txtFri Jan 29 17:03:17 2016 -0800
+++ b/test/ProblemList.txtFri Jan 29 21:01:06 2016 -0800
@@ -180,6 +180,9 @@
 # 7143960
 java/net/DatagramSocket/SendDatagramToBadAddress.java macosx-all

+# 8148626
+java/net/URL/B5086147.java windows-all
+
 

 # jdk_nio

Re: RFR 7183985: Class.getAnnotation() throws an ArrayStoreException when the annotation class not present

[Liam Miller-Cushon]

On Thu, Jan 28, 2016 at 11:50 AM, Joel Borggrén-Franck <
joel.borggren.fra...@gmail.com> wrote:

> Thanks for the update. I think it makes sense to expand testing
> slightly, testing all three parsing clauses that you fixed, and for
> all three of them also checking that a "thing" after the broken item
> is parsed correctly.
>

Sure, I'd be happy to add additional tests if it looks like the other
questions can be addressed.


> But, the reason we didn't fix this the last two times we looked at it
> (that I am aware of) is the compatibility story. In order to fix his
> you need to figure out two things:
>
> - Is this is a spec change, that is, can we get away with throwing an
> AnnotationFormatError where we would now do? Should we clarify the
> spec?
>

Can you clarify which parts of the spec might need to be updated? I can't
find anything relevant in the jls or jvms. The javadoc for AnnotatedElement
mentions some conditions under which TypeNotPresentException is thrown:

"If an annotation returned by a method in this interface contains (directly
or indirectly) a Class-valued member referring to a class that is not
accessible in this VM, attempting to read the class by calling the relevant
Class-returning method on the returned annotation will result in a
TypeNotPresentException."

So throwing TypeNotPresentException when an array-valued annotation
indirectly references an inaccessible class sounds like the right
behaviour, and is consistent with how the implementation currently handles
similar cases.

- Since this is a behaviorally incompatible change, how big is the
> impact? This is of course a hard question to answer, but if one could
> do a corpus analysis over a large code base and look for catches of
> ArrayStoreExceptions when reflecting over annotations, that could be
> useful. If it turns out that "a lot" of users have adopted to this
> bug, perhaps it isn't worth fixing? On the other hand I can imagine
> that this is so uncommon that no one catches either type of error.
>

I'm working on evaluating the impact. A review of our code base showed that
handling ArrayStoreException is fairly uncommon. Of the instances I found,
none of them were in code that was inspecting annotations.

The next step is to start using the patch internally and see if anything
breaks. I'll update the thread when I have results on that.

Re: RFR [9]: 8138699 Iproving JAX-B javadoc

[Lance Andersen]

Hi Miran,

Overall, looks ok.  Please change JavaSE to Java SE.

Has a CCC been approved for the javadoc changes as it will need to

Best
Lance
On Jan 29, 2016, at 12:40 PM, Miroslav Kos  wrote:

> Hi everybody,
> please review following patch - those are changes to javadoc, previously 
> consulted with sqe. The javadoc now is not always clear, which makes 
> difficult to develop proper tests.
> 
> JIRA: https://bugs.openjdk.java.net/browse/JDK-8138699
> webrev: http://cr.openjdk.java.net/~mkos/8138699/jaxws.01/
> 
> Thanks
> Miran
> 
> 



Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering 
1 Network Drive 
Burlington, MA 01803
lance.ander...@oracle.com

Re: Please approve fix for failing test JDK-8148638

[Martin Buchholz]

Thanks!
Pushed.

On Fri, Jan 29, 2016 at 1:56 PM, Roger Riggs  wrote:
> +1
>
>
> On 1/29/2016 4:49 PM, Martin Buchholz wrote:
>>
>> Sorry, I made a mistake in 8146467 and checked in a broken test.
>> Please approve my temporary fix
>> https://bugs.openjdk.java.net/browse/JDK-8148638
>>
>> http://cr.openjdk.java.net/~martin/webrevs/openjdk9/CompletableFuture-temporary-revert/
>
>

Re: Please approve fix for failing test JDK-8148638

[Roger Riggs]

+1

On 1/29/2016 4:49 PM, Martin Buchholz wrote:

Sorry, I made a mistake in 8146467 and checked in a broken test.
Please approve my temporary fix
https://bugs.openjdk.java.net/browse/JDK-8148638
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/CompletableFuture-temporary-revert/

Please approve fix for failing test JDK-8148638

[Martin Buchholz]

Sorry, I made a mistake in 8146467 and checked in a broken test.
Please approve my temporary fix
https://bugs.openjdk.java.net/browse/JDK-8148638
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/CompletableFuture-temporary-revert/

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Martin Buchholz]

On Fri, Jan 29, 2016 at 9:37 AM, Stephen Colebourne
 wrote:
> In the ideal fix, all methods that take the combination (long,
> TimeUnit) would be supplemented by an override that takes (Duration).
> eg. Future.get(long, TimeUnit) has an additional get(Duration).
> However, this is a lot of work, might be unpopular and would be slower
> for j.u.concurrent use.

Of course, having (long, TimeUnit) argument pairs is a performance
hack, but there's not much demand for an improved API, and I still
hope that someday we'll have proper value types in Java, at which time
adding support for value type durations will be more attractive.  So
let's do nothing else for now, unless there's a more urgent compelling
reason?

Re: JDK 9 RFR of JDK-8148627: Problem list TestMaxCachedBufferSize.java

[joe darcy]

Hi Alan,

Limiting the test to 64-bit platforms is fine with me; I'll push the 
change as you've suggested later today.


Thanks,

-Joe

On 1/29/2016 11:59 AM, Alan Bateman wrote:



On 29/01/2016 18:53, joe darcy wrote:

Hello,

Until JDK-8148540 is addressed, the test

sun/nio/ch/TestMaxCachedBufferSize.java

should be problem listed on the platforms is fails on. Please review 
the patch below.


I think this test will be tricky to be reliable on 32-bit so the 
simplest is to just restrict it to 64-bit with:


   @requires (sun.arch.data.model == "64")

-Alan.

Re: JDK 9 RFR of JDK-8148627: Problem list TestMaxCachedBufferSize.java

[Alan Bateman]

On 29/01/2016 18:53, joe darcy wrote:

Hello,

Until JDK-8148540 is addressed, the test

sun/nio/ch/TestMaxCachedBufferSize.java

should be problem listed on the platforms is fails on. Please review 
the patch below.


I think this test will be tricky to be reliable on 32-bit so the 
simplest is to just restrict it to 64-bit with:


   @requires (sun.arch.data.model == "64")

-Alan.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

>>> One other thing that I've been wondering about is the stream() and 
>>> entries() methods. Has there been any discussion about these doing filter?
>> 
>> Not that I’m aware of.
>> 
>>> Maybe it is too expensive and best left to the user of the API? Part of the 
>>> context for asking is modular JARs of course.
>> 
>> Perhaps you can elaborate.
> 
> Alan’s point is that traversing using entries()/stream() always returns the 
> versioned entries (if any) rather than all entries, thus in a sense filters.
> 
> My assumption was the traversal should by default be consistent with a calls 
> to getEntry, thus:
> 
> jarFile.stream().forEach(e -> {
>JarEntry je = jarFile.getJarEntry(e.getName());
>assert e.equals(je);
> });
> 
> There might need to be another stream method that returns all entries.

JarFile.entries() and JarFile.stream() return all the entries; they are not 
filtered.  Your example would work if the JarFile is constructed without a 
Release argument or with the Release.BASE argument.  The assertion would fail 
if it’s constructed with any other Release argument.  Adding a filter and a map 
to the stream pipeline would make your example succeed with all values of 
Release.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Roger Riggs]

Hi Martin,

On 1/29/2016 2:28 PM, Martin Buchholz wrote:

On Fri, Jan 29, 2016 at 11:17 AM, Roger Riggs  wrote:

Hi Martin,

Where did IllegalMonitorStateException creep in from?

Oops.  My Emacs keeps suggesting the wrong completion.  Fixed.


Should I update the CCC I started or will you start fresh?

If you could update the CCC, that would be awesome!
The only substantive change was removing a @throws IAE and adding @throws NPE.

ok

Roger

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Martin Buchholz]

On Fri, Jan 29, 2016 at 11:17 AM, Roger Riggs  wrote:
> Hi Martin,
>
> Where did IllegalMonitorStateException creep in from?

Oops.  My Emacs keeps suggesting the wrong completion.  Fixed.

> Should I update the CCC I started or will you start fresh?

If you could update the CCC, that would be awesome!
The only substantive change was removing a @throws IAE and adding @throws NPE.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Roger Riggs]

Hi Martin,

Where did IllegalMonitorStateException creep in from?

Should I update the CCC I started or will you start fresh?

Otherwise, ok.

Roger


On 1/29/2016 2:12 PM, Martin Buchholz wrote:

Here is a proposed alternative patch (against jsr166 CVS).
Code is reworded and reformatted.
Tests are junit-ified.
Round-trip tests are added.
toChronoUnit no longer throws IAE, because it cannot (and we commit to
having ChronoUnit be a superset of TimeUnit, in perpetuity;
toChronoUnit will forever be an injective function).

Index: src/main/java/util/concurrent/TimeUnit.java
===
RCS file: 
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/TimeUnit.java,v
retrieving revision 1.47
diff -u -r1.47 TimeUnit.java
--- src/main/java/util/concurrent/TimeUnit.java 20 Sep 2015 17:03:23 - 1.47
+++ src/main/java/util/concurrent/TimeUnit.java 29 Jan 2016 19:05:01 -
@@ -6,6 +6,9 @@

  package java.util.concurrent;

+import java.time.temporal.ChronoUnit;
+import java.util.Objects;
+
  /**
   * A {@code TimeUnit} represents time durations at a given unit of
   * granularity and provides utility methods to convert across units,
@@ -361,4 +364,48 @@
  }
  }

+/**
+ * Converts this {@code TimeUnit} to the equivalent {@code ChronoUnit}.
+ *
+ * @return the converted equivalent ChronoUnit
+ * @since 9
+ */
+public ChronoUnit toChronoUnit() {
+switch (this) {
+case NANOSECONDS:  return ChronoUnit.NANOS;
+case MICROSECONDS: return ChronoUnit.MICROS;
+case MILLISECONDS: return ChronoUnit.MILLIS;
+case SECONDS:  return ChronoUnit.SECONDS;
+case MINUTES:  return ChronoUnit.MINUTES;
+case HOURS:return ChronoUnit.HOURS;
+case DAYS: return ChronoUnit.DAYS;
+default: throw new AssertionError();
+}
+}
+
+/**
+ * Converts a {@code ChronoUnit} to the equivalent {@code TimeUnit}.
+ *
+ * @param chronoUnit the ChronoUnit to convert
+ * @return the converted equivalent TimeUnit
+ * @throws IllegalArgumentException if {@code chronoUnit} has no
+ * equivalent TimeUnit
+ * @throws NullPointerException if {@code chronoUnit} is null
+ * @since 9
+ */
+public static TimeUnit of(ChronoUnit chronoUnit) {
+switch (Objects.requireNonNull(chronoUnit, "chronoUnit")) {
+case NANOS:   return TimeUnit.NANOSECONDS;
+case MICROS:  return TimeUnit.MICROSECONDS;
+case MILLIS:  return TimeUnit.MILLISECONDS;
+case SECONDS: return TimeUnit.SECONDS;
+case MINUTES: return TimeUnit.MINUTES;
+case HOURS:   return TimeUnit.HOURS;
+case DAYS:return TimeUnit.DAYS;
+default:
+throw new IllegalArgumentException(
+"No TimeUnit equivalent for " + chronoUnit);
+}
+}
+
  }
Index: src/test/tck/TimeUnitTest.java
===
RCS file: /export/home/jsr166/jsr166/jsr166/src/test/tck/TimeUnitTest.java,v
retrieving revision 1.25
diff -u -r1.25 TimeUnitTest.java
--- src/test/tck/TimeUnitTest.java 25 Apr 2015 04:55:31 - 1.25
+++ src/test/tck/TimeUnitTest.java 29 Jan 2016 19:05:02 -
@@ -14,6 +14,7 @@
  import static java.util.concurrent.TimeUnit.NANOSECONDS;
  import static java.util.concurrent.TimeUnit.SECONDS;

+import java.time.temporal.ChronoUnit;
  import java.util.concurrent.CountDownLatch;
  import java.util.concurrent.TimeUnit;

@@ -433,4 +434,49 @@
  assertSame(x, serialClone(x));
  }

+/**
+ * tests for toChronoUnit.
+ */
+public void testToChronoUnit() throws Exception {
+assertSame(ChronoUnit.NANOS,   NANOSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MICROS,  MICROSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MILLIS,  MILLISECONDS.toChronoUnit());
+assertSame(ChronoUnit.SECONDS, SECONDS.toChronoUnit());
+assertSame(ChronoUnit.MINUTES, MINUTES.toChronoUnit());
+assertSame(ChronoUnit.HOURS,   HOURS.toChronoUnit());
+assertSame(ChronoUnit.DAYS,DAYS.toChronoUnit());
+
+// Every TimeUnit has a defined ChronoUnit equivalent
+for (TimeUnit x : TimeUnit.values())
+assertSame(x, TimeUnit.of(x.toChronoUnit()));
+}
+
+/**
+ * tests for TimeUnit.of(ChronoUnit).
+ */
+public void testTimeUnitOf() throws Exception {
+assertSame(NANOSECONDS,  TimeUnit.of(ChronoUnit.NANOS));
+assertSame(MICROSECONDS, TimeUnit.of(ChronoUnit.MICROS));
+assertSame(MILLISECONDS, TimeUnit.of(ChronoUnit.MILLIS));
+assertSame(SECONDS,  TimeUnit.of(ChronoUnit.SECONDS));
+assertSame(MINUTES,  TimeUnit.of(ChronoUnit.MINUTES));
+assertSame(HOURS,TimeUnit.of(ChronoUnit.HOURS));
+assertSame(DAYS, TimeUnit.of(ChronoUnit.DAYS));
+
+assertThrows(

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Martin Buchholz]

Here is a proposed alternative patch (against jsr166 CVS).
Code is reworded and reformatted.
Tests are junit-ified.
Round-trip tests are added.
toChronoUnit no longer throws IAE, because it cannot (and we commit to
having ChronoUnit be a superset of TimeUnit, in perpetuity;
toChronoUnit will forever be an injective function).

Index: src/main/java/util/concurrent/TimeUnit.java
===
RCS file: 
/export/home/jsr166/jsr166/jsr166/src/main/java/util/concurrent/TimeUnit.java,v
retrieving revision 1.47
diff -u -r1.47 TimeUnit.java
--- src/main/java/util/concurrent/TimeUnit.java 20 Sep 2015 17:03:23 - 1.47
+++ src/main/java/util/concurrent/TimeUnit.java 29 Jan 2016 19:05:01 -
@@ -6,6 +6,9 @@

 package java.util.concurrent;

+import java.time.temporal.ChronoUnit;
+import java.util.Objects;
+
 /**
  * A {@code TimeUnit} represents time durations at a given unit of
  * granularity and provides utility methods to convert across units,
@@ -361,4 +364,48 @@
 }
 }

+/**
+ * Converts this {@code TimeUnit} to the equivalent {@code ChronoUnit}.
+ *
+ * @return the converted equivalent ChronoUnit
+ * @since 9
+ */
+public ChronoUnit toChronoUnit() {
+switch (this) {
+case NANOSECONDS:  return ChronoUnit.NANOS;
+case MICROSECONDS: return ChronoUnit.MICROS;
+case MILLISECONDS: return ChronoUnit.MILLIS;
+case SECONDS:  return ChronoUnit.SECONDS;
+case MINUTES:  return ChronoUnit.MINUTES;
+case HOURS:return ChronoUnit.HOURS;
+case DAYS: return ChronoUnit.DAYS;
+default: throw new AssertionError();
+}
+}
+
+/**
+ * Converts a {@code ChronoUnit} to the equivalent {@code TimeUnit}.
+ *
+ * @param chronoUnit the ChronoUnit to convert
+ * @return the converted equivalent TimeUnit
+ * @throws IllegalArgumentException if {@code chronoUnit} has no
+ * equivalent TimeUnit
+ * @throws NullPointerException if {@code chronoUnit} is null
+ * @since 9
+ */
+public static TimeUnit of(ChronoUnit chronoUnit) {
+switch (Objects.requireNonNull(chronoUnit, "chronoUnit")) {
+case NANOS:   return TimeUnit.NANOSECONDS;
+case MICROS:  return TimeUnit.MICROSECONDS;
+case MILLIS:  return TimeUnit.MILLISECONDS;
+case SECONDS: return TimeUnit.SECONDS;
+case MINUTES: return TimeUnit.MINUTES;
+case HOURS:   return TimeUnit.HOURS;
+case DAYS:return TimeUnit.DAYS;
+default:
+throw new IllegalArgumentException(
+"No TimeUnit equivalent for " + chronoUnit);
+}
+}
+
 }
Index: src/test/tck/TimeUnitTest.java
===
RCS file: /export/home/jsr166/jsr166/jsr166/src/test/tck/TimeUnitTest.java,v
retrieving revision 1.25
diff -u -r1.25 TimeUnitTest.java
--- src/test/tck/TimeUnitTest.java 25 Apr 2015 04:55:31 - 1.25
+++ src/test/tck/TimeUnitTest.java 29 Jan 2016 19:05:02 -
@@ -14,6 +14,7 @@
 import static java.util.concurrent.TimeUnit.NANOSECONDS;
 import static java.util.concurrent.TimeUnit.SECONDS;

+import java.time.temporal.ChronoUnit;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;

@@ -433,4 +434,49 @@
 assertSame(x, serialClone(x));
 }

+/**
+ * tests for toChronoUnit.
+ */
+public void testToChronoUnit() throws Exception {
+assertSame(ChronoUnit.NANOS,   NANOSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MICROS,  MICROSECONDS.toChronoUnit());
+assertSame(ChronoUnit.MILLIS,  MILLISECONDS.toChronoUnit());
+assertSame(ChronoUnit.SECONDS, SECONDS.toChronoUnit());
+assertSame(ChronoUnit.MINUTES, MINUTES.toChronoUnit());
+assertSame(ChronoUnit.HOURS,   HOURS.toChronoUnit());
+assertSame(ChronoUnit.DAYS,DAYS.toChronoUnit());
+
+// Every TimeUnit has a defined ChronoUnit equivalent
+for (TimeUnit x : TimeUnit.values())
+assertSame(x, TimeUnit.of(x.toChronoUnit()));
+}
+
+/**
+ * tests for TimeUnit.of(ChronoUnit).
+ */
+public void testTimeUnitOf() throws Exception {
+assertSame(NANOSECONDS,  TimeUnit.of(ChronoUnit.NANOS));
+assertSame(MICROSECONDS, TimeUnit.of(ChronoUnit.MICROS));
+assertSame(MILLISECONDS, TimeUnit.of(ChronoUnit.MILLIS));
+assertSame(SECONDS,  TimeUnit.of(ChronoUnit.SECONDS));
+assertSame(MINUTES,  TimeUnit.of(ChronoUnit.MINUTES));
+assertSame(HOURS,TimeUnit.of(ChronoUnit.HOURS));
+assertSame(DAYS, TimeUnit.of(ChronoUnit.DAYS));
+
+assertThrows(NullPointerException.class,
+ () -> TimeUnit.of((ChronoUnit)null));
+
+// ChronoUnits either round trip to their TimeUnit
+// equivalents, or throw IllegalMonitorStateExceptio

RE: RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file

[Ramanand Patil]

Hi Roger,

 

Writing the regression test will be little tricky, because the transition rules 
are read from the IANA provided tzdata files and during JDK building the 
tzdb.dat file is created.

This issue was actually found after JDK9 build failure with integrated 
tzdata2016a files.

Being a very obvious trivial bug fix, I feel this should be Ok without test.

I have updated the label to include 'noreg-trivial'.

 

Regards,

Ramanand.

 

 

-Original Message-
From: Roger Riggs 
Sent: Friday, January 29, 2016 11:43 PM
To: core-libs-dev@openjdk.java.net
Subject: Re: RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer 
Exception While Compiling and building TZDB data file

 

Hi Ramanand,

 

Is there a specific regression test that can be written for the case?

 

Otherwise, looks fine.

 

Roger

 

 

On 1/29/2016 12:56 PM, Ramanand Patil wrote:

> HI all,

> 

> Please review this trivial fix for Bug  - 
> https://bugs.openjdk.java.net/browse/JDK-8148570

> 

> Bug Description - While compiling and building TZDB data file(tzdata2016a) 
> when transition rule(for Iran) doesn't have the day-of-week data a null 
> pointer exception is thrown.

> 

> Webrev - http://cr.openjdk.java.net/~rpatil/8148570/webrev.00/

> 

> Fix - While getting value from DayOfWeek, -1 is returned if the DayOfWeek is 
> null. The reason to return "-1" being, the same value is checked later while 
> getting day-of-week byte (dowbyte) at line no. 251, ZoneRules.java.

> 

>   

> 

> Regards,

> 

> Ramanand.

> 

>   

> 

>   

 

 

JDK 9 RFR of JDK-8148627: Problem list TestMaxCachedBufferSize.java

[joe darcy]

Hello,

Until JDK-8148540 is addressed, the test

sun/nio/ch/TestMaxCachedBufferSize.java

should be problem listed on the platforms is fails on. Please review the 
patch below.


Thanks,

-Joe

diff -r eecb3e75b0d8 test/ProblemList.txt
--- a/test/ProblemList.txtThu Jan 28 18:08:53 2016 -0800
+++ b/test/ProblemList.txtFri Jan 29 10:51:25 2016 -0800
@@ -197,6 +197,9 @@
 java/nio/file/WatchService/Basic.javasolaris-all
 java/nio/file/WatchService/LotsOfEvents.javasolaris-all

+# 8148540
+sun/nio/ch/TestMaxCachedBufferSize.java solaris-i586,windows-i586
+
 

 # jdk_rmi
diff -r eecb3e75b0d8 test/sun/nio/ch/TestMaxCachedBufferSize.java
--- a/test/sun/nio/ch/TestMaxCachedBufferSize.javaThu Jan 28 
18:08:53 2016 -0800
+++ b/test/sun/nio/ch/TestMaxCachedBufferSize.javaFri Jan 29 
10:51:25 2016 -0800

@@ -50,6 +50,7 @@
  * @run main/othervm -Djdk.nio.maxCachedBufferSize=1000 
TestMaxCachedBufferSize

  *
  * @summary Test the implementation of the jdk.nio.maxCachedBufferSize 
property.

+ * @key intermittent
  */
 public class TestMaxCachedBufferSize {
 private static final int DEFAULT_ITERS = 10 * 1000;

RFR 9: 8146773 java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails

[Roger Riggs]

Please review a fix for two test issues.  When run with -Xcomp and other 
switches that change
GC behavior; the CleanerTest was not giving enough to time to see the 
result of cleaning.

Also, added an adjustment of the number of cycles by the timeoutFactor.

Webrev:
  http://cr.openjdk.java.net/~rriggs/webrev-cleanertest-8146773/

8146773: java/lang/ref/CleanerTest.java CleanerTest.testRefSubtypes() fails
8148352: CleanerTest fails: Cleanable should have been freed

Thanks, Roger

Re: RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file

[Roger Riggs]

Hi Ramanand,

Is there a specific regression test that can be written for the case?

Otherwise, looks fine.

Roger


On 1/29/2016 12:56 PM, Ramanand Patil wrote:

HI all,

Please review this trivial fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8148570

Bug Description - While compiling and building TZDB data file(tzdata2016a) when 
transition rule(for Iran) doesn't have the day-of-week data a null pointer 
exception is thrown.

Webrev - http://cr.openjdk.java.net/~rpatil/8148570/webrev.00/

Fix - While getting value from DayOfWeek, -1 is returned if the DayOfWeek is null. The 
reason to return "-1" being, the same value is checked later while getting 
day-of-week byte (dowbyte) at line no. 251, ZoneRules.java.

  


Regards,

Ramanand.

  

  

RFR: 8148570: TzdbZoneRulesCompiler.java throws Null Pointer Exception While Compiling and building TZDB data file

[Ramanand Patil]

HI all,

Please review this trivial fix for Bug  - 
https://bugs.openjdk.java.net/browse/JDK-8148570

Bug Description - While compiling and building TZDB data file(tzdata2016a) when 
transition rule(for Iran) doesn't have the day-of-week data a null pointer 
exception is thrown.

Webrev - http://cr.openjdk.java.net/~rpatil/8148570/webrev.00/

Fix - While getting value from DayOfWeek, -1 is returned if the DayOfWeek is 
null. The reason to return "-1" being, the same value is checked later while 
getting day-of-week byte (dowbyte) at line no. 251, ZoneRules.java.

 

Regards,

Ramanand.

 

 

RFR [9]: 8138699 Iproving JAX-B javadoc

[Miroslav Kos]

Hi everybody,
please review following patch - those are changes to javadoc, previously 
consulted with sqe. The javadoc now is not always clear, which makes 
difficult to develop proper tests.


JIRA: https://bugs.openjdk.java.net/browse/JDK-8138699
webrev: http://cr.openjdk.java.net/~mkos/8138699/jaxws.01/

Thanks
Miran

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Paul Sandoz]

> On 29 Jan 2016, at 18:27, Steve Drach  wrote:
>> 
>> This may have come up before but JarFile has two sets of constructors, one 
>> takes the file path as a String, the other as a File. I just wondering about 
>> introduce a second constructor so that they match.
> 
> We felt that one constructor was sufficient on the theory that it’s use will 
> be infrequent, at least initially.  And it’s easy to map a String to a File.
> 

Right, my preference is to stick to one for the moment and add another later if 
really needed.


>> 
>> One other thing that I've been wondering about is the stream() and entries() 
>> methods. Has there been any discussion about these doing filter?
> 
> Not that I’m aware of.
> 
>> Maybe it is too expensive and best left to the user of the API? Part of the 
>> context for asking is modular JARs of course.
> 
> Perhaps you can elaborate.

Alan’s point is that traversing using entries()/stream() always returns the 
versioned entries (if any) rather than all entries, thus in a sense filters.

My assumption was the traversal should by default be consistent with a calls to 
getEntry, thus:

 jarFile.stream().forEach(e -> {
JarEntry je = jarFile.getJarEntry(e.getName());
assert e.equals(je);
 });

There might need to be another stream method that returns all entries.

Paul.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Stephen Colebourne]

In the ideal fix, all methods that take the combination (long,
TimeUnit) would be supplemented by an override that takes (Duration).
eg. Future.get(long, TimeUnit) has an additional get(Duration).
However, this is a lot of work, might be unpopular and would be slower
for j.u.concurrent use.

The proposal is minimal, allowing the conversion to occur more easily.
The Duration.of(long, TemporalUnit) could then be used to create a
Duration.

I'll note again that the alternative to this would be to make TimeUnit
implement TemporalUnit. Whether that is less confusing is an open
question.

The specific question asked is whether there should be a
TimeUnit::toDuration(long) method. I'm not overly fussed, as I think
there are probably enough alternatives.

Stephen


On 29 January 2016 at 17:24, Martin Buchholz  wrote:
> I propose that we the jsr166 maintainers take over this change (sorry
> for butting in!), pushing it into openjdk from jsr166 CVS.
> The tests as they are written today won't work because
> TimeUnit/Basic.java is not a testng test.
> But I don't think we should fix that - instead, tests for these
> methods should be added to our tck tests (I can do that).
> Coincidentally, I am close to committing our tck tests to openjdk.
>
> As for the new API - the simple changes in the webrev are perfectly
> fine, but I expected to see some additional conversions.  TimeUnit is
> for expressing elapsed time durations (even though j.u.c. doesn't have
> a separate class for that), so I vaguely expect conversions to/from
> Duration.  But we won't do anything unless date/time experts
> (Stephen?) think it's a good idea.
>
>
> On Fri, Jan 29, 2016 at 8:46 AM, Martin Buchholz  wrote:
>> I missed that this was modifying jsr166 files - looking now...
>>
>> On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty  
>> wrote:
>>> On 29/01/16 14:52, Roger Riggs wrote:

 Hi Nadeesh,

 Looks fine,

 Thanks, Roger


 On 1/27/2016 11:34 AM, nadeesh tv wrote:
>
> Hi all,
>
> Thanks for the suggestions.
> Please see the updated webrev
> http://cr.openjdk.java.net/~ntv/8141452/webrev.01/
>>>
>>>
>>> +1  This looks fine.
>>>
>>> Martin, Doug,
>>>
>>>   I assume you are ok to accept this small change in
>>> java.util.concurrent.TimeUnit.
>>>
>>> -Chris.
>>>
>>>
> Regards,
> Nadeesh TV
>
> On 1/25/2016 10:24 PM, Roger Riggs wrote:
>>
>> Hi Stephen, Nadeesh,
>>
>> TimeUnit.toChronoUnit is a static method.  It seems redundant to have
>> to pass an instance to a static method of its type.
>>  cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS);
>>
>> Instead of:
>>TimeUnit tu = TimeUnit.SECONDS;
>> ChronoUnit  cu = tu.toChronoUnit();
>>
>>
>> Minor edits please:
>>
>> in @param and @return use the type name when referring to the type.
>> For example, TimeUnit vs timeUnit (the parameter).
>>
>> in @throws, use the parameter name instead of "the unit";
>> For example,
>> + * @throws IllegalArgumentException if timeUnit cannot be converted
>> Thanks, Roger
>>
>> On 1/25/2016 11:06 AM, nadeesh tv wrote:
>>>
>>> Hi all,
>>>
>>> Please see the updated webrev
>>> http://cr.openjdk.java.net/~ntv/8141452/webrev.00/
>>>
>>> --
>>> Thanks and Regards,
>>> Nadeesh TV
>>>
>>>
>>> On 1/25/2016 9:01 PM, Stephen Colebourne wrote:

 Typo "TimeUnitequivalent"
 Otherwise looks good.
 thanks
 Stephen



 On 25 January 2016 at 15:25, nadeesh tv  wrote:

> Hi all,
>
> Please review a fix for conversion between Chronounit and Timeunit
>
> Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452
>
> webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/
>
> --
> Thanks and Regards,
> Nadeesh TV
>
>
>>>
>>
>
> --
> Thanks and Regards,
> Nadeesh TV
>

>>>

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Steve Drach]

>> I’ve released a new webrev, 
>> http://cr.openjdk.java.net/~sdrach/8132734/webrev.04/index.html 
>>  that 
>> addresses the above issue.
>> 
> Thank you, the javadoc is much clearer and readable now.
> 
> The first mention of multi-release JARs is in the first paragraph where it 
> has "for processing multi-release jar files". I would be tempted to drop this 
> from the first paragraph because the second paragraph introduces the concept.

It makes sense to leave it in there because of the context.  It’s one of 2 
things that differentiate JarFile from ZipFile
.
> 
> In the second paragraph is has "The versioned entries are partitioned by the 
> major version of Java platform releases, starting with release 9" and then it 
> goes on to explain how versioned entries are partitioned. This has potential 
> to confuse the reader as it gives an initial impression that the oldest 
> version entry is 9 but then the says 8 < n. I realize the text is trying to 
> say that Java SE 9 is the first release to support this but it could be 
> confused. I would be tempted to just drop the mention of release 9 in this 
> paragraph.

I’ll do that.

> 
> This may have come up before but JarFile has two sets of constructors, one 
> takes the file path as a String, the other as a File. I just wondering about 
> introduce a second constructor so that they match.

We felt that one constructor was sufficient on the theory that it’s use will be 
infrequent, at least initially.  And it’s easy to map a String to a File.

> 
> One other thing that I've been wondering about is the stream() and entries() 
> methods. Has there been any discussion about these doing filter?

Not that I’m aware of.

> Maybe it is too expensive and best left to the user of the API? Part of the 
> context for asking is modular JARs of course.

Perhaps you can elaborate.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Martin Buchholz]

I propose that we the jsr166 maintainers take over this change (sorry
for butting in!), pushing it into openjdk from jsr166 CVS.
The tests as they are written today won't work because
TimeUnit/Basic.java is not a testng test.
But I don't think we should fix that - instead, tests for these
methods should be added to our tck tests (I can do that).
Coincidentally, I am close to committing our tck tests to openjdk.

As for the new API - the simple changes in the webrev are perfectly
fine, but I expected to see some additional conversions.  TimeUnit is
for expressing elapsed time durations (even though j.u.c. doesn't have
a separate class for that), so I vaguely expect conversions to/from
Duration.  But we won't do anything unless date/time experts
(Stephen?) think it's a good idea.


On Fri, Jan 29, 2016 at 8:46 AM, Martin Buchholz  wrote:
> I missed that this was modifying jsr166 files - looking now...
>
> On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty  
> wrote:
>> On 29/01/16 14:52, Roger Riggs wrote:
>>>
>>> Hi Nadeesh,
>>>
>>> Looks fine,
>>>
>>> Thanks, Roger
>>>
>>>
>>> On 1/27/2016 11:34 AM, nadeesh tv wrote:

 Hi all,

 Thanks for the suggestions.
 Please see the updated webrev
 http://cr.openjdk.java.net/~ntv/8141452/webrev.01/
>>
>>
>> +1  This looks fine.
>>
>> Martin, Doug,
>>
>>   I assume you are ok to accept this small change in
>> java.util.concurrent.TimeUnit.
>>
>> -Chris.
>>
>>
 Regards,
 Nadeesh TV

 On 1/25/2016 10:24 PM, Roger Riggs wrote:
>
> Hi Stephen, Nadeesh,
>
> TimeUnit.toChronoUnit is a static method.  It seems redundant to have
> to pass an instance to a static method of its type.
>  cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS);
>
> Instead of:
>TimeUnit tu = TimeUnit.SECONDS;
> ChronoUnit  cu = tu.toChronoUnit();
>
>
> Minor edits please:
>
> in @param and @return use the type name when referring to the type.
> For example, TimeUnit vs timeUnit (the parameter).
>
> in @throws, use the parameter name instead of "the unit";
> For example,
> + * @throws IllegalArgumentException if timeUnit cannot be converted
> Thanks, Roger
>
> On 1/25/2016 11:06 AM, nadeesh tv wrote:
>>
>> Hi all,
>>
>> Please see the updated webrev
>> http://cr.openjdk.java.net/~ntv/8141452/webrev.00/
>>
>> --
>> Thanks and Regards,
>> Nadeesh TV
>>
>>
>> On 1/25/2016 9:01 PM, Stephen Colebourne wrote:
>>>
>>> Typo "TimeUnitequivalent"
>>> Otherwise looks good.
>>> thanks
>>> Stephen
>>>
>>>
>>>
>>> On 25 January 2016 at 15:25, nadeesh tv  wrote:
>>>
 Hi all,

 Please review a fix for conversion between Chronounit and Timeunit

 Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452

 webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/

 --
 Thanks and Regards,
 Nadeesh TV


>>
>

 --
 Thanks and Regards,
 Nadeesh TV

>>>
>>

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Paul Sandoz]

> On 29 Jan 2016, at 17:43, Hamlin Li  wrote:
> 
> 
> 
> On 2016/1/29 20:53, Paul Sandoz wrote:
>>> On 29 Jan 2016, at 13:43, Hamlin Li  wrote:
>>> 
>>> Hi Paul,
>>> 
>>> Sorry for delayed response, have been occupied by other higher priority 
>>> task.
>>> Thanks for your review, I agree with you that your second approach is 
>>> better.
>>> New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/
>>> 
>> The changes to the data providers look ok.
>> 
>> Would you mind splitting out the tests between StreamTestData and 
>> StreamTestData.small as outlined in 2) below. That way for the 
>> non-eplosive stuff we can still crunch on larger data without much of a slow 
>> down.
> Hi Pual,
> 
> Yes, you're right, it does not slow down too much, it cost 15.553 seconds 
> after the first revision(webrev.01), and it cost 16.064 after the second 
> revision(webrev.02).
> Please check the webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.02/ 
> 

+1, reviewed. Do you need me to push it?


>>> Below are times cost for different ops:
>>>  total:169.996
>>>  testOps only:108.988
>>>  testIntOps only :23.865
>>>  testLongOps only :22.326
>>>  testDoubleOps only :16.944
>>> so, I build small data providers for each of them.
>>> 
>> Ok, and i suspect/hope it drops by at least an order of magnitude with your 
>> changes applied.
> Yes, it cost 15.553 seconds after the first revision(webrev.01).

Great.


>> Out of curiosity i wonder what the times would be if using parallel GC 
>> rather than G1.
> With different GC options after second revision(webrev.02):
>  -UseParallelGC:  elapsed time (seconds): 16.047
>  +UseParallelGC: elapsed time (seconds): 13.263
>  -UseG1GC: elapsed time (seconds): 16.612
>  +UseG1GC:elapsed time (seconds): 16.998
>  -UseParallelOldGC: elapsed time (seconds): 16.039
>  +UseParallelOldGC: elapsed time (seconds): 14.297
> 

Ok, so i suspect in the case of when your patch is not applied the difference 
is greater in absolute terms and G1 in a sense might be causing such memory 
intensive tests to slow down.

Paul.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Martin Buchholz]

I missed that this was modifying jsr166 files - looking now...

On Fri, Jan 29, 2016 at 7:12 AM, Chris Hegarty  wrote:
> On 29/01/16 14:52, Roger Riggs wrote:
>>
>> Hi Nadeesh,
>>
>> Looks fine,
>>
>> Thanks, Roger
>>
>>
>> On 1/27/2016 11:34 AM, nadeesh tv wrote:
>>>
>>> Hi all,
>>>
>>> Thanks for the suggestions.
>>> Please see the updated webrev
>>> http://cr.openjdk.java.net/~ntv/8141452/webrev.01/
>
>
> +1  This looks fine.
>
> Martin, Doug,
>
>   I assume you are ok to accept this small change in
> java.util.concurrent.TimeUnit.
>
> -Chris.
>
>
>>> Regards,
>>> Nadeesh TV
>>>
>>> On 1/25/2016 10:24 PM, Roger Riggs wrote:

 Hi Stephen, Nadeesh,

 TimeUnit.toChronoUnit is a static method.  It seems redundant to have
 to pass an instance to a static method of its type.
  cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS);

 Instead of:
TimeUnit tu = TimeUnit.SECONDS;
 ChronoUnit  cu = tu.toChronoUnit();


 Minor edits please:

 in @param and @return use the type name when referring to the type.
 For example, TimeUnit vs timeUnit (the parameter).

 in @throws, use the parameter name instead of "the unit";
 For example,
 + * @throws IllegalArgumentException if timeUnit cannot be converted
 Thanks, Roger

 On 1/25/2016 11:06 AM, nadeesh tv wrote:
>
> Hi all,
>
> Please see the updated webrev
> http://cr.openjdk.java.net/~ntv/8141452/webrev.00/
>
> --
> Thanks and Regards,
> Nadeesh TV
>
>
> On 1/25/2016 9:01 PM, Stephen Colebourne wrote:
>>
>> Typo "TimeUnitequivalent"
>> Otherwise looks good.
>> thanks
>> Stephen
>>
>>
>>
>> On 25 January 2016 at 15:25, nadeesh tv  wrote:
>>
>>> Hi all,
>>>
>>> Please review a fix for conversion between Chronounit and Timeunit
>>>
>>> Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452
>>>
>>> webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/
>>>
>>> --
>>> Thanks and Regards,
>>> Nadeesh TV
>>>
>>>
>

>>>
>>> --
>>> Thanks and Regards,
>>> Nadeesh TV
>>>
>>
>

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Hamlin Li]

On 2016/1/29 20:53, Paul Sandoz wrote:

On 29 Jan 2016, at 13:43, Hamlin Li  wrote:

Hi Paul,

Sorry for delayed response, have been occupied by other higher priority task.
Thanks for your review, I agree with you that your second approach is better.
New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/


The changes to the data providers look ok.

Would you mind splitting out the tests between StreamTestData and 
StreamTestData.small as outlined in 2) below. That way for the non-eplosive 
stuff we can still crunch on larger data without much of a slow down.

Hi Pual,

Yes, you're right, it does not slow down too much, it cost 15.553 
seconds after the first revision(webrev.01), and it cost 16.064 after 
the second revision(webrev.02).

Please check the webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.02/

Below are times cost for different ops:
  total:169.996
  testOps only:108.988
  testIntOps only :23.865
  testLongOps only :22.326
  testDoubleOps only :16.944
so, I build small data providers for each of them.


Ok, and i suspect/hope it drops by at least an order of magnitude with your 
changes applied.

Yes, it cost 15.553 seconds after the first revision(webrev.01).

Out of curiosity i wonder what the times would be if using parallel GC rather 
than G1.

With different GC options after second revision(webrev.02):
  -UseParallelGC:  elapsed time (seconds): 16.047
  +UseParallelGC: elapsed time (seconds): 13.263
  -UseG1GC: elapsed time (seconds): 16.612
  +UseG1GC:elapsed time (seconds): 16.998
  -UseParallelOldGC: elapsed time (seconds): 16.039
  +UseParallelOldGC: elapsed time (seconds): 14.297

Thank you
-Hamlin


Paul.


Thank you
-Hamlin

On 2016/1/26 21:18, Paul Sandoz wrote:

Hi Hamlin,

Conservatively I would prefer not to remove data sets if at all possible. It 
will affect all tests, and leaf tasks for parallel streams should have enough 
data to crunch on.

I suspect the problem of the flatMap test is not necessarily due to the source 
sizes being of 1000 elements but that there are tests that substitute an 
element whose value is m for n elements from 0..m, which can explode things and 
generate lots of garbage.

Have you tried executing those kinds tests when the data size is < 1000?

My bet is the FlatMapOpTest will run significantly faster and you will not need 
to split it out.

There are two ways we could consider doing this:

1) Check the size in the test method:

if (data.size() < 1000) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

2) Include a new data provider for smaller data sets

@Test(dataProvider = "StreamTestData", dataProviderClass = 
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRef data) {
 Collection result = exerciseOps(data, s -> s.flatMap(mfId));
 assertEquals(data.size(), result.size());

 result = exerciseOps(data, s -> s.flatMap(mfNull));
 assertEquals(0, result.size());

 result = exerciseOps(data, s-> s.flatMap(e -> Stream.empty()));
 assertEquals(0, result.size());
}

@Test(dataProvider = "StreamTestData.small", dataProviderClass = 
StreamTestDataProvider.class)
public void testOpsX(String name, TestData.OfRef data) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

I prefer the latter approach (applied to ref and primitive data sets). It’s 
more work, but i think the right direction.

Paul.

Re: RFR: 8079136: Accessing a nested sublist leads to StackOverflowError

[Ivan Gerasimov]

Thank you Tagir for the review!

On 29.01.2016 7:16, Tagir F. Valeev wrote:

Hello!

AbstractList::subListRangeCheck could be replaced with new
Objects::checkFromToIndex for consistency with other methods. Also

Yes, that would be good.
However, for AbstractList.subList() we have this in spec:
 * @throws IllegalArgumentException if the endpoint indices are out 
of order

 * {@code (fromIndex > toIndex)}

but the 2-args checkFromToIndex() will throw IndexOutOfBoundsException 
instead.


If we use 3-args checkFromToIndex, it wouldn't save us anything in the 
terms of lines of code.


It's misfortune that the spec of subList() doesn't match the spec of 
similar methods, like CharSequence.subSequence() or String.substring().
It even contradicts the spec of List.subList(), which declares only IOOB 
to be thrown!




AbstractList::rangeCheck could be replaced with Obejcts::checkIndex.
The same for ArrayList class.


Right.
Here's the updated webrev:
http://cr.openjdk.java.net/~igerasim/8079136/06/webrev/

Sincerely yours,
Ivan


Otherwise looks good to me.

With best regards,
Tagir Valeev.

IG> Hello everyone!

IG> I'd like to respin the discussion.
IG> The previous attempt can be found here:
IG> http://mail.openjdk.java.net/pipermail/core-libs-dev/2015-May/033159.html

IG> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8079136
IG> WEBREV: http://cr.openjdk.java.net/~igerasim/8079136/05/webrev/

IG> Here's the summary of the proposed changes:
IG> 1)
IG> Sublist of an AbstractList (AbstractList.SubList class) now maintains a
IG> link to the root AbstractList, and not only to the immediate parent list.
IG> This allows to perform modifications on the chain of sub-lists in a loop
IG> instead of using recursion (which, in particular, helps avoid the
IG> stack-overflow problem).
IG> The sublist is still backed by its parent list, in sense that all the
IG> modifications are correctly reflected in the backing list (as well as in
IG> all the grand parents the sublist, if any), so the fix does not violate
IG> the existing specification.

IG> 2)
IG> It is proposed to update the spec of AbstractList.subList() in the
IG> @implSpec section by removing the words about private fields.
IG> With the fix, some of those private fields are removed.

IG> 3)
IG> A similar change is proposed for the ArrayList.SubList class.

IG> 4)
IG> Two regression tests are added:
IG> NestedSubList.java - demonstrates a stack-overflow problem when dealing
IG> with a long chain of sublists,
IG> SubList.java - tests basic functionality of sub-lists, which helps us
IG> make sure nothing is broken with the proposed change.

IG> If people agree that the fix is good, I'll file a CCC request for
IG> changing the spec of AbstractList.subList().

IG> Sincerely yours,
IG> Ivan

Re: JNI VERSION CHANGE: RFR: 8145098: JNI GetVersion should return JNI_VERSION_9

[Rachel Protacio]

Thanks for the review, Dan. We'll see how the 9 v. 9_0 discussion plays out.

Rachel

On 1/28/2016 1:37 PM, Daniel D. Daugherty wrote:

On 1/27/16 4:02 PM, Rachel Protacio wrote:

Hello!

Small but important change for review: updating the JNI_VERSION and 
in so doing, changing the format from JNI_VERSION_1_x to 
JNI_VERSION_x_y (see code/bug for details).


Bug: https://bugs.openjdk.java.net/browse/JDK-8145098

hotspot repo webrev: http://cr.openjdk.java.net/~rprotacio/JNI_hotspot/


src/share/vm/prims/jni.cpp
No comments.

src/share/vm/prims/jni.h
No comments.

src/share/vm/runtime/thread.cpp
No comments.

test/native_sanity/JniVersion.java
No comments.



jdk repo webrev: http://cr.openjdk.java.net/~rprotacio/JNI_jdk/


src/java.base/share/native/include/jni.h
No comments.

Meta-comment not specific to this bug. It sure would be nice if
finally had "one source of truth" for "jni.h" (and friends).
Perhaps in the top-level repo... :-)


Thumbs up!

On Alan's comments about JNI_VERSION_9_0 versus JNI_VERSION_9, my
personal preference is for trailing zero version. There was recently
a comment on hotspot-dev@o.j.n and verona-dev@o.j.n about the missing
trailing zeros. Subject line is: Version special case '9'

http://mail.openjdk.java.net/pipermail/hotspot-dev/2016-January/021525.html 



Dan




I grep'ed through the code to find references to the current JNI 
version and believe I have caught all the ones that needed changing, 
plus the fact that all these tests pass:


 * local hotspot jtreg tests
 * my own sample JNI test to print and visually inspect the version
   (essentially what is performed by the updated
   hotspot/test/native_sanity/JniVersion.java test)
 * jck vm tests
 * local rbt colcated and noncolocated tests, especially for the
   purpose of hitting tonga/src/nsk/share/jvmti tests where
   JNI_GetVersion() is used

Thank you,
Rachel

Re: RFR 8148115: Stream.findFirst for unordered source optimization

[Paul Sandoz]

> On 29 Jan 2016, at 15:20, Tagir F. Valeev  wrote:
> 
> Here's updated webrev:
> 
> http://cr.openjdk.java.net/~tvaleev/webrev/8148115/r2/
> 

Thanks, reviewed and in my queue.


> PS>  293 if (!this.mustFindFirst) {
> PS>  310 if (this.mustFindFirst) {
> PS> Small thing, no need for “this”.
> 
> Done.
> 
> 
> PS> You can now use Optional.ifPresentOrElse, which was added in 9:
> 
> PS>   ifPresentOrElse(
> PS> t -> { …},
> PS> () -> assertFalse(it.hasNext()):
> 
> Done. I also added printing of the searched value if it's not found:
> 
> assertTrue(contained, "Not found: "+r);
> 
> Hopefully additional toString() and string concatenation does not eat
> much time. Alternatively it could be rewritten as
> 
> if(!contained)
>  fail("Not found: "+r);
> 
> PS> For primitive streams, you are correct there are no unordered
> PS> providers of data. in certain cases unordered is induced e.g. forEach 
> testing.
> 
> PS> A focused fix would be to exercise with say
> PS> s.filter().unordered(), a more general fix would be as you say to
> PS> add another provider for int, long and double (backed by say
> PS> HashSet, or more simply/efficiently using unordered()).
> 
> PS> We have to carefully review that because that may increase test
> PS> times. We don’t have to apply to all data sets, plus we could also
> PS> refine the ref data sets too in that regard, thus keep the overall 
> actions similar or even smaller.
> 
> PS> Shall we separate that in a new issue?
> 
> Yes, I guess it would be better to log a new issue. Though I don't
> know how to formulate it correctly. "Add tests for unordered primitive
> sources for Stream operations"? "Refine test data sets for Stream
> API"? Probably it would be better if you open an issue by yourself :-)
> 

https://bugs.openjdk.java.net/browse/JDK-8148615 


Thanks,
Paul.

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Chris Hegarty]

On 29/01/16 14:52, Roger Riggs wrote:

Hi Nadeesh,

Looks fine,

Thanks, Roger


On 1/27/2016 11:34 AM, nadeesh tv wrote:

Hi all,

Thanks for the suggestions.
Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8141452/webrev.01/


+1  This looks fine.

Martin, Doug,

  I assume you are ok to accept this small change in 
java.util.concurrent.TimeUnit.


-Chris.


Regards,
Nadeesh TV

On 1/25/2016 10:24 PM, Roger Riggs wrote:

Hi Stephen, Nadeesh,

TimeUnit.toChronoUnit is a static method.  It seems redundant to have
to pass an instance to a static method of its type.
 cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS);

Instead of:
   TimeUnit tu = TimeUnit.SECONDS;
ChronoUnit  cu = tu.toChronoUnit();


Minor edits please:

in @param and @return use the type name when referring to the type.
For example, TimeUnit vs timeUnit (the parameter).

in @throws, use the parameter name instead of "the unit";
For example,
+ * @throws IllegalArgumentException if timeUnit cannot be converted
Thanks, Roger

On 1/25/2016 11:06 AM, nadeesh tv wrote:

Hi all,

Please see the updated webrev
http://cr.openjdk.java.net/~ntv/8141452/webrev.00/

--
Thanks and Regards,
Nadeesh TV


On 1/25/2016 9:01 PM, Stephen Colebourne wrote:

Typo "TimeUnitequivalent"
Otherwise looks good.
thanks
Stephen



On 25 January 2016 at 15:25, nadeesh tv  wrote:


Hi all,

Please review a fix for conversion between Chronounit and Timeunit

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452

webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/

--
Thanks and Regards,
Nadeesh TV








--
Thanks and Regards,
Nadeesh TV

Re: RFR:JDK-8141452:Convert between TimeUnit and ChronoUnit

[Roger Riggs]

Hi Nadeesh,

Looks fine,

Thanks, Roger


On 1/27/2016 11:34 AM, nadeesh tv wrote:

Hi all,

Thanks for the suggestions.
Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8141452/webrev.01/


Regards,
Nadeesh TV

On 1/25/2016 10:24 PM, Roger Riggs wrote:

Hi Stephen, Nadeesh,

TimeUnit.toChronoUnit is a static method.  It seems redundant to have 
to pass an instance to a static method of its type.

 cu = TimeUnit.toChronoUnit(TimeUnit.SECONDS);

Instead of:
   TimeUnit tu = TimeUnit.SECONDS;
ChronoUnit  cu = tu.toChronoUnit();


Minor edits please:

in @param and @return use the type name when referring to the type.
For example, TimeUnit vs timeUnit (the parameter).

in @throws, use the parameter name instead of "the unit";
For example,
+ * @throws IllegalArgumentException if timeUnit cannot be converted 
Thanks, Roger


On 1/25/2016 11:06 AM, nadeesh tv wrote:

Hi all,

Please see the updated webrev 
http://cr.openjdk.java.net/~ntv/8141452/webrev.00/


--
Thanks and Regards,
Nadeesh TV


On 1/25/2016 9:01 PM, Stephen Colebourne wrote:

Typo "TimeUnitequivalent"
Otherwise looks good.
thanks
Stephen



On 25 January 2016 at 15:25, nadeesh tv  wrote:


Hi all,

Please review a fix for conversion between Chronounit and Timeunit

Bug ID : https://bugs.openjdk.java.net/browse/JDK-8141452

webrev: http://cr.openjdk.java.net/~ntv/8141452/webrev.00/

--
Thanks and Regards,
Nadeesh TV








--
Thanks and Regards,
Nadeesh TV

Stream.foldLeft, one more time (8133680)

[Tagir F. Valeev]

Hello!

What do you think, is it still reasonable to add foldLeft operation to
JDK-9 Stream API (foldLeft only, no foldRight)?

I propose the following signatures:

interface Stream {
  public  U foldLeft(U seed, BiFunction accumulator);
  public Optional foldLeft(BinaryOperator accumulator);
}

interface IntStream {
  public int foldLeft(int seed, IntBinaryOperator accumulator);
  public OptionalInt foldLeft(IntBinaryOperator accumulator);
}

interface LongStream {
  public OptionalLong foldLeft(LongBinaryOperator accumulator);
  public long foldLeft(long seed, LongBinaryOperator accumulator);
}

interface DoubleStream {
  public OptionalDouble foldLeft(DoubleBinaryOperator accumulator);
  public double foldLeft(double seed, DoubleBinaryOperator accumulator);
}

They are similar to the corresponding reduce operations, but it's not
required for accumulator to be associative and it's not required for
seed to have identity properties. It should be clearly stated in
JavaDoc that for associative accumulator reduce should be used
instead.

All methods can be easily implemented as default methods in the
interfaces. For example, DoubleStream::foldLeft:

public double foldLeft(double seed, DoubleBinaryOperator accumulator) {
double[] box = new double[] { seed };
forEachOrdered(t -> box[0] = accumulator.applyAsDouble(box[0], t));
return box[0];
}

I can take this issue (if nobody works on it already), but in order
not to do the useless work I want to be sure that there are no strong
objections against such feature.

With best regards,
Tagir Valeev.

Re: RFR 8148115: Stream.findFirst for unordered source optimization

[Tagir F. Valeev]

Here's updated webrev:

http://cr.openjdk.java.net/~tvaleev/webrev/8148115/r2/

PS>  293 if (!this.mustFindFirst) {
PS>  310 if (this.mustFindFirst) {
PS> Small thing, no need for “this”.

Done.


PS> You can now use Optional.ifPresentOrElse, which was added in 9:

PS>   ifPresentOrElse(
PS> t -> { …},
PS> () -> assertFalse(it.hasNext()):

Done. I also added printing of the searched value if it's not found:

assertTrue(contained, "Not found: "+r);

Hopefully additional toString() and string concatenation does not eat
much time. Alternatively it could be rewritten as

if(!contained)
  fail("Not found: "+r);

PS> For primitive streams, you are correct there are no unordered
PS> providers of data. in certain cases unordered is induced e.g. forEach 
testing.

PS> A focused fix would be to exercise with say
PS> s.filter().unordered(), a more general fix would be as you say to
PS> add another provider for int, long and double (backed by say
PS> HashSet, or more simply/efficiently using unordered()).

PS> We have to carefully review that because that may increase test
PS> times. We don’t have to apply to all data sets, plus we could also
PS> refine the ref data sets too in that regard, thus keep the overall actions 
similar or even smaller.

PS> Shall we separate that in a new issue?

Yes, I guess it would be better to log a new issue. Though I don't
know how to formulate it correctly. "Add tests for unordered primitive
sources for Stream operations"? "Refine test data sets for Stream
API"? Probably it would be better if you open an issue by yourself :-)

With best regards,
Tagir Valeev.

PS> —

PS> I will take this and other patches (close/limit etc) you propose in bulk.

PS> Thanks,
PS> Paul.

>> On 23 Jan 2016, at 13:43, Tagir F. Valeev  wrote:
>> 
>> Hello, Paul!
>> 
>> Thank you for review. I implemented it according to your suggestions,
>> here's issue:
>> https://bugs.openjdk.java.net/browse/JDK-8148115
>> And webrev:
>> http://cr.openjdk.java.net/~tvaleev/webrev/8148115/r1/
>> 
>> This patch required some changes in existing tests as FindFirstOpTest
>> asserted that findFirst() result must be equal to .iterator().next()
>> even if the source is HashSet. I extracted the relevant part of
>> FindAnyOpTest into assertContains(Optional, Iterator) method and
>> placed it into LambdaTestHelpers.
>> 
>> The corresponding changes in primitive tests are unnecessary, because
>> it seems that primitive data providers never produce unordered
>> streams. I can try to add unordered primitive sources and correct
>> these tests as well if it's necessary.
>> 
>> With best regards,
>> Tagir Valeev.
>> 
>> 
>> PS> Hi Tagir,
>> 
>> PS> Thanks for looking at this.
>> 
>> PS> I would prefer if a boolean was passed into the task constructor e.g.:
>> 
>> PS> @Override
>> PS> public  O evaluateParallel(PipelineHelper helper,
>> PS>  Spliterator spliterator) {
>> PS> // This takes into account the upstream ops flags and the terminal
>> PS> // op flags and therefore takes into account findFirst or findAny
>> PS> boolean mustFindFirst =
>> PS> StreamOpFlag.ORDERED.isKnown(helper.getStreamAndOpFlags());
>> PS> return new FindTask<>(this, mustFindFirst, helper, 
>> spliterator).invoke();
>> PS> }
>> 
>> PS> so we keep the flag evaluation logic within the evaluate method.
>> 
>> PS> Then change FindOp.mustFindFirst to be an int of the opFlags calculated 
>> in the constructor:
>> 
>> PS>   this.opFlags = StreamOpFlag.IS_SHORT_CIRCUIT | (mustFindFirst ? 0 : 
>> StreamOpFlag.NOT_ORDERED);
>> 
>> PS> Paul.
>> 
 On 22 Jan 2016, at 07:19, Tagir F. Valeev  wrote:
 
 Hello!
 
 Seems that currently Stream.findFirst is not optimized for unordered
 source. I think it should work as findAny in this case. Here's a small
 patch which fixes this:
 
 http://cr.openjdk.java.net/~tvaleev/patches/findFirst/find_patch.txt
 
 Simple JMH test:
 http://cr.openjdk.java.net/~tvaleev/patches/findFirst/FindTest.java
 
 Original:
 http://cr.openjdk.java.net/~tvaleev/patches/findFirst/jmh_out_orig.txt
 # JMH 1.11.2 (released 85 days ago)
 # VM version: JDK 9-ea, VM 9-ea+99-2015-12-23-183325.javare.4146.nc
 BenchmarkMode  Cnt  Score  Error  Units
 FindTest.findAny avgt   30  12439,631 ± 1787,866  us/op
 FindTest.findAnyUnorderedavgt   30  12923,080 ± 1072,537  us/op
 FindTest.findFirst   avgt   30  48047,467 ± 2713,489  us/op
 FindTest.findFirstUnordered  avgt   30  52648,893 ± 3934,682  us/op
 
 Patched:
 http://cr.openjdk.java.net/~tvaleev/patches/findFirst/jmh_out_patched.txt
 BenchmarkMode  Cnt  Score  Error  Units
 FindTest.findAny avgt   30  11312,238 ±  386,627  us/op
 FindTest.findAnyUnorderedavgt   30  12136,953 ± 1536,817  us/op
 FindTest.findFirst   avgt   30 

Re: RFR-8148250: Stream.limit parallel ordered performance

[Paul Sandoz]

> On 29 Jan 2016, at 14:47, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> Here's webrev:
> http://cr.openjdk.java.net/~tvaleev/webrev/8148250/r1/
> 

Thanks, reviewed and in my queue to push.

Paul.

RFR-8148250: Stream.limit parallel ordered performance

[Tagir F. Valeev]

Hello!

Here's webrev:
http://cr.openjdk.java.net/~tvaleev/webrev/8148250/r1/

With best regards,
Tagir Valeev.

>> On 26 Jan 2016, at 16:51, Tagir F. Valeev  wrote:
>> 
>> Hello!
>> 
>> Thank you for review! Here's the issue:
>> https://bugs.openjdk.java.net/browse/JDK-8148250
>> Will post complete webrev later.
>> 
>> PS> Note that it is still the case that in almost all scenarios this
>> PS> is likely to be a bad form of parallel stream.
>> 
>> Well not always it's possible to estimate in advance the size of the
>> stream. Consider that we have user-specified filter upstream which
>> searches over the big collection (we want to return first 10 search
>> results, order is important):
>> 
>> data.parallelStream()
>>.filter(element -> userQuery.test(element))
>>.limit(10).collect(toList());
>> 
>> If user query produces 10-15 results, using parallel stream is very
>> reasonable, but if it produces millions of results it should not
>> regress very much (at least should not become much slower than
>> sequential version which is what we see currently).

PS> I have my doubts that the cost of splitting and filtering a small
PS> number of elements concurrently will pay off in the majority of
PS> scenarios, hence the “almost all”.

PS> It could work in cases where there is lots of data to be filtered
PS> and only a few items are reported that are proportionally spread
PS> out, or over small data and the filter operation is costly.

PS> In any case it’s good to avoid the OOME, i am very glad you found a simple 
way to resolve that.


>> 
>> PS> I think the comment you refer to still applies but now for larger n, so 
>> we should refine it.
>> 
>> Should we replace "regardless of the value of n" with "when
>> n*parallelismLevel is sufficiently large”?

PS> Yes, when N * P is sufficiently large e.g to pluck a number out of the air 
> 2^32, say

PS> Paul.

Re: RFR - 8132734: java.util.jar.* changes to support multi-release jar files

[Alan Bateman]

On 27/01/2016 23:37, Steve Drach wrote:


I'm still wondering about the phrase "root entry" as it continues to 
give the impression (to me anyway) that it's a resource in the root 
directory. I think "root" works in the JEP because it deals with 
simple resources like A.class and B.class that are in the root 
directory but it's confusing when there resources with a slash in the 
name. Add to this is the META-INF/versions/ directories which are 
roots for the version specific resources. I think part of the 
confusion is that the first mention of "root entry" is in the second 
paragraph where it has "overrides the unversioned root entry" without 
defining what it means. In summary, I'm wondering whether you would 
be up for change the terminology so that "root entry" isn't in the 
javadoc?


I’ve released a new webrev, 
http://cr.openjdk.java.net/~sdrach/8132734/webrev.04/index.html 
 that 
addresses the above issue.



Thank you, the javadoc is much clearer and readable now.

The first mention of multi-release JARs is in the first paragraph where 
it has "for processing multi-release jar files". I would be tempted to 
drop this from the first paragraph because the second paragraph 
introduces the concept.


In the second paragraph is has "The versioned entries are partitioned by 
the major version of Java platform releases, starting with release 9" 
and then it goes on to explain how versioned entries are partitioned. 
This has potential to confuse the reader as it gives an initial 
impression that the oldest version entry is 9 but then the says 8 < n. I 
realize the text is trying to say that Java SE 9 is the first release to 
support this but it could be confused. I would be tempted to just drop 
the mention of release 9 in this paragraph.


This may have come up before but JarFile has two sets of constructors, 
one takes the file path as a String, the other as a File. I just 
wondering about introduce a second constructor so that they match.


One other thing that I've been wondering about is the stream() and 
entries() methods. Has there been any discussion about these doing 
filter? Maybe it is too expensive and best left to the user of the API? 
Part of the context for asking is modular JARs of course.


-Alan

Re: RFR (L) JEP 280: Indify String Concatenation (integration)

[Erik Joelsson]

On 2016-01-27 14:55, Aleksey Shipilev wrote:

  c) (XS) Build changes that force emitting the "legacy" inline
StringBuilder concat in a few cases (e.g. when pre-JDK 9 bytecode is
expected):
http://cr.openjdk.java.net/~shade/8085796/webrev.root.00/
In what context do we need pre-JDK 9 bytecode for java.base and 
jdk.compiler?


/Erik

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Paul Sandoz]

> On 29 Jan 2016, at 13:43, Hamlin Li  wrote:
> 
> Hi Paul,
> 
> Sorry for delayed response, have been occupied by other higher priority task.
> Thanks for your review, I agree with you that your second approach is better.
> New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/
> 

The changes to the data providers look ok.

Would you mind splitting out the tests between StreamTestData and 
StreamTestData.small as outlined in 2) below. That way for the 
non-eplosive stuff we can still crunch on larger data without much of a slow 
down.


> 
> Below are times cost for different ops:
>  total:169.996
>  testOps only:108.988
>  testIntOps only :23.865
>  testLongOps only :22.326
>  testDoubleOps only :16.944
> so, I build small data providers for each of them.
> 

Ok, and i suspect/hope it drops by at least an order of magnitude with your 
changes applied.

Out of curiosity i wonder what the times would be if using parallel GC rather 
than G1.

Paul.

> Thank you
> -Hamlin
> 
> On 2016/1/26 21:18, Paul Sandoz wrote:
>> Hi Hamlin,
>> 
>> Conservatively I would prefer not to remove data sets if at all possible. It 
>> will affect all tests, and leaf tasks for parallel streams should have 
>> enough data to crunch on.
>> 
>> I suspect the problem of the flatMap test is not necessarily due to the 
>> source sizes being of 1000 elements but that there are tests that substitute 
>> an element whose value is m for n elements from 0..m, which can explode 
>> things and generate lots of garbage.
>> 
>> Have you tried executing those kinds tests when the data size is < 1000?
>> 
>> My bet is the FlatMapOpTest will run significantly faster and you will not 
>> need to split it out.
>> 
>> There are two ways we could consider doing this:
>> 
>> 1) Check the size in the test method:
>> 
>> if (data.size() < 1000) {
>> exerciseOps(data, s -> s.flatMap(mfLt));
>> exerciseOps(data, s -> s.flatMap(integerRangeMapper));
>> exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
>> e).boxed().limit(10)));
>> }
>> 
>> 2) Include a new data provider for smaller data sets
>> 
>> @Test(dataProvider = "StreamTestData", dataProviderClass = 
>> StreamTestDataProvider.class)
>> public void testOps(String name, TestData.OfRef data) {
>> Collection result = exerciseOps(data, s -> s.flatMap(mfId));
>> assertEquals(data.size(), result.size());
>> 
>> result = exerciseOps(data, s -> s.flatMap(mfNull));
>> assertEquals(0, result.size());
>> 
>> result = exerciseOps(data, s-> s.flatMap(e -> Stream.empty()));
>> assertEquals(0, result.size());
>> }
>> 
>> @Test(dataProvider = "StreamTestData.small", dataProviderClass = 
>> StreamTestDataProvider.class)
>> public void testOpsX(String name, TestData.OfRef data) {
>> exerciseOps(data, s -> s.flatMap(mfLt));
>> exerciseOps(data, s -> s.flatMap(integerRangeMapper));
>> exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
>> e).boxed().limit(10)));
>> }
>> 
>> I prefer the latter approach (applied to ref and primitive data sets). It’s 
>> more work, but i think the right direction.
>> 
>> Paul.

Re: RFR: 8076458: java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java timeout

[Hamlin Li]

Hi Paul,

Sorry for delayed response, have been occupied by other higher priority 
task.
Thanks for your review, I agree with you that your second approach is 
better.

New webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.01/


Below are times cost for different ops:
  total:169.996
  testOps only:108.988
  testIntOps only :23.865
  testLongOps only :22.326
  testDoubleOps only :16.944
so, I build small data providers for each of them.

Thank you
-Hamlin

On 2016/1/26 21:18, Paul Sandoz wrote:

Hi Hamlin,

Conservatively I would prefer not to remove data sets if at all possible. It 
will affect all tests, and leaf tasks for parallel streams should have enough 
data to crunch on.

I suspect the problem of the flatMap test is not necessarily due to the source 
sizes being of 1000 elements but that there are tests that substitute an 
element whose value is m for n elements from 0..m, which can explode things and 
generate lots of garbage.

Have you tried executing those kinds tests when the data size is < 1000?

My bet is the FlatMapOpTest will run significantly faster and you will not need 
to split it out.

There are two ways we could consider doing this:

1) Check the size in the test method:

if (data.size() < 1000) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

2) Include a new data provider for smaller data sets

@Test(dataProvider = "StreamTestData", dataProviderClass = 
StreamTestDataProvider.class)
public void testOps(String name, TestData.OfRef data) {
 Collection result = exerciseOps(data, s -> s.flatMap(mfId));
 assertEquals(data.size(), result.size());

 result = exerciseOps(data, s -> s.flatMap(mfNull));
 assertEquals(0, result.size());

 result = exerciseOps(data, s-> s.flatMap(e -> Stream.empty()));
 assertEquals(0, result.size());
}

@Test(dataProvider = "StreamTestData.small", dataProviderClass = 
StreamTestDataProvider.class)
public void testOpsX(String name, TestData.OfRef data) {
 exerciseOps(data, s -> s.flatMap(mfLt));
 exerciseOps(data, s -> s.flatMap(integerRangeMapper));
 exerciseOps(data, s -> s.flatMap((Integer e) -> IntStream.range(0, 
e).boxed().limit(10)));
}

I prefer the latter approach (applied to ref and primitive data sets). It’s 
more work, but i think the right direction.

Paul.


On 26 Jan 2016, at 08:08, Hamlin Li  wrote:

Hi everyone,

Would you please help to review the fix for bug 
https://bugs.openjdk.java.net/browse/JDK-8076458, 
java/util/stream/test/org/openjdk/tests/java/util/stream/FlatMapOpTest.java 
timeout.
webrev: http://cr.openjdk.java.net/~mli/8076458/webrev.00/

Thank you
-Hamlin

Re: RFR 8138578:MethodHandles.Lookup.findSpecial() Javadoc fails to consider static methods

[shilpi rastogi]

Hi All,

Please review the updated patch-

http://cr.openjdk.java.net/~srastogi/8138578/webrev.01/

I verified Lookup.unreflectSpecial() also throws 
java.lang.IllegalAccessException so updated the JavaDoc.


Thanks,
Shilpi



 Original Message 
Subject: 	Re: RFR 8138578:MethodHandles.Lookup.findSpecial() Javadoc 
fails to consider static methods

Date:   Fri, 22 Jan 2016 10:30:29 +0100
From:   Paul Sandoz 
CC: core-libs-dev@openjdk.java.net



Hi Shilpi,

Can you also double check Lookup.unreflectSpecial, it’s documentation might 
require updating too.

Separating the “or” statements with a comma would help readability in the 
JavaDoc (same applies to the the patch for findVirtual).

Paul.

> On 22 Jan 2016, at 10:22, shilpi rastogi  wrote:
>
>
> Gentle Reminder!
>
>  Original Message 
> Subject:   [9u-dev] 8138578:MethodHandles.Lookup.findSpecial() Javadoc fails 
to consider static methods
> Date:  Wed, 20 Jan 2016 18:12:30 +0530
> From:  shilpi rastogi
> Organization:  Oracle Corporation
> To:core-libs-dev@openjdk.java.net
>
>
>
> Hi All,
>
> Please review my fix related to java doc.
>
> MethodHandles.Lookup.findSpecial() Javadoc fails to consider static methods-
>
> MethodHandles.Lookup.findSpecial() throws IllegalAccessException if the 
target method is static.
>
> Bug link-https://bugs.openjdk.java.net/browse/JDK-8138578  

> Webrev link-http://cr.openjdk.java.net/~srastogi/8138578/webrev.00/  

>
> Thnaks,
> Shilpi
>
>

Re: ArrayList.subList().spliterator() is not late-binding

[Paul Sandoz]

Hi Tagir,

Last-binding while adding some complexity to the implementation is an important 
property, as it is to fail-fast on a best effort basis.

When one has mutable collections that can be structurally modified and lazy 
views over those collections (Streams/Iterators) then those views really need 
to act on the most up to date structure and not a snapshot (structural  or 
otherwise) when the view was created. Otherwise, it is a potential source of 
bugs producing incorrect results or exceptions, or another form of complexity 
for the implementation to maintain integrity under such conditions.

We took a short-cut here to reuse the existing top-level spliterator in a 
non-late-binding manner by specifying the absolute fence. We should fix it.

Glancing at the code i believe there is a simple fix. When -1 is passed as the 
fence to the ArrayList.ArrayListSpliterator then the fence is created lazily 
and should be the result of offset + list.size e.g. in the constructor:

  hi = fence = offset + lst.size;

The forEach impl also needs updating.

—

Sublists are also tricky beasts. See documentation on List.subList:

* The semantics of the list returned by this method become undefined if
* the backing list (i.e., this list) is structurally modified in
* any way other than via the returned list.  (Structural modifications are
* those that change the size of this list, or otherwise perturb it in such
* a fashion that iterations in progress may yield incorrect results.)

Paul.

> On 29 Jan 2016, at 05:32, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> When reviewing 8079136 I noticed that
> ArrayList.subList().spliterator() is not late-binding:
> 
> List list = new ArrayList<>(Arrays.asList(1,2,3,4)).subList(0, 3);
> Stream 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.
>