date:20131219

On 20/12/2013 12:57 PM, srikalyan chandrashekar wrote:

The ReferenceHandler thread can only get OOME if it allocates (directly
or indirectly) - so there has to be something in the unguarded part that
causes this. Again it may be an implicit action in the VM - similar to
the class load issue for InterruptedException.

David

I am still

unsure about the side effects of the code change and agree with your
thoughts(on memory exhaustion test's reliability).

PS: hotspot dev alias removed from CC.

--
Thanks
kalyan

On 12/19/13 5:08 PM, David Holmes wrote:

Hi Kalyan,

This is not a hotspot issue so I'm moving this to core-libs, please
drop hotspot from any replies.

On 20/12/2013 6:26 AM, srikalyan wrote:

Hi all, I have been working on the bug JDK-8022321
, this is a sporadic
failure and the webrev is available here
http://cr.openjdk.java.net/~srikchan/Regression/JDK-8022321_OOMEInReferenceHandler-webrev/

I'm really not sure what to make of this. We have a test that triggers
an out-of-memory condition but the OOME can actually turn up in the
ReferenceHandler thread causing it to terminate and the test to fail.
We previously accounted for the non-obvious occurrences of OOME due to
the Object.wait and the possible need to load the InterruptedException
class - but still the OOME can appear where we don't want it. So
finally you have just placed the whole for(;;) loop in a
try/catch(OOME) that ignores the OOME. I'm certain that makes the test
happy, but I'm not sure it is really what we want for the
ReferenceHandler thread. If the OOME occurs while cleaning, or
enqueuing then we will fail to clean and/or enqueue but there would be
no indication that has occurred and I think that is a bigger problem
than this test failing.

There may be no way to make this test 100% reliable. In fact I'd
suggest that no memory exhaustion test can be 100% reliable.

David

*
**"Root Cause:Still not known"*
2 places where there is a possibility for OOME
1) Cleaner.clean()
2) ReferenceQueue.enqueue()

1) The cleanup code in turn has 2 places where there is potential for
throwing OOME,
a) thunk Thread which is run from clean() method. This Runnable is
passed to Cleaner and appears in the following classes
java/nio/DirectByteBuffer.java
sun/misc/Perf.java
sun/nio/fs/NativeBuffer.java
sun/nio/ch/IOVecWrapper.java
sun/misc/Cleaner/ExitOnThrow.java
However none of the above overridden implementations ever create an
object in the clean() code.
b) new PrivilegedAction created in try catch Exception block of
clean() method but for this object to be created and to be held
responsible for OOME an Exception(other than OOME) has to be thrown.

2) No new heap objects are created in the enqueue method nor anywhere in
the deep call stack (VM.addFinalRefCount() etc) so this cannot be a
potential cause.

*Experimental change to java.lang.Reference.java* :
- Put one more guard (try catch with OOME block) in the Reference
Handler Thread which may give the Reference Handler a chance to cleanup.
This is fixing the test failure (several 1000 runs with 0 failures)
- Without the above change the test fails atleast 3-5 times for every
1000 run.

*PS*: The code change is to a very critical part of JDK and i am fully
not aware of the consequences of the change, hence seeking expert help
here. Appreciate your time and inputs towards this.

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

2013-12-19 Thread Mandy Chung


Hi Srikalyan,

Maybe you can get add an uncaught handler to see if you can get
any information.  I ran it for 1000 times but not able to duplicate
the failure.  Did you run it with jtreg (I didn't)?

Below is the patch to install a thread's uncaught handler that
you can take and try.

diff --git a/test/java/lang/ref/OOMEInReferenceHandler.java 
b/test/java/lang/ref/OOMEInReferenceHand
ler.java
--- a/test/java/lang/ref/OOMEInReferenceHandler.java
+++ b/test/java/lang/ref/OOMEInReferenceHandler.java
@@ -51,6 +51,14 @@
  return first;
  }

+ static class UEH implements Thread.UncaughtExceptionHandler {
+ public void uncaughtException(Thread t, Throwable e) {
+ System.err.println("ERROR: " + t.getName() + " exception " +
+ e.getMessage());
+ e.printStackTrace();
+ }
+ }
+
  public static void main(String[] args) throws Exception {
  // preinitialize the InterruptedException class so that the reference 
handler
  // does not die due to OOME when loading the class if it is the first 
use
@@ -77,6 +85,8 @@
  throw new IllegalStateException("Couldn't find Reference Handler 
thread.");
  }

+ referenceHandlerThread.setUncaughtExceptionHandler(new UEH());
+
  ReferenceQueue refQueue = new ReferenceQueue<>();
  Object referent = new Object();
  WeakReference weakRef = new WeakReference<>(referent, 
refQueue);

On 12/19/2013 6:57 PM, srikalyan chandrashekar wrote:
Hi David Thanks for your comments, the unguarded part(clean and 
enqueue) in the Reference Handler thread does not seem to create any 
new objects, so it is the application(the test in this case) which is 
adding objects to heap and causing the Reference Handler to die with 
OOME. I am still unsure about the side effects of the code change and 
agree with your thoughts(on memory exhaustion test's reliability).


PS: hotspot dev alias removed from CC.

--
Thanks
kalyan

On 12/19/13 5:08 PM, David Holmes wrote:

Hi Kalyan,

This is not a hotspot issue so I'm moving this to core-libs, please 
drop hotspot from any replies.


On 20/12/2013 6:26 AM, srikalyan wrote:

Hi all,  I have been working on the bug JDK-8022321
 , this is a sporadic
failure and the webrev is available here
http://cr.openjdk.java.net/~srikchan/Regression/JDK-8022321_OOMEInReferenceHandler-webrev/ 



I'm really not sure what to make of this. We have a test that 
triggers an out-of-memory condition but the OOME can actually turn up 
in the ReferenceHandler thread causing it to terminate and the test 
to fail. We previously accounted for the non-obvious occurrences of 
OOME due to the Object.wait and the possible need to load the 
InterruptedException class - but still the OOME can appear where we 
don't want it. So finally you have just placed the whole for(;;) loop 
in a try/catch(OOME) that ignores the OOME. I'm certain that makes 
the test happy, but I'm not sure it is really what we want for the 
ReferenceHandler thread. If the OOME occurs while cleaning, or 
enqueuing then we will fail to clean and/or enqueue but there would 
be no indication that has occurred and I think that is a bigger 
problem than this test failing.


There may be no way to make this test 100% reliable. In fact I'd 
suggest that no memory exhaustion test can be 100% reliable.


David


*
**"Root Cause:Still not known"*
2 places where there is a possibility for OOME
1) Cleaner.clean()
2) ReferenceQueue.enqueue()

1)  The cleanup code in turn has 2 places where there is potential for
throwing OOME,
 a) thunk Thread which is run from clean() method. This Runnable is
passed to Cleaner and appears in the following classes
 java/nio/DirectByteBuffer.java
 sun/misc/Perf.java
 sun/nio/fs/NativeBuffer.java
 sun/nio/ch/IOVecWrapper.java
 sun/misc/Cleaner/ExitOnThrow.java
However none of the above overridden implementations ever create an
object in the clean() code.
 b) new PrivilegedAction created in try catch Exception block of
clean() method but for this object to be created and to be held
responsible for OOME an Exception(other than OOME) has to be thrown.

2) No new heap objects are created in the enqueue method nor 
anywhere in

the deep call stack (VM.addFinalRefCount() etc) so this cannot be a
potential cause.

*Experimental change to java.lang.Reference.java* :
- Put one more guard (try catch with OOME block) in the Reference
Handler Thread which may give the Reference Handler a chance to 
cleanup.

This is fixing the test failure (several 1000 runs with 0 failures)
- Without the above change the test fails atleast 3-5 times for every
1000 run.

*PS*: The code change is to a very critical part of JDK and i am fully
not aware of the consequences of the change, hence seeking expert help
here. Appreciate your time and inputs towards this.

Re: RFR for JDK-8030284 TEST_BUG: intermittent StackOverflow in RMI bench/serial test


Hi Stuart,

On 20/12/2013 1:06 PM, Stuart Marks wrote:

On 12/18/13 10:25 PM, Tristan Yan wrote:

Hi Everyone

Please help to review the fix for JDK-8030284.

http://cr.openjdk.java.net/~tyan/JDK-8030284/webrev.00/


This is a one line fix that add -Xss to prevent StackOverflowError.


Hi, I guess this might make sense, but this still seems like a mystery
to me.

Do we have any evidence that this test hit the stack limit but otherwise
is behaving identically? It does load 50 classes recursively. It seems
strange that this test apparently ran for years without problems as a
shell test, but when run in a jtreg environment, adding the additional
six or so stack frames for jtreg would have pushed it over the limit.


If you were always one frame from the end then it is not so surprising 
that a simple change pushes you past the limit :) Try running the shell 
test with additional recursive loads and see when it fails.



It's also kind of strange that in the two stack traces I've seen (I
think I managed to capture only one in the bug report though) the
StackOverflowError occurs on loading exactly the 50th class. Since we're
observing intermittent behavior (happens sometimes but not others) the
stack size is apparently variable. Since it's variable I'd expect to see
it failing at different times, possibly the 49th or 48th recursive
classload, not just the 50th. And in such circumstances, do we know what
the default stack size is?


Classloading consumes a reasonable chunk of stack so if the variance 
elsewhere is quite small it is not that surprising that the test always 
fails on the 50th class. I would not expect run-to-run stack usage 
variance to be high unless there is some random component to the test.



I don't know if you were able to reproduce this issue. If you were, it
would be good to understand in more detail exactly what's going on.


FWIW there was a recent change in 7u to bump up the number of stack 
shadow pages in hotspot as "suddenly" StackOverflow tests were crashing 
instead of triggering StackOverflowError. So something started using 
more stack in a way the caused there to not be enough space to process a 
stackoverflow properly. Finding the exact cause can be somewhat tedious.


Cheers,
David


s'marks

Re: RFR for JDK-6963118 Intermittent test failure: test/java/nio/channels/Selector/Wakeup.java fail intermittently (win)


This should probably be reviewed by the nio-dev folk.

Increasing the timeouts seems okay but how confident are you that this 
is sufficient for a wide range of platforms. In other tests we often see 
timeouts of a few seconds get extended even further, so three seconds is 
not so big.


Also the yield loops:

while (sleeper.entries < 5)
Thread.yield();

would be better as sleep loops (as used elsewhere) to avoid potential 
issues with yield being a no-op on some platforms (Yes I see it is 
already used that way elsewhere in the test but the sleep is better :) ).


Thanks,
David

On 3/12/2013 12:39 PM, srikalyan chandrashekar wrote:

Hi all, I am working on bug JDK-6963118
 .
Root Cause:
- Sensitive timing dependency between events in Main and Sleeper threads
are causes for test failure.

Suggested Fix:
   1) Main thread should wait for more than 1sec(made it 3sec) and check
more often than 50ms(made it 1ms) intervals , sleeper thread may be
still waiting for interrupt/wakeup hence main thread waiting for just
1sec to flag a failure is premature . The reason is especially on
windows high priority virus scanners etc run(we faced it when simulating
failures) and kept the system busy.
   2) The test is essentially a sequence of 2 events
   a)Firing up wakeups/interrupts followed by a
   b)Check
  Check the sleeper.entries value and yield the main thread as required
so that the above 2 events step in tandem.

The webrev is hosted at
http://cr.openjdk.java.net/~cl/host_for_kal/6963118-Wakeup/ .
Please let me know if you have any comments or suggestions.

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

Sorry Kalyan but I don't see the need for all the incidental changes if 
the primary change is to just increase the iterations. I also don't see 
why you need to do anything for BrokenBarrierException as it is not 
expected to happen and the test should just fail if it does.


David

On 10/12/2013 6:15 AM, srikalyan wrote:

Hi David/Martin a gentle reminder for review.

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


On 12/2/13, 11:21 AM, srikalyan wrote:

Hi David, Thanks for the review, the new webrev is hosted at
http://cr.openjdk.java.net/~cl/host_for_kal/6772009-CancelledLockLoop/
. Please see inline text.

On 11/20/13, 6:35 PM, David Holmes wrote:

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.

Made the necessary changes for atomic update.


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

I retained it in the fix for completeness in updating the numbers,
please let me know if you still think otherwise.



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.

Fixed the spelling error and styling issues.


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

Removed the claim :)


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

Retained the tracking to understand if it is again the timing issue
which is the cause in an event of a failure, however i can remove it
if you think it is not necessary (OR) include an alternate solution as
you may want to suggest.


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?

Here the time out is not about waiting for threads to complete
something but to "be interrupted" before being considered done, so we
decreased the timeout. However we now chose to increase the number of
iterations to 500 from 100(thanks to tristan for the
suggestion) instead of decreasing the timeout as done earlier because
the increasing iterations ensures the threads are busy for long time
curtailing the need to touch the timeout.



Thanks,
David


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







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 for JDK-7168267: TEST_BUG: Cleanup of rmi regression tests (activation and others)

2013-12-19 Thread Tristan Yan


Thanks Stuart
I changed ReadTimeoutTest.java only apply CountdownLatch part. Please 
review.


http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.02/

Thank you
Tristan

On 12/20/2013 10:47 AM, Stuart Marks wrote:

Hi Tristan,

Changes mostly look good.

There is an ignored InterruptedException in both versions of 
UseCustomSocketFactory.java, but that was there already; it's not 
clear what should be done in this case anyway. This is something to 
keep in mind for a future cleanup. Hm, some duplicate code here as 
well, again something to think about for the future.


There is a serious problem with the change to ReadTimeoutTest.java, 
however. The change removes the (old) line 72, which is

 TestIface stub = impl.export();
probably because there was an IDE warning that the variable "stub" is 
unused. This much is true, but it's essential for impl.export() to be 
called, because that exports the object, which creates a socket using 
the socket factory, which eventually results in the fac.whichPort() 
call below returning the port that was open. In the absence of the 
export() call, whichPort() returns zero which causes the test to abort 
immediately.


In addition, the refactoring to use try-with-resources changes the 
order of execution of certain code, and it changes the scope of things 
handled by the finally-block.


One purpose of the finally-block is to unexport the remote object so 
it makes sense to begin the try-block immediately following the 
export. The original code did this (well, only after a benign local 
variable declaration). The change moves the try-block few lines down, 
which means there is a larger window of time within which the 
finally-block won't be executed. This isn't obviously a problem, but 
it's a change nonetheless.


Also, the change alters the order of opening the client socket and the 
"connecting to listening port" message, so the message comes after the 
port is opened, instead of before. Again, an apparently small change, 
but if there's an exception opening the port, the port number being 
opened won't be printed out.


The main point of the changes to this file, however, is good, which is 
to replace the unsafe use of multi-thread access to a boolean array 
and polling of that value, with a CountDownLatch. So that part of the 
change should go in. The problem is the apparently innocuous code 
cleanups (use of try-with-resources, removal of apparently unused 
local variable) which cause the test to break or otherwise change its 
behavior.


I could go ahead and push this changeset, omitting the changes to 
ReadTimeoutTest.java. Or, you could update the changeset to revert all 
of the changes to ReadTimeoutTest.java except for those necessary to 
implement the use of CountDownLatch. Either way is fine with me.


Which would you prefer?

s'marks


On 12/18/13 6:51 AM, Tristan Yan wrote:

Hi Everyone
Please review the code fix for bug JDK-7168267

http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.01/ 



This is a cleanup for RMI tests. trying to use real timeout to 
replace a fixed number of loop.

Thank you

Tristan


On 12/12/2013 05:33 AM, Stuart Marks wrote:

On 12/10/13 6:10 PM, Tristan Yan wrote:

/Hi everyone
I am working on bug JDK-7168267



Correct link is

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


Root Cause:
- Per Stuart's comment, this is a clean up bug.

Suggested Fix:
- Will use timeout to replace loop.


We should probably look at specific cases for this. There are places 
where the test is waiting for some external service to become ready 
(e.g., rmiregistry). There's no notification for things like this so 
wait-with-timeout cannot be used. Pretty much the only thing that 
can be done is to poll reasonably often until the service is ready, 
or until the timeout is exceeded.



- Also I am fixing two test's performance
java/rmi/activation/Activatable/forceLogSnapshot - method 
waitAllStarted is

using sleep to poll 50 restartedObject to be true, we can use modern
CountDownLatch to implement blocking-time wait.
java/rmi/activation/Activatable/checkAnnotations - We can subclass
ByteArrayOutputStream which support notification when data was 
written. Also use

two thread wait output string and error string to be not null.


These sound reasonble. Go ahead and file sub-tasks for these and 
then choose one to work on first. (I think it will get too confusing 
if we try to work on them all simultaneously.) Either post a 
detailed description of what you intend to do, or if it's simple 
enough, just post a webrev.


s'marks



Please let me know if you have any comments or suggestions.
/ /
Thank you
Tristan

On 12/05/2013 09:02 AM, Stuart Marks wrote:
/

/On 12/3/13 11:05 PM, Tristan Yan wrote:
/
/I am working on 
https://bugs.openjdk.java.net/browse/JDK-7168267. This bug is
asking performance improvement for RMI test. Because this would 
involve
different RMI tests. I’d like to use t

Re: RFR for JDK-8030284 TEST_BUG: intermittent StackOverflow in RMI bench/serial test

2013-12-19 Thread Stuart Marks


On 12/18/13 10:25 PM, Tristan Yan wrote:

Hi Everyone

Please help to review the fix for JDK-8030284.

http://cr.openjdk.java.net/~tyan/JDK-8030284/webrev.00/


This is a one line fix that add -Xss to prevent StackOverflowError.


Hi, I guess this might make sense, but this still seems like a mystery to me.

Do we have any evidence that this test hit the stack limit but otherwise is 
behaving identically? It does load 50 classes recursively. It seems strange that 
this test apparently ran for years without problems as a shell test, but when 
run in a jtreg environment, adding the additional six or so stack frames for 
jtreg would have pushed it over the limit.


It's also kind of strange that in the two stack traces I've seen (I think I 
managed to capture only one in the bug report though) the StackOverflowError 
occurs on loading exactly the 50th class. Since we're observing intermittent 
behavior (happens sometimes but not others) the stack size is apparently 
variable. Since it's variable I'd expect to see it failing at different times, 
possibly the 49th or 48th recursive classload, not just the 50th. And in such 
circumstances, do we know what the default stack size is?


I don't know if you were able to reproduce this issue. If you were, it would be 
good to understand in more detail exactly what's going on.


s'marks

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

2013-12-19 Thread srikalyan chandrashekar

Hi David Thanks for your comments, the unguarded part(clean and enqueue)
in the Reference Handler thread does not seem to create any new objects,
so it is the application(the test in this case) which is adding objects
to heap and causing the Reference Handler to die with OOME. I am still
unsure about the side effects of the code change and agree with your
thoughts(on memory exhaustion test's reliability).

PS: hotspot dev alias removed from CC.

--
Thanks
kalyan

On 12/19/13 5:08 PM, David Holmes wrote:

Hi Kalyan,

This is not a hotspot issue so I'm moving this to core-libs, please
drop hotspot from any replies.

On 20/12/2013 6:26 AM, srikalyan wrote:

There may be no way to make this test 100% reliable. In fact I'd
suggest that no memory exhaustion test can be 100% reliable.

David

*
**"Root Cause:Still not known"*
2 places where there is a possibility for OOME
1) Cleaner.clean()
2) ReferenceQueue.enqueue()

2) No new heap objects are created in the enqueue method nor anywhere in
the deep call stack (VM.addFinalRefCount() etc) so this cannot be a
potential cause.

*PS*: The code change is to a very critical part of JDK and i am fully
not aware of the consequences of the change, hence seeking expert help
here. Appreciate your time and inputs towards this.

Re: RFR for JDK-7168267: TEST_BUG: Cleanup of rmi regression tests (activation and others)

2013-12-19 Thread Stuart Marks


Hi Tristan,

Changes mostly look good.

There is an ignored InterruptedException in both versions of 
UseCustomSocketFactory.java, but that was there already; it's not clear what 
should be done in this case anyway. This is something to keep in mind for a 
future cleanup. Hm, some duplicate code here as well, again something to think 
about for the future.


There is a serious problem with the change to ReadTimeoutTest.java, however. The 
change removes the (old) line 72, which is


TestIface stub = impl.export();

probably because there was an IDE warning that the variable "stub" is unused. 
This much is true, but it's essential for impl.export() to be called, because 
that exports the object, which creates a socket using the socket factory, which 
eventually results in the fac.whichPort() call below returning the port that was 
open. In the absence of the export() call, whichPort() returns zero which causes 
the test to abort immediately.


In addition, the refactoring to use try-with-resources changes the order of 
execution of certain code, and it changes the scope of things handled by the 
finally-block.


One purpose of the finally-block is to unexport the remote object so it makes 
sense to begin the try-block immediately following the export. The original code 
did this (well, only after a benign local variable declaration). The change 
moves the try-block few lines down, which means there is a larger window of time 
within which the finally-block won't be executed. This isn't obviously a 
problem, but it's a change nonetheless.


Also, the change alters the order of opening the client socket and the 
"connecting to listening port" message, so the message comes after the port is 
opened, instead of before. Again, an apparently small change, but if there's an 
exception opening the port, the port number being opened won't be printed out.


The main point of the changes to this file, however, is good, which is to 
replace the unsafe use of multi-thread access to a boolean array and polling of 
that value, with a CountDownLatch. So that part of the change should go in. The 
problem is the apparently innocuous code cleanups (use of try-with-resources, 
removal of apparently unused local variable) which cause the test to break or 
otherwise change its behavior.


I could go ahead and push this changeset, omitting the changes to 
ReadTimeoutTest.java. Or, you could update the changeset to revert all of the 
changes to ReadTimeoutTest.java except for those necessary to implement the use 
of CountDownLatch. Either way is fine with me.


Which would you prefer?

s'marks


On 12/18/13 6:51 AM, Tristan Yan wrote:

Hi Everyone
Please review the code fix for bug JDK-7168267

http://cr.openjdk.java.net/~tyan/JDK-7168267/webrev.01/ 



This is a cleanup for RMI tests. trying to use real timeout to replace a fixed 
number of loop.

Thank you

Tristan


On 12/12/2013 05:33 AM, Stuart Marks wrote:

On 12/10/13 6:10 PM, Tristan Yan wrote:

/Hi everyone
I am working on bug JDK-7168267



Correct link is

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


Root Cause:
- Per Stuart's comment, this is a clean up bug.

Suggested Fix:
- Will use timeout to replace loop.


We should probably look at specific cases for this. There are places where 
the test is waiting for some external service to become ready (e.g., 
rmiregistry). There's no notification for things like this so 
wait-with-timeout cannot be used. Pretty much the only thing that can be done 
is to poll reasonably often until the service is ready, or until the timeout 
is exceeded.



- Also I am fixing two test's performance
java/rmi/activation/Activatable/forceLogSnapshot - method waitAllStarted is
using sleep to poll 50 restartedObject to be true, we can use modern
CountDownLatch to implement blocking-time wait.
java/rmi/activation/Activatable/checkAnnotations - We can subclass
ByteArrayOutputStream which support notification when data was written. Also 
use

two thread wait output string and error string to be not null.


These sound reasonble. Go ahead and file sub-tasks for these and then choose 
one to work on first. (I think it will get too confusing if we try to work on 
them all simultaneously.) Either post a detailed description of what you 
intend to do, or if it's simple enough, just post a webrev.


s'marks



Please let me know if you have any comments or suggestions.
/ /
Thank you
Tristan

On 12/05/2013 09:02 AM, Stuart Marks wrote:
/

/On 12/3/13 11:05 PM, Tristan Yan wrote:
/
/I am working on https://bugs.openjdk.java.net/browse/JDK-7168267. This 
bug is

asking performance improvement for RMI test. Because this would involve
different RMI tests. I’d like to use this cr as an umbrella bug, create 
sub-cr
for different test. Then I can make progress on sub-cr. Please let me know 
your

opinion on this.
/

/
Actually JDK-7168267 is more about various test cleanups, and JDK-8005436 is

Re: RFR [6968459] JNDI timeout fails before timeout is reached

If you track the elapsed waiting time using System.nanoTime you do not 
need to be concerned whether anyone messes with the TOD clock.


David

On 4/12/2013 2:05 AM, Peter Levart wrote:

On 12/03/2013 03:35 PM, Ivan Gerasimov wrote:

Hi Peter!

Thank you for your review!

You are right, the patch changed the behavior of the code.
I've reverted back all the unnecessary changes. This should minimize
the risk.

I've also made another correction: After decrementing the remaining
timeOut, the startTime should be set to currTime.

Would you please help review the updated webrev:
http://cr.openjdk.java.net/~igerasim/6968459/2/webrev/

Sincerely yours,
Ivan


Hi Ivan,

That's better. You could move the initial request for ldr.getReplyBer()
(line 447) out of while loop, since it is not needed in the loop (all
further ldr.getReplyBer() calls in loop are performed in synchronized
block already - line 465). "else { break; }" (line 469) is not needed in
that case. I would also make timeOut variable long to avoid overflows.
Simplified logic could look like this:


 BerDecoder readReply(LdapRequest ldr)
 throws IOException, NamingException {
 long timeOut = (readTimeout > 0) ?
 readTimeout : 15 * 1000; // Default read timeout of 15 sec

 long currTime = System.currentTimeMillis();

 BerDecoder rber = ldr.getReplyBer();

 while (rber == null && timeOut > 0L) {
 long startTime = currTime;
 // If socket closed, don't even try
 synchronized (this) {
 if (sock == null) {
 throw new ServiceUnavailableException(host + ":" +
port +
"; socket closed");
 }
 }
 synchronized (ldr) {
 // check if condition has changed since our last check
 rber = ldr.getReplyBer();
 if (rber == null) {
 try {
 ldr.wait(timeOut);
 } catch (InterruptedException ex) {
 throw new InterruptedNamingException(
 "Interrupted during LDAP operation");
 }
 }
 }
 currTime = System.currentTimeMillis();
 if (startTime < currTime) {
 timeOut -= (currTime - startTime);
 }
 // else system time must have changed backwards (or not at
all)
 }

 if (rber == null && readTimeout > 0) {
 removeRequest(ldr);
 throw new NamingException("LDAP response read timed out,
timeout used:"
 + readTimeout + "ms." );
 }

 return rber;
 }


What do you think?

Regards, Peter





From quick look I notice a danger that Connection.readReply() pauses
(for the timeOut time) in spite of the fact that a reply is ready and
enqueued before waiting.

Imagine a situation where the 1st try of obtaining result returns null:

 450 // Get the reply if it is already available.
 451 BerDecoder rber = ldr.getReplyBer();


...after that, but before reaching the following lines:

 471 synchronized (ldr) {
 472 ldr.wait(timeOut);
 473 }


The "other" thread enqueues a reply (LdapRequest.addReplyBer())
because the code in readReply() is executing without holding a lock
on "ldr". When "this" thread (executing readReply()) finally enters
synchronized block and waits, it will wait until wait() times out or
another reply is enqueued which will issue another notify(). This can
lead to unnecessary pauses. Old code does not exhibit this problem,
because it re-checks the readiness of a reply immediately after
entering the synchronized block.


Regards, Peter

Re: Analysis on JDK-8022321 java/lang/ref/OOMEInReferenceHandler.java fails intermittently

Hi Kalyan,

This is not a hotspot issue so I'm moving this to core-libs, please drop
hotspot from any replies.

On 20/12/2013 6:26 AM, srikalyan wrote:

I'm really not sure what to make of this. We have a test that triggers
an out-of-memory condition but the OOME can actually turn up in the
ReferenceHandler thread causing it to terminate and the test to fail. We
previously accounted for the non-obvious occurrences of OOME due to the
Object.wait and the possible need to load the InterruptedException class
- but still the OOME can appear where we don't want it. So finally you
have just placed the whole for(;;) loop in a try/catch(OOME) that
ignores the OOME. I'm certain that makes the test happy, but I'm not
sure it is really what we want for the ReferenceHandler thread. If the
OOME occurs while cleaning, or enqueuing then we will fail to clean
and/or enqueue but there would be no indication that has occurred and I
think that is a bigger problem than this test failing.

There may be no way to make this test 100% reliable. In fact I'd suggest
that no memory exhaustion test can be 100% reliable.

David

*
**"Root Cause:Still not known"*
2 places where there is a possibility for OOME
1) Cleaner.clean()
2) ReferenceQueue.enqueue()

2) No new heap objects are created in the enqueue method nor anywhere in
the deep call stack (VM.addFinalRefCount() etc) so this cannot be a
potential cause.

*PS*: The code change is to a very critical part of JDK and i am fully
not aware of the consequences of the change, hence seeking expert help
here. Appreciate your time and inputs towards this.

Re: Theoretical data race on java.util.logging.Handler.sealed

2013-12-19 Thread Mandy Chung


On 12/19/13 7:49 AM, Peter Levart wrote:

Hi Mandy, Daniel,

I didn't like the package-protected getters either. So here's another 
variant that replaces Handler.configure() method with a 
package-protected constructor which is chained from JDK subclasses:


http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/



Looks good.  Thanks for making the change and the new test.  It'd be 
good to close the handlers by the test. The test is running in othervm 
mode and the Cleaner thread will close the handler when VM exits and the 
test is fine as it is.


Digress:  Just notice that the closeable handler classes are not 
AutoCloseable (they don't implement Closeable either).  The close() 
method don't throw IOException but instead throws SecurityException an 
unchecked exception.  Otherwise, we could use try-with-resources.



I filed another bug that is fixed by this patch:

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

And I created a test (see webrev.07) that almost passes when run 
against unchanged JDK 8 (the failure is just at the end when calling 
new SocketHandler(host, port) - access denied 
("java.util.logging.LoggingPermission" "control")). If I comment-out 
the System.setSecurityManager() from the test, it passes with 
unchanged code. This is to verify the test itself. When run against 
the patched JDK 8, it passes even when SecurityManager is active - 
this verifies two things:
- the behaviour of patched code is analogous to unpatched code as far 
as defaults and configured handler properties is concerned and it 
conforms to javadoc
- the patched code does not require any new permissions - it actually 
requires less, because it fixes bug 8030801.




Yes I agree this is a bug that should get fixed.

All java/util/logging jtreg tests pass with patched code. I hope that 
"localhost" is a resolvable name on all environments and that new 
ServerSocket(0) creates a server socket bound at least to the IP 
address that "localhost" resolves to. Is this reasonable to assume?


"localhost" should be fine and there are other tests depending on it be 
resolvable.


thanks
Mandy

Re: Bug in Long.parseUnsignedLong

Here's a formalization of the suggested fix:

http://cr.openjdk.java.net/~bpb/8030814/webrev/

It works against the test case.

Brian

On Dec 19, 2013, at 11:26 AM, Brian Burkhalter wrote:

> Upon inspection only that indeed looks correct.
> 
> Thanks …
> 
> On Dec 19, 2013, at 10:28 AM, Louis Wasserman wrote:
> 
>> Here's one approach that works: there is overflow iff 
>> 
>> compareUnsigned(first, divideUnsigned(MAX_UNSIGNED, radix)) > 0 || (first == 
>> divideUnsigned(MAX_UNSIGNED, radix) && second > 
>> remainderUnsigned(MAX_UNSIGNED, radix));
>> 
>> Since radix <= Character.MAX_RADIX, you can precompute the divides and 
>> remainders in a small table.
>

Re: Bug in Long.parseUnsignedLong

Upon inspection only that indeed looks correct.

Thanks …

On Dec 19, 2013, at 10:28 AM, Louis Wasserman wrote:

> Here's one approach that works: there is overflow iff 
> 
> compareUnsigned(first, divideUnsigned(MAX_UNSIGNED, radix)) > 0 || (first == 
> divideUnsigned(MAX_UNSIGNED, radix) && second > 
> remainderUnsigned(MAX_UNSIGNED, radix));
> 
> Since radix <= Character.MAX_RADIX, you can precompute the divides and 
> remainders in a small table.

Re: RFR of lang level code migration patches

2013-12-19 Thread Mike Duigou

These look good to me.

On Dec 19 2013, at 06:51 , Paul Sandoz  wrote:

> Hi,
> 
> Here are some patches that migrate some code to use more up to date language 
> features. I will create a bug later on after feedback.
> 
> This is motivated from Brian's patches to lang tools.
> 
> Compress catches
> http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/

The comment in JarVerifier "// e.g. sun.security.pkcs.ParsingException" is now 
probably too broad to be useful. Maybe just remove it.

Re: RFR of lang level code migration patches

2013-12-19 Thread Alan Bateman


On 19/12/2013 14:51, Paul Sandoz wrote:

:

I ran all the j.u. tests locally and there were no regressions.

That's the main thing with changes like this.

I skimmed over the changes too and they look okay. I'm sometimes wary of 
IDEs re-arranging things but there's nothing here. It would be good to 
get a few other areas done too while things are quiet.


-Alan.

Re: RFR: java/util/logging/Logger/setResourceBundle/TestSetResourceBundle.java failing again


On 12/19/13 5:39 PM, Mandy Chung wrote:


On 12/19/2013 3:04 AM, Daniel Fuchs wrote:

On 12/18/13 9:35 PM, Mandy Chung wrote:


On 12/17/2013 3:57 AM, Daniel Fuchs wrote:

Hi,

Please find below a fix for what I believe is a test bug.
I plan to push this in JDK 9 dev.

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

This seems to be a very intermittent failure.
It looks as if a logger held in a local variable can be
arbitrarily garbage collected if that variable is no longer
used.
To prevent arbitrary garbage collection of such loggers, the fix
makes sure to use the variable again at the end of the test.



Looks like running the test in -Xcomp and also with jtreg creates
pressure to the young gen and more object allocation failure and so
unreachable objects get collected in the middle of a small method body
(like in this case).

You added code to reference the local variables referencing the
loggers.  Would it be simpler to have this test  just to hold a strong
reference to the logger?


Hmmm. I don't think it would be simpler. I could have added an
array list of loggers and cleared that at the end of the test.
It would just have been another way of making sure that the
loggers weren't gc'ed.


Are you concerned that a static/instance field holding the loggers gets
GC'ed if not referenced before the test method returns?   They should be
only collected when the class/instance becomes unreachable and it
shouldn't be an issue (as do in other tests).


Yes - that was part of my concern. Reusing the variable at the end
of the test makes it obviously visible that it can't be gc'ed before
that.


I'm fine with what you have.


OK thanks Mandy, I think I will push what I have then.
This is what I have tested - by running it for several hours in a
loop :-)

-- daniel



Mandy

Re: Bug in Long.parseUnsignedLong

On Dec 19, 2013, at 9:36 AM, Paul Sandoz wrote:

> I think the logic for overflow when using the compareUnsigned is incorrect in 
> Long:
> 
> […]
>long result = first * radix + second;
>if (compareUnsigned(result, first) < 0) {

I concur and verified that yesterday by replacing the above logic with 
BigInteger equivalents which passed the test.

> My brain is too clogged up with cold at the moment to propose a fix :-(

Hope you recover soon!

Thanks,

Brian

Re: Bug in Long.parseUnsignedLong

2013-12-19 Thread Paul Sandoz

Hi,

I think the logic for overflow when using the compareUnsigned is incorrect in 
Long:

long first = parseLong(s.substring(0, len - 1), radix);
int second = Character.digit(s.charAt(len - 1), radix);
if (second < 0) {
throw new NumberFormatException("Bad digit at end of " + s);
}
long result = first * radix + second;
if (compareUnsigned(result, first) < 0) {

Take for example a string representation consisting of 17 digits 
"12000"

The long that will be parsed is one char less "1200":

  first = 0x1200___
  second = 0;
  result = 0x2000___ // first << 4, result > first

i.e. overflow of the overflow if you consider 16 additions, rather than a 
multiply.

My brain is too clogged up with cold at the moment to propose a fix :-(

Paul.

On Dec 19, 2013, at 12:52 AM, Louis Wasserman  wrote:

> Derp.  Here is the test case:
> 
> import java.math.BigInteger;
> 
> public class UnsignedLongBug {
>  public static void main(String[] args) {
>try {
>  String input = "1234567890abcdef1";
>  System.out.println(Long.parseUnsignedLong(input, 16));
>  BigInteger maxUnsignedLong =
> BigInteger.ONE.shiftLeft(64).subtract(BigInteger.ONE);
>  BigInteger inputValue = new BigInteger(input, 16);
>  System.out.println(maxUnsignedLong.compareTo(inputValue));
>  throw new AssertionError();
>} catch (NumberFormatException expected) {
>  System.out.println("Correct");
>}
>  }
> }
> 
> 
> On Wed, Dec 18, 2013 at 3:51 PM, Louis Wasserman wrote:
> 
>> The Javadoc of Long.parseUnsignedLong specifies that it should throw a
>> NumberFormatException if "the value represented by the string is larger
>> than the largest unsigned long, 2^64-1."
>> 
>> This does not appear to be happening:
>> 
>> --
>> Louis Wasserman
>> 
> 
> 
> 
> -- 
> Louis Wasserman

Re: RFR of lang level code migration patches

2013-12-19 Thread Chris Hegarty

I looked over the patch files, and they look good to me.

Limiting each webrev to a single language feature makes reviewing quite 
straight forward.

-Chris.

On 19 Dec 2013, at 14:51, Paul Sandoz  wrote:

> Hi,
> 
> Here are some patches that migrate some code to use more up to date language 
> features. I will create a bug later on after feedback.
> 
> This is motivated from Brian's patches to lang tools.
> 
> I focused just on java.util, minus the concurrent packages, and i used the 
> IDE to assist in the code migration. It's easy to pick off other packages 
> over time. This makes for more of a low-brow effort [*], perfect when one has 
> a cold.
> 
> Use <> syntax:
> http://cr.openjdk.java.net/~psandoz/tl/j.u.diamond/webrev/
> 
> Replace for with for-each 
> http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach/webrev/
> 
> Replace while with for-each
> http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach-while/webrev/
> 
> Compress catches
> http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/
> 
> Use switch for strings; not sure this one is worth it
> http://cr.openjdk.java.net/~psandoz/tl/j.u.swtich/webrev/
> 
> I ran all the j.u. tests locally and there were no regressions. Yet to run a 
> JPRT job.
> 
> Paul.
> 
> [*] although one needs to be vigilant since sometimes the IDE can refactor 
> incorrectly and sometimes code is arranged in a certain way for a reason 
> (which one reason for leaving j.u.concurrent packages alone for now).

Re: RFR of lang level code migration patches

2013-12-19 Thread Brian Goetz

Paul deliberately stayed away from the JUC classes.  Can we get a 
definitive list of non-JUC classes that primarily live in the JSR166 CVS?


On 12/19/2013 11:48 AM, Martin Buchholz wrote:

(as always) Please don't modify jsr166 classes (ArrayDeque) here, since
they are maintained upstream in jsr166 CVS.  Since jsr166 targets a variety
of java runtimes in general, it tends to be a late adopter of new language
features.  Although it's probably time to start using jdk7 features.


On Thu, Dec 19, 2013 at 6:51 AM, Paul Sandoz  wrote:


Hi,

Here are some patches that migrate some code to use more up to date
language features. I will create a bug later on after feedback.

This is motivated from Brian's patches to lang tools.

I focused just on java.util, minus the concurrent packages, and i used the
IDE to assist in the code migration. It's easy to pick off other packages
over time. This makes for more of a low-brow effort [*], perfect when one
has a cold.

Use <> syntax:
http://cr.openjdk.java.net/~psandoz/tl/j.u.diamond/webrev/

Replace for with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach/webrev/

Replace while with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach-while/webrev/

Compress catches
http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/

Use switch for strings; not sure this one is worth it
http://cr.openjdk.java.net/~psandoz/tl/j.u.swtich/webrev/

I ran all the j.u. tests locally and there were no regressions. Yet to run
a JPRT job.

Paul.

[*] although one needs to be vigilant since sometimes the IDE can refactor
incorrectly and sometimes code is arranged in a certain way for a reason
(which one reason for leaving j.u.concurrent packages alone for now).

Re: Bug in Long.parseUnsignedLong

Thanks, you saved me the trouble. I already verified it yesterday myself.

Brian

On Dec 19, 2013, at 8:26 AM, roger riggs wrote:

> Created JDK-8030814  to 
> track this issue.

Re: RFR of lang level code migration patches

2013-12-19 Thread Brian Goetz


+1 on all changes, except perhaps for this one in Collections.copy:

 ListIterator di=dest.listIterator();
  ListIterator si=src.listIterator();
  for (int i=0; iThe code is bizarre enough (calling next based on a range loop rather 
than hasNext()) to leave alone.


On 12/19/2013 9:51 AM, Paul Sandoz wrote:

Hi,

Here are some patches that migrate some code to use more up to date language 
features. I will create a bug later on after feedback.

This is motivated from Brian's patches to lang tools.

I focused just on java.util, minus the concurrent packages, and i used the IDE 
to assist in the code migration. It's easy to pick off other packages over 
time. This makes for more of a low-brow effort [*], perfect when one has a cold.

Use <> syntax:
http://cr.openjdk.java.net/~psandoz/tl/j.u.diamond/webrev/

Replace for with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach/webrev/

Replace while with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach-while/webrev/

Compress catches
http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/

Use switch for strings; not sure this one is worth it
http://cr.openjdk.java.net/~psandoz/tl/j.u.swtich/webrev/

I ran all the j.u. tests locally and there were no regressions. Yet to run a 
JPRT job.

Paul.

[*] although one needs to be vigilant since sometimes the IDE can refactor 
incorrectly and sometimes code is arranged in a certain way for a reason (which 
one reason for leaving j.u.concurrent packages alone for now).

Re: RFR: java/util/logging/Logger/setResourceBundle/TestSetResourceBundle.java failing again

2013-12-19 Thread Mandy Chung



On 12/19/2013 3:04 AM, Daniel Fuchs wrote:

On 12/18/13 9:35 PM, Mandy Chung wrote:


On 12/17/2013 3:57 AM, Daniel Fuchs wrote:

Hi,

Please find below a fix for what I believe is a test bug.
I plan to push this in JDK 9 dev.

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

This seems to be a very intermittent failure.
It looks as if a logger held in a local variable can be
arbitrarily garbage collected if that variable is no longer
used.
To prevent arbitrary garbage collection of such loggers, the fix
makes sure to use the variable again at the end of the test.



Looks like running the test in -Xcomp and also with jtreg creates
pressure to the young gen and more object allocation failure and so
unreachable objects get collected in the middle of a small method body
(like in this case).

You added code to reference the local variables referencing the
loggers.  Would it be simpler to have this test  just to hold a strong
reference to the logger?


Hmmm. I don't think it would be simpler. I could have added an
array list of loggers and cleared that at the end of the test.
It would just have been another way of making sure that the
loggers weren't gc'ed.


Are you concerned that a static/instance field holding the loggers gets 
GC'ed if not referenced before the test method returns?   They should be 
only collected when the class/instance becomes unreachable and it 
shouldn't be an issue (as do in other tests).


I'm fine with what you have.

Mandy

Re: Bug in Long.parseUnsignedLong

2013-12-19 Thread roger riggs

Created JDK-8030814  
to track this issue.


Roger


On 12/18/2013 6:52 PM, Louis Wasserman wrote:

Derp.  Here is the test case:

import java.math.BigInteger;

public class UnsignedLongBug {
   public static void main(String[] args) {
 try {
   String input = "1234567890abcdef1";
   System.out.println(Long.parseUnsignedLong(input, 16));
   BigInteger maxUnsignedLong =
BigInteger.ONE.shiftLeft(64).subtract(BigInteger.ONE);
   BigInteger inputValue = new BigInteger(input, 16);
   System.out.println(maxUnsignedLong.compareTo(inputValue));
   throw new AssertionError();
 } catch (NumberFormatException expected) {
   System.out.println("Correct");
 }
   }
}


On Wed, Dec 18, 2013 at 3:51 PM, Louis Wasserman wrote:


The Javadoc of Long.parseUnsignedLong specifies that it should throw a
NumberFormatException if "the value represented by the string is larger
than the largest unsigned long, 2^64-1."

This does not appear to be happening:

--
Louis Wasserman

Re: Theoretical data race on java.util.logging.Handler.sealed


Hi Peter,

Good idea to add a package constructor in Handler.
It looks much cleaner than the configure method.

Good tests too - thanks for adding that!

To access files with JPRT there is a simpler manner (though what
you have looks good).
JPRT sets a system property "test.src" which point at the
directory where the sources are located.

So you could have used something like:

System.setProperty(CONFIG_FILE_PROPERTY,
   new File(System.getProperty("test.src", "."),
this.getClass().getSimpleName()+".props")
.getAbsolutePath());

best regards,

-- daniel

On 12/19/13 4:49 PM, Peter Levart wrote:

On 12/18/2013 11:55 PM, Mandy Chung wrote:


On 12/18/2013 9:03 AM, Peter Levart wrote:

Hi Mandy, Daniel,

Here's yet another variant that reduces the doPrivileged code to just
Handler's setters. This way no LogManager methods are invoked under
elevated privilege:

http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/




This version looks good.  I like the refactoring to have the subclass
to call the common code Handler.configure method.  It may be better to
have the configure method (or a new one) that takes the default Level
and default Formatter instead of the package-private getters.

I don't see why the handler constructors are designed to call the
overridden methods rather than the initialization and if a subclass
has its custom field, it should initialize its custom fields in its
constructor implementation.Anyway this would be a separate clean
up task from this one.

Can you also add a sanity test to verify that these handlers can be
constructed successfully with a security manager installed?



Hi Mandy, Daniel,

I didn't like the package-protected getters either. So here's another
variant that replaces Handler.configure() method with a
package-protected constructor which is chained from JDK subclasses:

http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/

I filed another bug that is fixed by this patch:

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

And I created a test (see webrev.07) that almost passes when run against
unchanged JDK 8 (the failure is just at the end when calling new
SocketHandler(host, port) - access denied
("java.util.logging.LoggingPermission" "control")). If I comment-out the
System.setSecurityManager() from the test, it passes with unchanged
code. This is to verify the test itself. When run against the patched
JDK 8, it passes even when SecurityManager is active - this verifies two
things:
- the behaviour of patched code is analogous to unpatched code as far as
defaults and configured handler properties is concerned and it conforms
to javadoc
- the patched code does not require any new permissions - it actually
requires less, because it fixes bug 8030801.

All java/util/logging jtreg tests pass with patched code. I hope that
"localhost" is a resolvable name on all environments and that new
ServerSocket(0) creates a server socket bound at least to the IP address
that "localhost" resolves to. Is this reasonable to assume?


Regards, Peter

Re: Theoretical data race on java.util.logging.Handler.sealed

2013-12-19 Thread Peter Levart


On 12/18/2013 11:55 PM, Mandy Chung wrote:


On 12/18/2013 9:03 AM, Peter Levart wrote:

Hi Mandy, Daniel,

Here's yet another variant that reduces the doPrivileged code to just 
Handler's setters. This way no LogManager methods are invoked under 
elevated privilege:


http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.06/ 





This version looks good.  I like the refactoring to have the subclass 
to call the common code Handler.configure method.  It may be better to 
have the configure method (or a new one) that takes the default Level 
and default Formatter instead of the package-private getters.


I don't see why the handler constructors are designed to call the 
overridden methods rather than the initialization and if a subclass 
has its custom field, it should initialize its custom fields in its 
constructor implementation.Anyway this would be a separate clean 
up task from this one.


Can you also add a sanity test to verify that these handlers can be 
constructed successfully with a security manager installed?




Hi Mandy, Daniel,

I didn't like the package-protected getters either. So here's another 
variant that replaces Handler.configure() method with a 
package-protected constructor which is chained from JDK subclasses:


http://cr.openjdk.java.net/~plevart/jdk8-tl/jul.Handler.sealed/webrev.07/

I filed another bug that is fixed by this patch:

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

And I created a test (see webrev.07) that almost passes when run against 
unchanged JDK 8 (the failure is just at the end when calling new 
SocketHandler(host, port) - access denied 
("java.util.logging.LoggingPermission" "control")). If I comment-out the 
System.setSecurityManager() from the test, it passes with unchanged 
code. This is to verify the test itself. When run against the patched 
JDK 8, it passes even when SecurityManager is active - this verifies two 
things:
- the behaviour of patched code is analogous to unpatched code as far as 
defaults and configured handler properties is concerned and it conforms 
to javadoc
- the patched code does not require any new permissions - it actually 
requires less, because it fixes bug 8030801.


All java/util/logging jtreg tests pass with patched code. I hope that 
"localhost" is a resolvable name on all environments and that new 
ServerSocket(0) creates a server socket bound at least to the IP address 
that "localhost" resolves to. Is this reasonable to assume?



Regards, Peter

Re: RFR of lang level code migration patches


Hi Paul,

I looked at the modifications in java.util.logging and they
look both sensible and desirable.

Had to look at the code for Objects.toString(Object,Object) as
I had never used that before :-)


Thanks for taking care of that,

-- daniel

On 12/19/13 3:51 PM, Paul Sandoz wrote:

Hi,

Here are some patches that migrate some code to use more up to date language 
features. I will create a bug later on after feedback.

This is motivated from Brian's patches to lang tools.

I focused just on java.util, minus the concurrent packages, and i used the IDE 
to assist in the code migration. It's easy to pick off other packages over 
time. This makes for more of a low-brow effort [*], perfect when one has a cold.

Use <> syntax:
http://cr.openjdk.java.net/~psandoz/tl/j.u.diamond/webrev/

Replace for with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach/webrev/

Replace while with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach-while/webrev/

Compress catches
http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/

Use switch for strings; not sure this one is worth it
http://cr.openjdk.java.net/~psandoz/tl/j.u.swtich/webrev/

I ran all the j.u. tests locally and there were no regressions. Yet to run a 
JPRT job.

Paul.

[*] although one needs to be vigilant since sometimes the IDE can refactor 
incorrectly and sometimes code is arranged in a certain way for a reason (which 
one reason for leaving j.u.concurrent packages alone for now).

RFR of lang level code migration patches

2013-12-19 Thread Paul Sandoz

Hi,

Here are some patches that migrate some code to use more up to date language 
features. I will create a bug later on after feedback.

This is motivated from Brian's patches to lang tools.

I focused just on java.util, minus the concurrent packages, and i used the IDE 
to assist in the code migration. It's easy to pick off other packages over 
time. This makes for more of a low-brow effort [*], perfect when one has a cold.

Use <> syntax:
http://cr.openjdk.java.net/~psandoz/tl/j.u.diamond/webrev/

Replace for with for-each 
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach/webrev/

Replace while with for-each
http://cr.openjdk.java.net/~psandoz/tl/j.u.foreach-while/webrev/

Compress catches
http://cr.openjdk.java.net/~psandoz/tl/j.u.catch/webrev/

Use switch for strings; not sure this one is worth it
http://cr.openjdk.java.net/~psandoz/tl/j.u.swtich/webrev/

I ran all the j.u. tests locally and there were no regressions. Yet to run a 
JPRT job.

Paul.

[*] although one needs to be vigilant since sometimes the IDE can refactor 
incorrectly and sometimes code is arranged in a certain way for a reason (which 
one reason for leaving j.u.concurrent packages alone for now).

Re: Theoretical data race on java.util.logging.Handler.sealed

2013-12-19 Thread Alan Bateman


On 14/12/2013 11:25, Peter Levart wrote:


It's unfortunate that a lambda debugging feature prevents us from 
using a basic language feature in j.u.logging code. As far as I know, 
java.lang.invoke.ProxyClassesDumper is only used if 
'jdk.internal.lambda.dumpProxyClasses' system property is set to point 
to a directory where lambda proxy class files are to be dumped as they 
are generated - a debugging hook therefore. Wouldn't it be good-enough 
if error messages about not-being able to set-up/use the dump facility 
were output to System.err directly - not using PlatformLogger at all?
It is unfortunate although my comment was mostly thinking about the 
String.format usages when generating proxy classes. It may be that they 
can be changed (we've had to back off on at least one case of using 
method handles because of this).


-Alan

hg: jdk8/tl/jdk: 2 new changesets

2013-12-19 Thread chris . hegarty

Changeset: e2bdddb8bedf
Author:dl
Date:  2013-12-19 10:31 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/e2bdddb8bedf

8026155: Enhance ForkJoin pool
Reviewed-by: chegar, alanb, ahgross

! src/share/classes/java/util/concurrent/ForkJoinPool.java
! src/share/classes/java/util/concurrent/ForkJoinWorkerThread.java

Changeset: c841815be720
Author:chegar
Date:  2013-12-19 10:38 +
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c841815be720

Merge

Re: Demo for Parallel Core Collection API

2013-12-19 Thread Tristan Yan


Hi Paul And Everyone
Sorry for getting back late.
I took Paul's suggestion and have written other two demos which presents 
usage of parallel computation. One is using Monte-Carlo to calculate 
value of PI. Other is find a big prime by given length. Please review it.
http://cr.openjdk.java.net/~tyan/sample/webrev.00/ 

There is another demo which present mandelbrot set was designed 
Alexander Kouznetsov has been already in reviewing. It's not my code 
review request.

Thank you very much
Tristan


On 10/15/2013 11:20 PM, Paul Sandoz wrote:


On Oct 15, 2013, at 4:35 PM, Tristan Yan > wrote:



Hi Paul
you have comments "suggest that all streams are sequential. There is 
an inconsistency in the use and in some cases it is embedded in other 
stream usages."


We do not really understand what exactly is meant, could you 
elaborate a little bit. Is it because we want to show ppl that we 
should use stream more than parallelStream?


Going parallel is easy to do but not always the right thing to do. 
Going parallel almost always requires more work with the expectation 
that work will complete sooner than the work required to get the same 
result sequentially. There are a number of factors that affect whether 
parallel is faster than sequential. Two of those factors are N, the 
size of the data, and Q the cost of processing an element in the 
pipeline. N * Q is a simple cost model, the large that product the 
better the chances of parallel speed up. N is easy to know, Q not so 
easy but can often be intuitively guessed. (Note that there are other 
factors such as the properties of the stream source and operations 
that Brian and I talked about in our J1 presentation.)


Demo code that just makes everything (or most streams) parallel is 
sending out the wrong message.


So i think the demo code should present two general things:

1) various stream functionality, as you have done;

2) parallel vs. sequential for various cases where it is known that 
parallel is faster on a multi-core system.


For 2) i strongly recommend measuring using jmh [1]. The data sets you 
have may or may not be amenable to parallel processing, it's worth 
investigating though.


I have ideas for other parallel demos. One is creating probably primes 
(now that SecureRandom is replaced with ThreadLocalRandom), creating a 
probably prime that is a BigInteger is an relatively expensive 
operation so Q should be high. Another more advanced demo is a 
Monte-Carlo calculation of PI using SplittableRandom and a special 
Spliterator, in this case N should be largish. But there are other 
simpler demonstrations like sum of squares etc to get across that N 
should be large. Another demo could be calculation of a mandelbrot 
set, which is embarrassingly parallel over an area in the complex plane.


So while you should try and fit some parallel vs. sequential execution 
into your existing demos i do think it worth having a separate set of 
demos that get across the the simple cost model of N * Q. So feel free 
to use some of those ideas previously mentioned, i find those ideas 
fun so perhaps others will too :-)


Paul.

[1] http://openjdk.java.net/projects/code-tools/jmh/

On Oct 15, 2013, at 4:37 PM, Tristan Yan > wrote:



Also there is one more question I missed

You suggested ""ParallelCore" is not a very descriptive name. Suggest 
"streams"."

1) yes we agree this demo is not for parallel computation per se
2) but we do not have a clear demo for parallel computation
3) if we are to rename this, we need to develop another one, do you 
have a scenario for that?

Re: Request: Remove System.out check from Class.checkInitted()


On 19/12/2013 9:59 PM, Alan Bateman wrote:

On 19/12/2013 10:13, David Holmes wrote:


Not necessarily. The question is, are there any code paths that lead
to checkInitted being called after  setOut0(newPrintStream(fdOut,
props.getProperty("sun.stdout.encoding"))) but before the call to
sun.misc.VM.booted(). If so these would fail under the proposed change.

The initialization sequence is fragile and intimately tied to the
Hotspot VM - ie the VM initialization process initiates the core class
initialization. In a "normal" initialization System is initialized
before Class and Class must be initialized before checkInitted can be
called, so this doesn't trigger initialization of System.

I also don't see how System.out being null can be valid
post-initialization state given the call to
setOut0(newPrintStream(fdOut, props.getProperty("sun.stdout.encoding")))

It would be unusual to change it later with System.setOut but it is
possible.

In any case, using VM.isBooted is the normal way to check if system
initialization has completed.


Yes but it may not be sufficient in this case. As I said we have to be 
sure checkInitted can not be called by anything between the setting of 
out to non-null and the call to booted().


David


-Alan.

New default for ForkJoinPool.commonPool on systems with SecurityManagers

2013-12-19 Thread Doug Lea


[Cross-posting on core-libs-dev and concurrency-interest.]

The ForkJoinPool common pool is used in JDK8 for
all parallel Stream operations, parallel sorting, etc.
When designing this, we knew that in some managed environments,
administrators might want to limit or disable parallelism,
so we support a way to do so using system property
java.util.concurrent.ForkJoinPool.common.parallelism.

But we hadn't provided a way to allow only "innocuous" parallelism;
performed by worker threads that are happy to help sort
data etc, but cannot do anything (like open a file) that
needs a Permission. We allowed people to create a special
ForkJoinWorkerThreadFactory and use for the default via property
java.util.concurrent.ForkJoinPool.common.threadFactory.
But it is not at all easy for people running Java EE and other
managed platforms to define a suitable factory.

So we added an extra-conservative safe-out-of-the-box default:
If not already overridden by system properties,
and a SecurityManager is present, the default is now
to use instances of an internal (non-public)
InnocuousForkJoinWorkerThread class. Each worker
has no Permssions set, is not a member of any user-defined
ThreadGroup, and erases all ThreadLocals after running
any externally-submitted task.

Thanks especially to Chris Hegarty and Alan Bateman for helping
to figure out what "innocuous" should entail.

Even though it is running very late in the release process,
this seems like such a good idea that we hope to get it
in for JDK8. The internal mechanics to  support this currently
require  quite a lot of encapsulation breakage.
Someday we might want to try to generalize the idea of
innocuous threads in a way that could be more tastefully supported.

This is currently committed in 166 CVS, and Chris Hegarty will
soon put out a webrev for OpenJDK.

This support was also added to the jsr166e version so people can
also experiment with it even if not yet able to use JDK8.
Although I believe that this will only work if jsr166e.jar
is in bootclasspath.

Also note that extra-paranoid and/or mean-spirited
administrators can still disable all parallelism.

-Doug

Re: Request: Remove System.out check from Class.checkInitted()

2013-12-19 Thread Alan Bateman


On 19/12/2013 10:13, David Holmes wrote:


Not necessarily. The question is, are there any code paths that lead 
to checkInitted being called after  setOut0(newPrintStream(fdOut, 
props.getProperty("sun.stdout.encoding"))) but before the call to 
sun.misc.VM.booted(). If so these would fail under the proposed change.


The initialization sequence is fragile and intimately tied to the 
Hotspot VM - ie the VM initialization process initiates the core class 
initialization. In a "normal" initialization System is initialized 
before Class and Class must be initialized before checkInitted can be 
called, so this doesn't trigger initialization of System.


I also don't see how System.out being null can be valid 
post-initialization state given the call to 
setOut0(newPrintStream(fdOut, props.getProperty("sun.stdout.encoding")))
It would be unusual to change it later with System.setOut but it is 
possible.


In any case, using VM.isBooted is the normal way to check if system 
initialization has completed.


-Alan.

Re: Request: Remove System.out check from Class.checkInitted()


On 19/12/2013 9:19 PM, Jeroen Frijters wrote:

System.out can be null, because System.setOut(null) is legal.


Ah I see. That isn't an initialization issue but a post-initialization 
issue.


Thanks,
David


Regards,
Jeroen


From: David Holmes [mailto:david.hol...@oracle.com]
Sent: Thursday, December 19, 2013 11:14
To: Alan Bateman
Cc: Jeroen Frijters; core-libs-dev@openjdk.java.net
Subject: Re: Request: Remove System.out check from Class.checkInitted()

On 4/12/2013 7:00 PM, Alan Bateman wrote:

On 04/12/2013 08:24, Jeroen Frijters wrote:

Hi,

I'd like to propose to change Class.checkInitted() to check if the VM
initialization has completed by using sun.misc.VM.isBooted() instead
of System.out != null.

This should be changed (I can only guess that whoever added this
wasn't aware of VM.isBooted).


Not necessarily. The question is, are there any code paths that lead to
checkInitted being called after  setOut0(newPrintStream(fdOut,
props.getProperty("sun.stdout.encoding"))) but before the call to
sun.misc.VM.booted(). If so these would fail under the proposed change.

The initialization sequence is fragile and intimately tied to the
Hotspot VM - ie the VM initialization process initiates the core class
initialization. In a "normal" initialization System is initialized
before Class and Class must be initialized before checkInitted can be
called, so this doesn't trigger initialization of System.

I also don't see how System.out being null can be valid post-
initialization state given the call to setOut0(newPrintStream(fdOut,
props.getProperty("sun.stdout.encoding")))

David
-


-Alan.

RE: Request: Remove System.out check from Class.checkInitted()

2013-12-19 Thread Jeroen Frijters

System.out can be null, because System.setOut(null) is legal.

Regards,
Jeroen

> From: David Holmes [mailto:david.hol...@oracle.com]
> Sent: Thursday, December 19, 2013 11:14
> To: Alan Bateman
> Cc: Jeroen Frijters; core-libs-dev@openjdk.java.net
> Subject: Re: Request: Remove System.out check from Class.checkInitted()
> 
> On 4/12/2013 7:00 PM, Alan Bateman wrote:
> > On 04/12/2013 08:24, Jeroen Frijters wrote:
> >> Hi,
> >>
> >> I'd like to propose to change Class.checkInitted() to check if the VM
> >> initialization has completed by using sun.misc.VM.isBooted() instead
> >> of System.out != null.
> > This should be changed (I can only guess that whoever added this
> > wasn't aware of VM.isBooted).
> 
> Not necessarily. The question is, are there any code paths that lead to
> checkInitted being called after  setOut0(newPrintStream(fdOut,
> props.getProperty("sun.stdout.encoding"))) but before the call to
> sun.misc.VM.booted(). If so these would fail under the proposed change.
> 
> The initialization sequence is fragile and intimately tied to the
> Hotspot VM - ie the VM initialization process initiates the core class
> initialization. In a "normal" initialization System is initialized
> before Class and Class must be initialized before checkInitted can be
> called, so this doesn't trigger initialization of System.
> 
> I also don't see how System.out being null can be valid post-
> initialization state given the call to setOut0(newPrintStream(fdOut,
> props.getProperty("sun.stdout.encoding")))
> 
> David
> -
> 
> > -Alan.
> >

Re: RFR: java/util/logging/Logger/setResourceBundle/TestSetResourceBundle.java failing again


On 12/18/13 9:35 PM, Mandy Chung wrote:


On 12/17/2013 3:57 AM, Daniel Fuchs wrote:

Hi,

Please find below a fix for what I believe is a test bug.
I plan to push this in JDK 9 dev.

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

This seems to be a very intermittent failure.
It looks as if a logger held in a local variable can be
arbitrarily garbage collected if that variable is no longer
used.
To prevent arbitrary garbage collection of such loggers, the fix
makes sure to use the variable again at the end of the test.



Looks like running the test in -Xcomp and also with jtreg creates
pressure to the young gen and more object allocation failure and so
unreachable objects get collected in the middle of a small method body
(like in this case).

You added code to reference the local variables referencing the
loggers.  Would it be simpler to have this test  just to hold a strong
reference to the logger?


Hmmm. I don't think it would be simpler. I could have added an
array list of loggers and cleared that at the end of the test.
It would just have been another way of making sure that the
loggers weren't gc'ed.

-- daniel




Mandy




In the event that my analysis were wrong, I have also added
some debug traces that should help if this test fails again in
similar situations.

http://cr.openjdk.java.net/~dfuchs/webrev_8030187/webrev.00/

best regards,

-- daniel

Re: Request: Remove System.out check from Class.checkInitted()