Re: RFR for JDK-8027973. javax/xml/jaxp/transform/jdk8004476/XSLTExFuncTest.java hangs (win)

[huizhe wang]

Thanks for working on the issue.  The fix looks good.

Regards,
Joe

On 11/20/2013 7:58 PM, Patrick Zhang wrote:

Hi Everyone,

I am working on https://bugs.openjdk.java.net/browse/JDK-8027973. The 
problem is caused by wrong URL format on windows. file://c:\xxx is one 
invalid schema.


Solution:
Replace "file://" with "file:///" then it will work on windows and it 
will not impact other platforms. And remove the test from ProblemList.txt


webrev:
http://cr.openjdk.java.net/~pzhang/8027973/webrev/

Result on windows:
http://cr.openjdk.java.net/~pzhang/8027973/XSLTExFuncTest.jtr
Result on Linux:
http://cr.openjdk.java.net/~pzhang/8027973/XSLTExFuncTest.linux.jtr

Regards
Patrick

Re: RFR(2): 7174936: several String methods claim to always create new String

[Stuart Marks]

On 11/15/13 8:36 AM, Alan Bateman wrote:

On 14/11/2013 23:56, Stuart Marks wrote:


On 11/14/13 2:04 AM, David Holmes wrote:

Sorry for the delay (been enroute). Only issue I have remains the subSequence
change - you can't weaken the post-condition of CharSequence.subSequence without
breaking subtype substitutability.


Yes, in general, what you say about weakening post-conditions is true. In this
case, though, I can't see how it can cause any practical harm.

I've looked through the webrev and read the exchange between you and David.

I think it might be better to leave the subSequence change out for now (the
@apiNote is fine) and submit another bug to track the discrepancy between the
spec and implementation. From what I can tell, this has existed since
CharSequence was added in 1.4, in which case there may be a good argument to
just document the potential deviation.


OK, I'll remove the String.subSequence change from this changeset and push it 
when I get approval.


I've filed this bug:

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

to cover the CharSequence.subSequence issue and potential spec change. It 
probably isn't that much work to do, but it probably is easier to handle it 
separately.


s'marks

RFR for JDK-8027973. javax/xml/jaxp/transform/jdk8004476/XSLTExFuncTest.java hangs (win)

[Patrick Zhang]

Hi Everyone,

I am working on https://bugs.openjdk.java.net/browse/JDK-8027973. The 
problem is caused by wrong URL format on windows. file://c:\xxx is one 
invalid schema.


Solution:
Replace "file://" with "file:///" then it will work on windows and it 
will not impact other platforms. And remove the test from ProblemList.txt


webrev:
http://cr.openjdk.java.net/~pzhang/8027973/webrev/

Result on windows:
http://cr.openjdk.java.net/~pzhang/8027973/XSLTExFuncTest.jtr
Result on Linux:
http://cr.openjdk.java.net/~pzhang/8027973/XSLTExFuncTest.linux.jtr

Regards
Patrick

Re: RFR: 8028185 - XMLFormatter.format emits incorrect year

[Stuart Marks]

On 11/19/13 9:40 AM, Mandy Chung wrote:

On 11/19/13 2:23 AM, Daniel Fuchs wrote:

Hi,

Please find below a webrev for:

8028185: XMLFormatter.format emits incorrect year
https://bugs.openjdk.java.net/browse/JDK-8028185

The fix is trivial:
http://cr.openjdk.java.net/~dfuchs/webrev_8028185/webrev.00/


Looks good.   Nit: the test can use StringBuilder which is generally in
preference to StringBuffer unless synchronization is required.

The kind of warning fix [1] causing this regression should probably have a
regression test to go with it to verify the change.

thanks
Mandy
[1] http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c1f129f62f36


Interesting and subtle. To save others the research, the changeset that 
introduced the regression removed a deprecation warning by replacing


Date.getYear() + 1900

with

Calendar.get(Calendar.YEAR) + 1900

This is incorrect and thus introduced the regression. The javadoc for 
Date.getYear() says to replace calls to it with:


Calendar.get(Calendar.YEAR) - 1900

So, during warnings cleanup, a careful review of the code change plus the 
javadoc for the deprecated method being replaced would have revealed the error. 
Obviously, though, it's difficult always to be sufficiently careful in practice.


Alan said earlier,


This one is a good reminder as to how fixing warnings can break things.


I think this is true. The warnings fixes aren't risk-free. For example, even 
generifying code can introduce ClassCastExceptions if the original code were to 
put values of different types into collections. This style has always been rare, 
but older code would do things like that.


Instead of writing regression tests for warnings fixes, though, I think I'd 
rather have good unit tests for a particular area. That way, general 
maintenance, as well as cleanups (like fixing warnings) are better protected 
against regressions.


s'marks

Re: 8022206: Intermittent test failures in java/lang/ProcessBuilder/Basic.java

[Stuart Marks]

On 11/19/13 1:41 PM, Alan Bateman wrote:

On 19/11/2013 20:14, Martin Buchholz wrote:

In jsr166 tests we have mostly switched to 10 second timeouts to mean "forever".
But in ProcessBuilder tests we are starting up new java processes with their
well-known startup problems, so using a much larger value of "forever" seems
reasonable.  I vote for 1 minute.  You can write

waitFor(1, TimeUnit.MINUTES);


I agree that a timeout on that order might be needed when starting process.
However I think this one is just starting a thread that exercises the new
waitFor method.


I'll share a discussion we had before I pushed my "fix" yesterday that raised a 
timeout from 30 to 60 seconds.


http://hg.openjdk.java.net/jdk8/tl/jdk/rev/19d2e9649138

Some of the test machines in our test farm can be startlingly, even absurdly 
slow. In this case part of the test involved having a parent process fork a 
child process, and the child turns around and connects back to the parent. 
There's a 30 second timeout for this. It should never timeout, right? But it did 
in our testing environment.


One clue is that on my laptop, this test (including framework setup and 
teardown) took about 8 seconds to run. The test log of the failure showed that 
the test took 2 MINUTES AND 49 SECONDS to run.


(This was on 32-bit Windows, of course.)

I can only guess at what causes the machines to run so slowly (virus scanning? 
other VMs running on the same host?) but sometimes they can be so slow that the 
unbelievable happens.


s'marks

Re: RFR for JDK-6772009 Intermittent test failure: java/util/concurrent/locks/ReentrantLock/CancelledLockLoops.java test failed with 'Completed != 2'

[David Holmes]

On 21/11/2013 10:28 AM, Martin Buchholz wrote:

I again tried and failed to reproduce a failure.  Even if I go whole hog
and multiply TIMEOUT by 100 and divide ITERS by 100, the test continues to
PASS.  Is it just me?!


I think you are going the wrong way Martin - you want the timeout to be 
smaller than the time it takes to execute ITERS.



I don't think there's any reason to make result long.  It's not even used
except to inhibit hotspot optimizations.

+private volatile long result = 17;//Can get int overflow,so using long


Further the subsequent use of += is incorrect as it is not an atomic 
operation. Even if we don't care about the value, it looks bad.


I'm not sure result must be updated if we get a BrokenBarrierException 
either. Probably harmless, but necessary?



need to fix spelling and spacing below.

+barrier.await();//If a BrokeBarrierException happens
here(due to


There are a number of style issues with spacing around comments.

And I don't think this change is sufficient to claim co-author status 
with Doug either ;-)


The additional tracing may be useful but seems stylistically different 
from the rest of the code.


Overall I'm suspicious that the changed timeout actually fixes anything 
- normally we need to add longer timeouts not shorter ones. Does this 
fail on a range of machines or only specific ones? Have we verified that 
the clocks/timers are behaving properly on those systems?


Thanks,
David





On Wed, Nov 20, 2013 at 11:52 AM, srikalyan <
srikalyan.chandrashe...@oracle.com> wrote:


  Hi Martin , apologies for the delay , was trying to get help for hosting
my webrev.  .  Please see inline text.


On 11/19/13, 10:35 PM, Martin Buchholz wrote:

Hi Kalyan,

  None of us can review your changes yet because you haven't given us a
URL of your webrev.

It is located here
http://cr.openjdk.java.net/~cl/host_for_srikalyan_6772009_CancelledLockLoops/


  I've tried to make the jsr166 copy of CancelledLockLoops fail by
adjusting ITERS and TIMEOUT radically up and down, but the test just keeps
on passing for me.  Hints appreciated.

Bump up the timeout to 500ms and you will see a failure (i can see it
consistently on my machine Linux 64bit,8GBRAM,dual cores, with JDK 1.8
latest any promoted build).



On Tue, Nov 19, 2013 at 6:39 PM, srikalyan chandrashekar <
srikalyan.chandrashe...@oracle.com> wrote:


Suggested Fix:

a) Decrease the timeout from 100 to 50ms which will ensure that the test
will pass even on faster machines




  This doesn't look like a permanent fix - it just makes the failing case
rarer.

Thats true , the other way is to make the main thread wait on TIMEOUT
after firing the interrupts instead of other way round, but that would be
over-optimization which probably is not desirable as well.  The 50 ms was
arrived at empirically after running several 100 times on multiple
configurations and did not cause failures.

--
Thanks
kalyan
Ph: (408)-585-8040

Re: RFR: JDK-8028628 - java/nio/channels/FileChannel/Size.java failed once in the same binary run

[Dan Xu]

Hi All,

I have updated my fix based on your suggestions. I have changed to 
create testing files in the working directory, moved those static member 
variables into local method variables, and used try-with-resources to 
read and write the testing files. After the change, the file delete is 
no longer important. So I just do the clean-up with deleteOnExit() for 
simplicity. If the test fails, it is better to keep the test file to 
give more clue. Therefore, I don't put the file clean-up into finally 
block. Thanks!


Webrev: http://cr.openjdk.java.net/~dxu/8028628/webrev01/

-Dan


On 11/20/2013 04:08 AM, Alan Bateman wrote:

On 19/11/2013 23:57, Dan Xu wrote:

Hi All,

Please review the simple fix towards Size.java testcase. It failed 
once on windows platform in the recent same binary run, which is 
mostly due to some interferences and the special delete handling on 
windows.


In the fix, I remove the delete operation in initTestFile() method 
because FileOutputStream will truncate the file content and it is not 
necessary to delete it first. Thanks!


Bug:https://bugs.openjdk.java.net/browse/JDK-8028628
Webrev: http://cr.openjdk.java.net/~dxu/8028628/webrev/ 

This does look like a case where the test is needlessly deleting and 
re-creating the file (although still annoying to have interference 
from virus checkers or other background services). As you point out, 
FileOutputStream will truncate an existing file so it's not needed. So 
I think your changes to remove the exist/delete from the init method 
is good.


If you have the cycles then there are probably a few clean-ups that 
could be done on this test. I don't think blah needs to be static, it 
could use try-with-resources and delete blah in the finally block. 
Also test2 looks historical, it may be that this can be enabled on 
Linux and Windows now (the bug/comments seem to date from JDK 1.4).


-Alan

Re: Map.Entry.setValue as a default method

[John Rose]

On Nov 20, 2013, at 4:31 PM, Remi Forax  wrote:

> But while you can declare a default hashCode and equals, it will not work 
> because the implementation of Object.hashCode and Object.equals will always 
> be preferred to the default methods by the VM, this is how default methods 
> are specified. Not something I'm very proud.

Next question:  What's the best practice for declaring reusable code for 
exactly those restricted methods (hashCode/equals, toString)?  Because of the 
irregularity with Object, the opt-in isn't by default, but there should still 
be a convention for supplying the code as a "would-be default method".

Maybe one of:
  interface KoolReusablePair {
default boolean defaultEquals(Object x) { ... }
static int hashCode(KoolReusablePair self) { ... }
...
  }

  class KoolImpl implements KoolReusablePair {
@Override //manual opt-in:
public boolean equals(Object x) { return 
KoolReusablePair.super.defaultEquals(x); }
@Override //manual opt-in:
public int hashCode() { return KoolReusablePair.hashCode(this); }
...
  }

— John

Re: Map.Entry.setValue as a default method

[Mike Duigou]

On Nov 20 2013, at 16:31 , Remi Forax  wrote:

> Hi mike,
> 
> On 11/21/2013 12:07 AM, Mike Duigou wrote:
>> It could still be added in a future rev.
> 
> yes, I've post that here as a remainder for the future generation :)
> 
>>  setValue was less compelling and obviously correct than Iterator.remove(). 
>> I had a version of Map.Entry earlier on which provided provided setValue() 
>> along defaults for the required implementations of hashCode() and equals(). 
>> The spec for these later two methods is part of the API so defaults seemed 
>> appropriate.
> 
> Totally appropriate because a lot of people forget to implement them when 
> implementing Map.Entry,
> 3 or 4 years back, hibernate collections had that issue, by example.
> 
> But while you can declare a default hashCode and equals, it will not work 
> because the implementation of Object.hashCode and Object.equals will always 
> be preferred to the default methods by the VM, this is how default methods 
> are specified. Not something I'm very proud.

Ah, yes. It had worked for a while but then that decision was made.

> 
>>  We essentially didn't implement them to avoid scope creep--none of these 
>> were strictly necessary for our other goals. For Iterator.remove() we could 
>> get immediate benefit.
>> 
>> Mike
> 
> Rémi
> 
> 
>> 
>> On Nov 20 2013, at 14:54 , Remi Forax  wrote:
>> 
>>> A good question of one of my student,
>>> why Iterator.remove() is a default method that throws UOE in 8 but 
>>> Map.Entry.setValue() is not a default method with the same semantics.
>>> 
>>> regards,
>>> Rémi
>>> 
> 

Re: Map.Entry.setValue as a default method

[Remi Forax]

Hi mike,

On 11/21/2013 12:07 AM, Mike Duigou wrote:

It could still be added in a future rev.


yes, I've post that here as a remainder for the future generation :)


  setValue was less compelling and obviously correct than Iterator.remove(). I 
had a version of Map.Entry earlier on which provided provided setValue() along 
defaults for the required implementations of hashCode() and equals(). The spec 
for these later two methods is part of the API so defaults seemed appropriate.


Totally appropriate because a lot of people forget to implement them 
when implementing Map.Entry,

3 or 4 years back, hibernate collections had that issue, by example.

But while you can declare a default hashCode and equals, it will not 
work because the implementation of Object.hashCode and Object.equals 
will always be preferred to the default methods by the VM, this is how 
default methods are specified. Not something I'm very proud.



  We essentially didn't implement them to avoid scope creep--none of these were 
strictly necessary for our other goals. For Iterator.remove() we could get 
immediate benefit.

Mike


Rémi




On Nov 20 2013, at 14:54 , Remi Forax  wrote:


A good question of one of my student,
why Iterator.remove() is a default method that throws UOE in 8 but 
Map.Entry.setValue() is not a default method with the same semantics.

regards,
Rémi

Re: Map.Entry.setValue as a default method

[Mike Duigou]

It could still be added in a future rev. setValue was less compelling and 
obviously correct than Iterator.remove(). I had a version of Map.Entry earlier 
on which provided provided setValue() along defaults for the required 
implementations of hashCode() and equals(). The spec for these later two 
methods is part of the API so defaults seemed appropriate. We essentially 
didn't implement them to avoid scope creep--none of these were strictly 
necessary for our other goals. For Iterator.remove() we could get immediate 
benefit.

Mike

On Nov 20 2013, at 14:54 , Remi Forax  wrote:

> A good question of one of my student,
> why Iterator.remove() is a default method that throws UOE in 8 but 
> Map.Entry.setValue() is not a default method with the same semantics.
> 
> regards,
> Rémi
> 

Map.Entry.setValue as a default method

[Remi Forax]

A good question of one of my student,
why Iterator.remove() is a default method that throws UOE in 8 but 
Map.Entry.setValue() is not a default method with the same semantics.


regards,
Rémi

hg: jdk8/tl/jdk: 8028734: test/java/util/Locale/InternationalBAT.java changes does not restore the default TimeZone

[alan . bateman]

Changeset: ecd6c25b54ce
Author:alanb
Date:  2013-11-20 21:34 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ecd6c25b54ce

8028734: test/java/util/Locale/InternationalBAT.java changes does not restore 
the default TimeZone
Reviewed-by: naoto

! test/java/util/Locale/InternationalBAT.java

Re: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use of fixed port

[Stuart Marks]

Hi, sorry about the delay, I'm still backlogged from traveling. I'll get to this 
soon.


s'marks

On 11/19/13 6:27 PM, Tristan Yan wrote:

Hi Stuart
Did you get chance to review it again.
Let me know if you have any new comments or suggestions.
Thanks
Tristan

-邮件原件-
发件人: Tristan Yan
发送时间: Thursday, November 14, 2013 11:09 PM
收件人: Stuart Marks
抄送: core-libs-dev@openjdk.java.net
主题: 答复: RFR for JDK-7190106 RMI benchmark fails intermittently because of use 
of fixed port

Thank you Stuart
It took me a little time to correct the code. sorry be late. This is new webrev 
for the code change. Please help to review it again.

Description:
1. Convert shell script test to Java program test.
2. Using random server port by reusing Darryl Mocek's 
work(TestLibrary.getUnusedRandomPort) to replace fixed server port.
3. Because TestLibrary doesn't have package. Here is using reflection to call 
TestLibrary.getUnusedRandomPort. This is going to change when TestLibrary moves 
to named package.
4. Using java Process class to start client process. Client and server are 
sharing IO.
5. Also convert other shell script test runSerialBench.sh to java program test 
also.
6. ProblemList has been changed to get back 
java/rmi/reliability/benchmark/runRmiBench.sh test.

http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/

Thank you so much
Tristan


-邮件原件-
发件人: Stuart Marks
发送时间: Tuesday, November 12, 2013 11:38 PM
收件人: Tristan Yan
抄送: core-libs-dev@openjdk.java.net; Alexandre (Shura) Iline
主题: Re: RFR for JDK-7190106 RMI benchmark fails intermittently because of use 
of fixed port

Unfortunately we can't use jdk.testlibrary.Utils.getFreePort() for the RMI tests, since 
RMI's TestLibrary.getUnusedRandomPort() respects a "reserved" port range that's 
used by some RMI tests that have to used fixed ports.

s'marks

On 11/11/13 2:39 PM, Tristan Yan wrote:

Hi Stuart
Also there is one more solution, which is there is one
jdk.testlibrary.Utils.getFreePort() method under test/lib. It's same
function as
TestLibrary.getUnusedRandomPort() and it has named package. Do you
mind I use this one?
Since these two functions provide same functionality. Maybe we should
think about to merge them at the same time.
Thank you
Tristan

On 11/10/2013 11:19 AM, Tristan Yan wrote:

Hi Stuart
I tried your suggestion but unfortunately all the benchmarks have
dependencies to Main class because they need get stub from server. I
suggest we move the benchmark tests to unnamed package unless we do
want to put TestLibrary into a named package right now.
Please let me know if you have objection on this.
Thank you
Tristan

On 11/09/2013 02:28 AM, Stuart Marks wrote:

Hi Tristan,

Yes, it's kind of a problem that the RMI TestLibrary is in the
unnamed package. Classes in a named package cannot import classes
from the unnamed package. We've run into problems with this before.
Eventually, we should move TestLibrary a named package.

I think it's possible to work around this without too much
difficulty. Note that classes in the unnamed package can import classes from 
named packages.
So, perhaps you can put the RmiBench main class in the unnamed
package so it has access to TestLibrary. Then have the benchmarks
themselves in the bench.rmi package. The config file already
references the benchmarks by fully qualified class name (e.g.,
"bench.rmi.NullCalls") so with a bit of tinkering you ought to be able to get 
this to work.

s'marks

On 11/8/13 3:00 AM, Tristan Yan wrote:

Thank you, Stuart
There is one review point I want to ask you opinion. Which is the
reason that I moved from
test/java/rmi/reliability/benchmark/bench/rmi to
test/java/rmi/reliability/benchmark is Main.java need access class
TestLibrary for supporting random port. TestLibrary is a unpackage
class, I couldn't find a way to let a class which has Package to access the 
class without package. Do you have suggestion on that?
Thank you so much.
Tristan

On 11/06/2013 09:50 AM, Stuart Marks wrote:



On 11/1/13 9:18 AM, Tristan Yan wrote:

Hi Everyone
http://cr.openjdk.java.net/~pzhang/Tristan/7190106/webrev/

Description:
1. Convert shell script test to Java program test.
2. Using random server port by reusing Darryl Mocek's work to
replace fixed server port.
3. Using java Process class to start client process.
4. Also convert other shell script test runSerialBench.sh to java
program test also


Hi Tristan,

Several comments on this webrev.


** The original arrangement within the
test/java/rmi/reliability/benchmark
directory had the main benchmark files (scripts) at the top, some
benchmark framework files in the "bench" subdirectory, and the
actual RMI and serialization benchmarks in bench/rmi and bench/serial 
subdirectories.

The webrev moves all the RMI benchmarks to the top benchmark
directory but leaves the serial benchmarks in bench/serial. The
RMI benchmarks are now all cluttering the top directory, but the
main serial benchmark test is now buried in the bench/serial
directory. Th

Re: RFR: JDK-8027413: Clarify javadoc for j.l.a.Target and j.l.a.ElementType

[Joel Borggrén-Franck]

Thanks for the review, change has been pushed.

cheers
/Joel

On 19 Nov 2013, at 21:52, Joe Darcy  wrote:

> Hi Joel,
> 
> The change looks good; approved to go back.
> 
> Thanks,
> 
> -Joe
> 
> On 11/15/2013 04:26 AM, Joel Borggren-Franck wrote:
>> Hi
>> 
>> Please review this javadoc clarification for j.l.annnotation.Target and
>> j.l.annotation.ElementType as part of the type annotations work.
>> 
>> Webrev: http://cr.openjdk.java.net/~jfranck/8027413/webrev.00/
>> JBS:https://bugs.openjdk.java.net/browse/JDK-8027413
>> 
>> This is based on the update to JLS from Alex:
>> http://cr.openjdk.java.net/~abuckley/308.pdf (section 1.6).
>> 
>> cheers
>> /Joel
> 

Re: RFR for JDK-6772009 Intermittent test failure: java/util/concurrent/locks/ReentrantLock/CancelledLockLoops.java test failed with 'Completed != 2'

[srikalyan]

Hi David , webrev is hosted here 
http://cr.openjdk.java.net/~cl/host_for_srikalyan_6772009_CancelledLockLoops/ 
.


--
Thanks
kalyan
Ph: (408)-585-8040


On 11/19/13, 11:03 AM, David Holmes wrote:

Hi,

Attachments are stripped. Please post on cr.openjdk.java.net (get a 
colleague to host this if you don't have an account yet.)


Thanks,
David

On 19/11/2013 4:12 PM, srikalyan chandrashekar wrote:

Hi all, I am working on bug JDK-6772009
 .

Root Cause:
The timeout value gives too much grace period on faster machines on
which the "TO BE INTERRUPTED" threads complete faster before being
interrupted at right time.

Suggested Fix:
a) Decrease the timeout from 100 to 50ms which will ensure that the test
will pass even on faster machines , and ensures the threads will be
canceled when running and anyways there is a Barrier to ensure the test
threads all complete gracefully.
Miscellaneous fixes
b) Convert result from int to long(possible integer overflow otherwise)
c) Catch BrokenBarrierException in more granular fashion in
ReentrantLockLoop to update and print the results (which will be missed
otherwise)
Add more diagnostics
d) Assign names to threads
e) Print which threads update the 'completed' variable.

I have attached the webrev for convenience as the changes are
interleaved and is best viewed as sdiff.
Please let me know if you have any comments or suggestions.



Thank you

Re: RFR for JDK-6772009 Intermittent test failure: java/util/concurrent/locks/ReentrantLock/CancelledLockLoops.java test failed with 'Completed != 2'

[srikalyan]

Hi Martin , apologies for the delay , was trying to get help for hosting 
my webrev.  .  Please see inline text.


On 11/19/13, 10:35 PM, Martin Buchholz wrote:

Hi Kalyan,

None of us can review your changes yet because you haven't given us a 
URL of your webrev.
It is located here 
http://cr.openjdk.java.net/~cl/host_for_srikalyan_6772009_CancelledLockLoops/ 



I've tried to make the jsr166 copy of CancelledLockLoops fail by 
adjusting ITERS and TIMEOUT radically up and down, but the test just 
keeps on passing for me.  Hints appreciated.
Bump up the timeout to 500ms and you will see a failure (i can see it 
consistently on my machine Linux 64bit,8GBRAM,dual cores, with JDK 1.8 
latest any promoted build).



On Tue, Nov 19, 2013 at 6:39 PM, srikalyan chandrashekar 
> wrote:



Suggested Fix:
a) Decrease the timeout from 100 to 50ms which will ensure
that the test will pass even on faster machines



This doesn't look like a permanent fix - it just makes the failing 
case rarer.
Thats true , the other way is to make the main thread wait on TIMEOUT 
after firing the interrupts instead of other way round, but that would 
be over-optimization which probably is not desirable as well.  The 50 ms 
was arrived at empirically after running several 100 times on multiple 
configurations and did not cause failures.


--
Thanks
kalyan
Ph: (408)-585-8040

Re: Initial request for review of JDK-8006572 DoubleStream.sum()/average() implementations that reduce numerical errors

[Joe Darcy]

Hi Mike,

On 11/20/2013 10:31 AM, Mike Duigou wrote:

- Collectors averagingDouble could use the same @implNote as in DoublePipeline.

- DoublePipeline implementation could document the usage of the double array 
indices.

- @summary in test.


I'll reflect these in the next iteration.



- I agree with Paul that refactoring as a testNG test would be nice.


While learning about testNG would be useful, I don't want to take that 
on right at the moment ;-)




- I wondered at the mechanism for combining compensation values. Do you have a 
reference which explains the correctness? I thought perhaps directly adding the 
compensations together before doing compensated addition of the two sums might 
be better.


One of the inquiries I have out to my numerical reviewer is how to best 
combine two compensations.


In the code as written, from the perspective of one compensation, the 
two doubles in the other other compensation are just two more double 
values. So I think this is correct, if not ideal.


The compensation portion of one running sum could be of vastly different 
magnitude than the compensation portion (or the high-order sum bits) of 
the other sum. Therefore, I believe a direct combination of either (sum1 
+ sum2) or (comp1 + comp2) would lose the numerical properties of 
interest here.


Thanks for the feedback,

-Joe



Mike


On Nov 20 2013, at 00:21 , Joe Darcy  wrote:


Hi Paul,

On 11/18/2013 07:38 AM, Paul Sandoz wrote:

Hi Joe,

You can use the three arg form of collect for DoublePipeline.sum/average impls, 
which is already used for average:

 public final OptionalDouble average() {
 double[] avg = collect(() -> new double[2],
(ll, i) -> {
ll[0]++;
ll[1] += i;
},
(ll, rr) -> {
ll[0] += rr[0];
ll[1] += rr[1];
});
 return avg[0] > 0
? OptionalDouble.of(avg[1] / avg[0])
: OptionalDouble.empty();
 }

That would be the most expedient way.

Thanks for the tip. For the moment, I'm feeling a bit expedient and used the 
array-based approach in an iteration of the change:

http://cr.openjdk.java.net/~darcy/8006572.3/

hg: jdk8/tl/langtools: 8027977: javadoc dies on NumberFormat/DateFormat subclass

[bhavesh . x . patel]

Changeset: ef44a2971cb1
Author:bpatel
Date:  2013-11-20 10:53 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/ef44a2971cb1

8027977: javadoc dies on NumberFormat/DateFormat subclass
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javadoc/DocEnv.java
+ test/com/sun/javadoc/testCompletionFailure/TestCompletionFailure.java
+ test/com/sun/javadoc/testCompletionFailure/pkg1/NumberFormatTest.java

Re: Initial request for review of JDK-8006572 DoubleStream.sum()/average() implementations that reduce numerical errors

[Mike Duigou]

- Collectors averagingDouble could use the same @implNote as in DoublePipeline.

- DoublePipeline implementation could document the usage of the double array 
indices.

- @summary in test.

- I agree with Paul that refactoring as a testNG test would be nice. 

- I wondered at the mechanism for combining compensation values. Do you have a 
reference which explains the correctness? I thought perhaps directly adding the 
compensations together before doing compensated addition of the two sums might 
be better.

Mike


On Nov 20 2013, at 00:21 , Joe Darcy  wrote:

> Hi Paul,
> 
> On 11/18/2013 07:38 AM, Paul Sandoz wrote:
>> Hi Joe,
>> 
>> You can use the three arg form of collect for DoublePipeline.sum/average 
>> impls, which is already used for average:
>> 
>> public final OptionalDouble average() {
>> double[] avg = collect(() -> new double[2],
>>(ll, i) -> {
>>ll[0]++;
>>ll[1] += i;
>>},
>>(ll, rr) -> {
>>ll[0] += rr[0];
>>ll[1] += rr[1];
>>});
>> return avg[0] > 0
>>? OptionalDouble.of(avg[1] / avg[0])
>>: OptionalDouble.empty();
>> }
>> 
>> That would be the most expedient way.
> 
> Thanks for the tip. For the moment, I'm feeling a bit expedient and used the 
> array-based approach in an iteration of the change:
> 
>http://cr.openjdk.java.net/~darcy/8006572.3/

RFR(L) - 2nd round: 8024854: Basic changes and files to build the class library on AIX

[Volker Simonis]

Hi,

this is the second review round for "8024854: Basic changes and files to
build the class library on
AIX".
The previous reviews can be found at the end of this mail in the references
section.

I've tried to address all the comments and suggestions from the first round
and to further streamline the patch (it perfectly builds on Linux/x86_64,
Linux/PPC664, AIX, Solaris/SPARC and Windows/x86_64). The biggest change
compared to the first review round is the new "aix/" subdirectory which
I've now created under "jdk/src" and which contains AIX-only code.

The webrev is against our
http://hg.openjdk.java.net/ppc-aix-port/stagerepository which has been
recently synchronised with the jdk8 master:

http://cr.openjdk.java.net/~simonis/webrevs/8024854.v2/

Below (and in the webrev) you can find some comments which explain the
changes to each file. In order to be able to push this big change, I need
the approval of reviewers from the lib, net, svc, 2d, awt and sec groups.
So please don't hesitate to jump at it - there are enough changes for
everybody:)

Thank you and best regards,
Volker

*References:*

The following reviews have been posted so far (thanks Iris for collecting
them :)

- Alan Bateman (lib): Initial comments (16 Sep [2])
- Chris Hegarty (lib/net): Initial comments (20 Sep [3])
- Michael McMahon (net): Initial comments (20 Sept [4])
- Steffan Larsen (svc): APPROVED (20 Sept [5])
- Phil Race (2d): Initial comments  (18 Sept [6]); Additional comments (15
Oct [7])
- Sean Mullan (sec): Initial comments (26 Sept [8])

[2]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001045.html
[3]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001078.html
[4]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001079.html
[5]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001077.html
[6]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001069.html
[7]: http://mail.openjdk.java.net/pipermail/2d-dev/2013-October/003826.html
[8]:
http://mail.openjdk.java.net/pipermail/ppc-aix-port-dev/2013-September/001081.html


*Detailed change description:*

The new "jdk/src/aix" subdirectory contains the following new and
AIX-specific files for now:

 src/aix/classes/sun/awt/fontconfigs/aix.fontconfig.properties
 src/aix/classes/sun/nio/ch/AixAsynchronousChannelProvider.java
 src/aix/classes/sun/nio/ch/AixPollPort.java
 src/aix/classes/sun/nio/fs/AixFileStore.java
 src/aix/classes/sun/nio/fs/AixFileSystem.java
 src/aix/classes/sun/nio/fs/AixFileSystemProvider.java
 src/aix/classes/sun/nio/fs/AixNativeDispatcher.java
 src/aix/classes/sun/tools/attach/AixAttachProvider.java
 src/aix/classes/sun/tools/attach/AixVirtualMachine.java
 src/aix/native/java/net/aix_close.c
 src/aix/native/sun/nio/ch/AixPollPort.c
 src/aix/native/sun/nio/fs/AixNativeDispatcher.c
 src/aix/native/sun/tools/attach/AixVirtualMachine.c
 src/aix/porting/porting_aix.c
 src/aix/porting/porting_aix.h

src/aix/porting/porting_aix.c
src/aix/porting/porting_aix.h

   - Added these two files for AIX relevant utility code.
   - Currently these files only contain an implementation of dladdr which
   is not available on AIX.
   - In the first review round the existing dladdr users in the code either
   called the version from the HotSpot on AIX (
   src/solaris/native/sun/awt/awt_LoadLibrary.c) or had a private copy of
   the whole implementation (src/solaris/demo/jvmti/hprof/hprof_md.c). This
   is now not necessary any more.

 The new file layout required some small changes to the makefiles to make
them aware of the new directory locations:

makefiles/CompileDemos.gmk

   - Add an extra argument to SetupJVMTIDemo which can be used to pass
   additional source locations.
   - Currently this is only used on AIX for the AIX porting utilities which
   are required by hprof.

makefiles/lib/Awt2dLibraries.gmk
makefiles/lib/ServiceabilityLibraries.gmk

   - On AIX add the sources of the AIX porting utilities to the build. They
   are required by src/solaris/native/sun/awt/awt_LoadLibrary.c and
   src/solaris/demo/jvmti/hprof/hprof_md.c because dladdr is not available
   on AIX.

makefiles/lib/NioLibraries.gmk

   - Make the AIX-build aware of the new NIO source locations under
   src/aix/native/sun/nio/.

makefiles/lib/NetworkingLibraries.gmk

   - Make the AIX-build aware of the new aix_close.c source location under
   src/aix/native/java/net/.

src/share/bin/jli_util.h

   - Define JLI_Lseek on AIX.

src/share/lib/security/java.security-aix

   - Provide default java.security-aix for AIX based on the latest Linux
   version as suggested by Sean Mullan.

src/share/native/common/check_code.c

   - On AIX malloc(0) and calloc(0, ...) return a NULL pointer, which is
   legal, but the code in check_code.c does not handles this properly. So
   we wrap the two methods on AIX and return a non-NULL pointer even if we
   allo

hg: jdk8/tl/jdk: 8028647: Add instrumentation in GetSafepointSyncTime.java and remove it from ProblemList.txt

[mandy . chung]

Changeset: 90e27a47ff28
Author:mchung
Date:  2013-11-20 10:00 -0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/90e27a47ff28

8028647: Add instrumentation in GetSafepointSyncTime.java and remove it from 
ProblemList.txt
Reviewed-by: sla, chegar

! test/ProblemList.txt
! test/sun/management/HotspotRuntimeMBean/GetSafepointSyncTime.java

Re: Request to review JDK-8028094

[Balchandra Vaidya]

Thanks Martin.  I will incorporate your suggested improvements, and
will send out another webrev.

Regards
Balchandra

On 19/11/2013 22:53, Martin Buchholz wrote:

I see this is already submitted.

I thought I could do better than using pkill, but no - there is no 
convenient communication from the java process to the grandchild. 
 This is a hairy test!


Nevertheless, I would like you to incorporate the following improvements:
- invoke pkill directly
- do some extra checking
- join with reader thread
- don't fail if pkill is not available

--- a/test/java/lang/ProcessBuilder/Basic.java
+++ b/test/java/lang/ProcessBuilder/Basic.java
@@ -2016,7 +2016,7 @@
 && new File("/bin/bash").exists()
 && new File("/bin/sleep").exists()) {
 final String[] cmd = { "/bin/bash", "-c", 
"(/bin/sleep )" };
-final String[] cmdkill = { "/bin/bash", "-c", 
"(/usr/bin/pkill -f \"sleep \")" };
+final String[] cmdkill = { "/usr/bin/pkill", "-f", 
"sleep " };

 final ProcessBuilder pb = new ProcessBuilder(cmd);
 final Process p = pb.start();
 final InputStream stdout = p.getInputStream();
@@ -2044,7 +2044,9 @@
 stdout.close();
 stderr.close();
 stdin.close();
-new ProcessBuilder(cmdkill).start();
+// Try to clean up the sleep process, but don't fret 
about it.

+try { new ProcessBuilder(cmdkill).start(); }
+catch (IOException noPkillCommandInstalled) { }
 //--
 // There remain unsolved issues with asynchronous close.
 // Here's a highly non-portable experiment to 
demonstrate:

@@ -2063,8 +2065,14 @@
 };
 new ProcessBuilder(wakeupJeff).start().waitFor();
 // If wakeupJeff worked, reader probably got EBADF.
-reader.join();
 }
+long startTime = System.nanoTime();
+long timeout = 60L * 1000L;
+reader.join(timeout);
+long elapsedTimeMillis =
+(System.nanoTime() - startTime) / (1024L * 1024L);
+check(elapsedTimeMillis < timeout);
+check(!reader.isAlive());
 }
 } catch (Throwable t) { unexpected(t); }



On Tue, Nov 19, 2013 at 4:21 AM, Balchandra Vaidya 
mailto:balchandra.vai...@oracle.com>> 
wrote:



Hi,

Here is one possible solution for the issue JDK-8028094.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev/


I am not sure pkill is available in all Unix flavor at /usr/bin
directory,
but it is present in Solaris and OEL 6. I have tested on Solaris
and OEL and "sleep " is no longer left over.


Thanks
Balchandra

Re: Request to review JDK-8028094

[Balchandra Vaidya]

Hi Martin,

It is just an effort try to cleanup the leftover sleep process before 
the test terminates.


Thanks
Balchandra

On 19/11/2013 19:44, Martin Buchholz wrote:
Is there an actual problem caused by all those sleep processes, or are 
you just trying to be tidy?



On Tue, Nov 19, 2013 at 6:07 AM, Balchandra Vaidya 
mailto:balchandra.vai...@oracle.com>> 
wrote:



Hi,

Here is one possible solution for the issue JDK-8028094.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev/


I am not sure pkill is available in all Unix flavor at /usr/bin
directory,
but it is present in Solaris and OEL 6. I have tested on Solaris
and OEL and "sleep " is no longer left over.


Thanks
Balchandra

Re: Request to review JDK-8028094

[Balchandra Vaidya]

On 19/11/2013 19:42, Martin Buchholz wrote:
It would be nice if every request for review included URLs for the bug 
report (in addition to the webrev).




Here is the bug id: https://bugs.openjdk.java.net/browse/JDK-8028094
The existing test has a check for the existence of /bin/sleep before 
running it.  We should do the same for any other Unix command like pkill.

Agree. I will work it.

Thanks
Balchandra

 But we should also try to think of better solutions than having to 
use pkill.



On Tue, Nov 19, 2013 at 6:07 AM, Balchandra Vaidya 
mailto:balchandra.vai...@oracle.com>> 
wrote:



Hi,

Here is one possible solution for the issue JDK-8028094.
http://cr.openjdk.java.net/~bvaidya/8/8028094/webrev/


I am not sure pkill is available in all Unix flavor at /usr/bin
directory,
but it is present in Solaris and OEL 6. I have tested on Solaris
and OEL and "sleep " is no longer left over.


Thanks
Balchandra

hg: jdk8/tl/jdk: 8027413: Clarify javadoc for j.l.a.Target and j.l.a.ElementType

[joel . franck]

Changeset: f39be11835ff
Author:jfranck
Date:  2013-11-20 13:12 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/f39be11835ff

8027413: Clarify javadoc for j.l.a.Target and j.l.a.ElementType
Reviewed-by: darcy

! src/share/classes/java/lang/annotation/ElementType.java
! src/share/classes/java/lang/annotation/Target.java

hg: jdk8/tl/langtools: 6557966: Multiple upper bounds of the TypeVariable

[jan . lahoda]

Changeset: 7c89d200781b
Author:jlahoda
Date:  2013-11-20 13:44 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/7c89d200781b

6557966: Multiple upper bounds of the TypeVariable
Summary: Adjusting javax.lang.model javadoc regarding IntersectionType, 
IntersectionType.accept now calls visitIntersection for all kinds of 
IntersectionTypes.
Reviewed-by: darcy, vromero
Contributed-by: joe.da...@oracle.com, jan.lah...@oracle.com

! src/share/classes/com/sun/tools/javac/code/Type.java
! src/share/classes/com/sun/tools/javac/comp/Attr.java
! src/share/classes/javax/lang/model/type/DeclaredType.java
! src/share/classes/javax/lang/model/type/IntersectionType.java
! src/share/classes/javax/lang/model/type/TypeVariable.java
! test/tools/javac/processing/model/type/IntersectionPropertiesTest.java

Re: RFR: JDK-8028628 - java/nio/channels/FileChannel/Size.java failed once in the same binary run

[Alan Bateman]

On 19/11/2013 23:57, Dan Xu wrote:

Hi All,

Please review the simple fix towards Size.java testcase. It failed 
once on windows platform in the recent same binary run, which is 
mostly due to some interferences and the special delete handling on 
windows.


In the fix, I remove the delete operation in initTestFile() method 
because FileOutputStream will truncate the file content and it is not 
necessary to delete it first. Thanks!


Bug:https://bugs.openjdk.java.net/browse/JDK-8028628
Webrev: http://cr.openjdk.java.net/~dxu/8028628/webrev/ 

This does look like a case where the test is needlessly deleting and 
re-creating the file (although still annoying to have interference from 
virus checkers or other background services). As you point out, 
FileOutputStream will truncate an existing file so it's not needed. So I 
think your changes to remove the exist/delete from the init method is good.


If you have the cycles then there are probably a few clean-ups that 
could be done on this test. I don't think blah needs to be static, it 
could use try-with-resources and delete blah in the finally block. Also 
test2 looks historical, it may be that this can be enabled on Linux and 
Windows now (the bug/comments seem to date from JDK 1.4).


-Alan

hg: jdk8/tl/jdk: 7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails

[erik . gahlin]

Changeset: 894a4bae9e33
Author:egahlin
Date:  2013-11-20 12:32 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/894a4bae9e33

7141544: TEST_BUG: com/sun/jdi/BreakpointWithFullGC.sh fails
Reviewed-by: sla

! test/com/sun/jdi/BreakpointWithFullGC.sh

Re: RFR: JDK-8028628 - java/nio/channels/FileChannel/Size.java failed once in the same binary run

[Chris Hegarty]

On 20/11/13 05:00, Mandy Chung wrote:

Thanks for the details Dan [1].  It is useful.   The reason why I was
wondering the difference with temp dir is that the test workdir might be
excluded from the anti-virus scanner on that test machine. Now you have
explained it.


Right, this would be my preference too. There is more control if tests 
write to the current working directory.


The changes look fine to me, but there is still a ' blah.delete();' on 
L63 of the new file. I think this should use 
FileUtils.deleteFileWithRetry() [1], as it could still fail, right?


Thanks,
-Chris.

[1] 
http://hg.openjdk.java.net/jdk8/tl/jdk/file/19d2e9649138/test/lib/testlibrary/jdk/testlibrary/FileUtils.java





thanks
Mandy
[1]
http://mail.openjdk.java.net/pipermail/net-dev/2013-November/007783.html

On 11/19/2013 8:13 PM, Dan Xu wrote:

Hi Mandy,

I think that writing to the current directory and writing to the temp
directory will get the same interference in this testcase. Because the
interference is mostly coming from the anti-virus software or some
windows system services. Any file changes in the file system may
trigger them. Due to the interference, if a test deletes a file and
then immediately create a file with the same name, the create
operation may fail with access denied exception. I have described it
in detail when discussing Chris's webrev,
http://openjdk.5641.n7.nabble.com/RFR-8022213-Intermittent-test-failures-in-java-net-URLClassLoader-Add-jdk-testlibrary-FileUtils-java-td165561.html.
Thanks!

-Dan

On 11/19/2013 07:08 PM, Mandy Chung wrote:


On 11/19/2013 3:57 PM, Dan Xu wrote:

Hi All,

Please review the simple fix towards Size.java testcase. It failed
once on windows platform in the recent same binary run, which is
mostly due to some interferences and the special delete handling on
windows.

In the fix, I remove the delete operation in initTestFile() method
because FileOutputStream will truncate the file content and it is
not necessary to delete it first. Thanks!

Bug:https://bugs.openjdk.java.net/browse/JDK-8028628
Webrev: http://cr.openjdk.java.net/~dxu/8028628/webrev/



The patch itself is okay.   Would you think if writing to a file in
the current directory rather than temp is less prone to the
interference?

Mandy

Re: Initial request for review of JDK-8006572 DoubleStream.sum()/average() implementations that reduce numerical errors

[Paul Sandoz]

Hi Joe,

On Nov 20, 2013, at 9:21 AM, Joe Darcy  wrote:

> Hi Paul,
> 
> On 11/18/2013 07:38 AM, Paul Sandoz wrote:
>> Hi Joe,
>> 
>> You can use the three arg form of collect for DoublePipeline.sum/average 
>> impls, which is already used for average:
>> 
>> public final OptionalDouble average() {
>> double[] avg = collect(() -> new double[2],
>>(ll, i) -> {
>>ll[0]++;
>>ll[1] += i;
>>},
>>(ll, rr) -> {
>>ll[0] += rr[0];
>>ll[1] += rr[1];
>>});
>> return avg[0] > 0
>>? OptionalDouble.of(avg[1] / avg[0])
>>: OptionalDouble.empty();
>> }
>> 
>> That would be the most expedient way.
> 
> Thanks for the tip. For the moment, I'm feeling a bit expedient and used the 
> array-based approach in an iteration of the change:
> 
>http://cr.openjdk.java.net/~darcy/8006572.3/
> .

In DoublePipeline:

 378 @Override
 379 public final double sum() {
 380 double[] summation = collect(() -> new double[2],
 381(ll, d) -> {
 382Collectors.sumWithCompensation(ll, d);
 383},
 384(ll, rr) -> {
 385Collectors.sumWithCompensation(ll, 
rr[0]);
 386Collectors.sumWithCompensation(ll, 
rr[1]);
 387});
 388 return summation[0];
 389 }


I think you can replace the lambda expression at #381 with a method reference.


 410 @Override
 411 public final OptionalDouble average() {
 412 double[] avg = collect(() -> new double[3],
 413(ll, d) -> {
 414ll[2]++;
 415Collectors.sumWithCompensation(ll, d);
 416},
 417(ll, rr) -> {
 418Collectors.sumWithCompensation(ll, 
rr[0]);
 419Collectors.sumWithCompensation(ll, 
rr[1]);
 420ll[2] += rr[2];
 421});
 422 return avg[0] > 0
 423? OptionalDouble.of(avg[0] / avg[2])
 424: OptionalDouble.empty();
 425 }


#422 needs to be "avg[2] > 0" since you changed the location of the count. It 
suggests we don't have tests for a non-empty stream whose sum is zero :-)

Other non-test code looks good.


> This allows one of the kernels of the compensated summation logic to be 
> shared in two of the use-sites.
> 
> For the test, all 6 scenarios fail on a JDK *without* the compensated 
> summation code and all 6 pass with it; all the other java/util/streams 
> regression tests pass with the new code too.
> 

I still strongly encourage you to remove the Iterator and replace with 
Stream.iterate. You can probably half the number of lines of test code.

The following:

  80 private static  Stream testBoxedStream(double base, double 
increment, int count) {
  81 TestDoubleIterator tdi = new TestDoubleIterator(base, increment, 
count);
  82 Double[] tmp = new Double[count];
  83 int i = 0;
  84 while(tdi.hasNext()) {
  85 tmp[i] = tdi.next();
  86 i++;
  87 }
  88 return Stream.of(tmp);
  89 }

can be replaced with a one liner:

  DoubleStream.iterate(base, e -> increment).limit(count).boxed();

You can abstract the creation of DoubleStream with a Suppiler:

  // Factory for double a stream of [base, increment, ..., increment] limited 
to a size of count
  Supplier ds = () -> DoubleStream.iterate(base, e -> 
increment).limit(count);

so you don't have to keep repeating the passing of the base/increment/count 
arguments each time:

  failures += compareUlpDifference(expectedSum, ds.get().sum(), 3);
  failures += compareUlpDifference(expectedSum, ds.get().average() 
getAsDouble(), 3);

  double collectorSum = 
ds.get().boxed().collect(Collectors.summingDouble(Function.identity());
  ...
  double collectorAvg = ds.get().boxed().collect(Collectors. 
averagingDouble(Function.identity()));
 
--

You can even abstract out the compareUlpDifference for the expected and 
threshold:

  Function comp = d -> compareUlpDifference(expected, d, 
threshold);
  ..
  failures += comp.apply(ds.get().boxed().collect(Collectors. 
averagingDouble(Function.identity(

Anyway... not suggesting you do that, just wanted to point out a simple partial 
function trick that i have found useful that can, when used appropriately, be 
an effective way to reduce code boilerplate.

--

My preference is to use TestNG and have s

Re: Initial request for review of JDK-8006572 DoubleStream.sum()/average() implementations that reduce numerical errors

[Joe Darcy]

Hi Paul,

On 11/18/2013 07:38 AM, Paul Sandoz wrote:

Hi Joe,

You can use the three arg form of collect for DoublePipeline.sum/average impls, 
which is already used for average:

 public final OptionalDouble average() {
 double[] avg = collect(() -> new double[2],
(ll, i) -> {
ll[0]++;
ll[1] += i;
},
(ll, rr) -> {
ll[0] += rr[0];
ll[1] += rr[1];
});
 return avg[0] > 0
? OptionalDouble.of(avg[1] / avg[0])
: OptionalDouble.empty();
 }

That would be the most expedient way.


Thanks for the tip. For the moment, I'm feeling a bit expedient and used 
the array-based approach in an iteration of the change:


http://cr.openjdk.java.net/~darcy/8006572.3/

This allows one of the kernels of the compensated summation logic to be 
shared in two of the use-sites.


For the test, all 6 scenarios fail on a JDK *without* the compensated 
summation code and all 6 pass with it; all the other java/util/streams 
regression tests pass with the new code too.


Off-list, I've asked a numerically inclined colleague to look over the 
summation logic and how the compensated summation states are combined.


Thanks,

-Joe


  However, for sum() it is somewhat unfortunate to have to create a double[1] 
box for each leaf. If you are willing to write a little more more code you can 
create your own double sum ReducingSink implementation as Brian suggests.


Testing wise you don't need to implement an Iterator, you can do the following:

   DoubleStream.iterate(base, e -> increment).limit(count)

It might be more convenient to test as follows:
  
   Stream s = Stream.iterate(false, e -> true).limit(count); // [*]

   DoubleSummaryStatistics stats = s.collect(Collectors.summarizingDouble(e -> 
e ? increment : base)); // Cross fingers that compiles!

   Stream s = Stream.iterate(false, e -> true).limit(count);
   Double d = s.iterate(false, e -> 
true).limit(count)..collect(Collectors.summingDouble(e ? increment : base));

Thereby covering both Collector implementations.

I guess it might be harder to test the combining step using parallel streams 
since combining will be platform dependent (depends on #cores) unless you track 
how things are combined. Perhaps the Collector instance could be tested 
directly with combination?

Paul.

[*] Another way is to use stream concatenation:

   Stream.concat(Stream.of(false), IntStream.range(1, count).mapToObj(e -> 
true))
   Stream.concat(Stream.of(false), Stream.generate(() -> true)).limit(count)


On Nov 14, 2013, at 9:08 AM, Joe Darcy  wrote:


Hello,

Please take an initial look over a fix for

JDK-8006572 DoubleStream.sum() & DoubleSummaryStats implementations that 
reduce numerical errors
http://cr.openjdk.java.net/~darcy/8006572.0/

The basic approach is to use compensated summation

http://en.wikipedia.org/wiki/Kahan_summation_algorithm

to computed streams-related sum and average statistics in the various locations 
that this can be done.

All existing streams tests pass and new newly-written test passes too.

I believe the DoubleSummaryStatistics.java portion, including the test, is 
fully review-worthy. In the test, for the sample computation in question, the 
naive summation implementation had a error of 500,000 ulps compared to 2 ups 
with the new implementation.

Two other locations I've found where this summation technique should be used 
are in

java.util.stream.Collectors.{summingDouble, averagingDouble}

and

java.util.stream.DoublePipeline.{sum, average}

DoublePipeline is the primary implementation class of DoubleStream.

For Collectors, the proposed code is a fairly clear adaptation of how the 
current code passes state around; there is not currently a dedicated test for 
the new summation technique in this location.

I'm new to the streams API so for DoublePipeline I don't know the idiomatic way 
to phrase the collect I want to perform over the code. (Based on my current 
understanding, I believe I want to perform a collect rather than a reduce since 
for the compensated summation I need to maintain some additional state.) 
Guidance here welcome.

Thanks,

-Joe