Re: Time to put a stop to Thread.stop?

[Dawid Weiss]

Hello everyone,

To contribute to the discussion concerning Thread.stop(*) and its
usefulness. I've implemented a JUnit test runner for Lucene/ Solr and
we needed test suite isolation badly but at the same time wanted to
reuse the same JVM (for performance reasons).

The initial implementation worked by detecting leaked threads (those
started within a test suite and crossing its lifecycle boundary) and
then tried to gracefully interrupt them; if this didn't help, an
attempt to kill those threads was issued with a call to
Thread.stop().

This works very well on small examples only. In practice what happens is:

1) you get loops with a catch (Throwable) inside; pretty much prevents
the thread from being stopped (these we called ChuckNorris-type...),
2) threads going into native sections and hung there (not necessarily
via JNI, even via system IO calls),

Eventually we just dropped thread.stop altogether -- if a thread
cannot be interrupted, it is considered a zombie and the test
framework either proceeds or aborts entirely (depending on the
configuration).

So, based on this experience my point of view on the matter is that
Thread.stop() is useless. For rethrowing checked exceptions you can
use the generics-based sneaky throw. If you have control over the code
of the threads you want to stop then you shouldn't use Thread.stop()
anyway. For any code you don't have the control over it's not 100%
reliable so I doubt its use is really fully justified.

A much more needed addition to the standard library in place of
Thread.stop() would be methods to acquire:

1) your own PID (this one is in planning for 1.8 or even has been
added already),
2) forked process PID (?),
3) a way to kill the forked process (and possibly its sub-tree).

These are platform-specific operations and the workarounds for these
missing API calls are truly horrible, even if implemented nicely (see
sources of Jenkins for example [1]).

Dawid

[1] 
https://github.com/jenkinsci/jenkins/blob/master/core/src/main/java/hudson/util/ProcessTree.java#L501

hg: jdk8/tl/langtools: 8004133: Provide javax.lang.model.* implementation backed by core reflection

[joe . darcy]

Changeset: bcd927639039
Author:darcy
Date:  2013-05-15 00:00 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/bcd927639039

8004133: Provide javax.lang.model.* implementation backed by core reflection
Summary: Joint work by darcy and jfranck to provide sample code for JEP 119.
Reviewed-by: jjg
Contributed-by: joe.da...@oracle.com, joel.fra...@oracle.com

+ src/share/sample/language/model/CoreReflectionFactory.java

Re: bug 8014477 Race in String.contentEquals

[Peter Levart]

Mike, David,

Could you try this variant of the test:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.05/


I tried it on 3 different machines (4-core i7 Linux, 8-core sparc T-2 
Solaris, 4-core amd64 Solaris) x 2 different JDK8 JVMs (64bit and 32bit) 
and it fails immediately on all 6 of them.


Regards, Peter

On 05/14/2013 05:39 PM, Mike Duigou wrote:

Good to see the test added. I was also unable to reproduce the failure on my 
Core 2 Duo Mac laptop but the test and fix match up.

Mike


On May 14 2013, at 04:34 , Peter Levart wrote:


On 05/14/2013 01:17 PM, David Holmes wrote:

Thanks Peter this looks good to me.

One minor thing please set the correct copyright year in the test, it should 
just be

Copyright (c) 2013, Oracle ...

Ok, fixed:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.04/



I couldn't get the test to actually fail, but I can see that it could.

Hm, do you have a multi-core chip? On my computer (i7 CPU) it fails immediately 
and always. You could try varying the number of concurrent threads and or the 
time to wait for exception to be thrown...


David

On 14/05/2013 9:04 PM, Peter Levart wrote:

Ok, here's the corrected fix:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.03/

I also added a reproducer for the bug.

Regards, peter

On 05/14/2013 07:53 AM, Peter Levart wrote:

Right, sb.length() should be used. I will correct that as soon as I
get to the keyboard.

Regards, Peter

On May 14, 2013 7:22 AM, David Holmes david.hol...@oracle.com
mailto:david.hol...@oracle.com wrote:

Thanks Peter! I've filed

8014477
Race condition in String.contentEquals when comparing with
StringBuffer

On 14/05/2013 8:34 AM, Peter Levart wrote:

On 05/14/2013 12:09 AM, Peter Levart wrote:

I noticed a synchronization bug in String.contentEquals
method. If
called with a StringBuffer argument while concurrent thread is
modifying the StringBuffer, the method can either throw
ArrayIndexOutOfBoundsException or return true even though
the content
of the StringBuffer has never been the same as the String's.

Here's a proposed patch:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.01/
http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.01/

Regards, Peter


Or even better (with some code clean-up):

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.02/
http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.02/


This part is not correct I believe:

1010 private boolean
nonSyncContentEquals(AbstractStringBuilder sb) {
1011 char v1[] = value;
1012 char v2[] = sb.getValue();
1013 int n = v1.length;
1014 if (n != v2.length) {
1015 return false;
1016 }

v2 can be larger than v1 if v2 is not being fully used (ie count 
length).

David
-

Re: Time to put a stop to Thread.stop?

[David Holmes]

On 15/05/2013 3:16 PM, Martin Buchholz wrote:

On Tue, May 14, 2013 at 8:17 PM, David Holmes david.hol...@oracle.com
mailto:david.hol...@oracle.com wrote:

On 15/05/2013 2:57 AM, Martin Buchholz wrote:

On Tue, May 14, 2013 at 7:45 AM, Jeroen Frijters
jer...@sumatra.nl mailto:jer...@sumatra.nl wrote:

IMO Thread.currentThread().stop(__new Throwable()) should
continue to work.
It is not unsafe and it is probably used in a lot of code to
workaround the
madness that is checked exceptions.


That is truly awful! Why wouldn't people just wrap in a runtime
exception  Truly, truly awful. :(


General purpose library code sometimes would like to rethrow an
exception that was previously caught.
How should it do that?


Umm catch it and throw it. If it is a checked-exception that you want to 
propogate then you should have declared it on your method, else you are 
going to wrap it in a runtime exception or error. There is no need for 
such sleaze.



I don't think there's a generally accepted
solution, although there's more than one (sneaky) way to do it, and we
could stop using Thread.stop for that purpose.




If we had to we could special-case for currentThread. :(


There are existing JDK tests that use currentThread().stop to
implement the
occasionally necessary sneakyThrow.

I suspect there are important uses of unsafe otherThread.stop in
the real
world, where it is used as a last resort to shut down an
application
running within a java vm, and works reasonably well in practice.


I would dispute that it can work reasonably well in practice given
the near impossibility of writing async-exception-safe non-trivial
Java code. That aside, the proposal is only for the stop(throwable)
form which I would not expect to be used for the termination case.


I agree it's unsafe.  But you have the same problem to a lesser extent
with kill -9, which is also an indispensable part of every engineer's
toolbox, and works well enough in practice.


There is a huge difference between blowing away a complete process with 
kill and having a single thread starting to propagate an async 
exception, unwinding its stack and executing finally blocks with 
completely broken state invariants.


David

Re: Time to put a stop to Thread.stop?

[Alan Bateman]

On 15/05/2013 08:00, Dawid Weiss wrote:

:

A much more needed addition to the standard library in place of
Thread.stop() would be methods to acquire:

1) your own PID (this one is in planning for 1.8 or even has been
added already),
2) forked process PID (?),
3) a way to kill the forked process (and possibly its sub-tree).

We have JEP 102 to update Process. Some of the low hanging fruit, like 
Process.destroyForcibly, went into jdk8 so they have been removed from 
the JEP. There has been a number of attempts at adding support for 
exposing the process-id. Implementation is trivial, the hard part has 
always been consideration for environments where they may not be a 1-1 
mapping. In any case, thanks for the comments and experiences on using 
Thread.stop.


-Alan.

Re: Time to put a stop to Thread.stop?

[Martin Desruisseaux]

Le 15/05/13 10:05, David Holmes a écrit :
There is a huge difference between blowing away a complete process 
with kill and having a single thread starting to propagate an async 
exception, unwinding its stack and executing finally blocks with 
completely broken state invariants.


Given the risk, what about a mechanism similar to the one which 
currently hides the Sun internal packages from the compilation phase but 
not yet from the runtime phase? Would it be acceptable to have 'javac' 
and 'javadoc' woking as if the 'Thread.stop(Throwable)' method did not 
existed anymore, while having the method still working (for now) at 
runtime for existing libraries? Maybe with an annotation yet stronger 
than @Deprecated.


Martin

Re: Time to put a stop to Thread.stop?

[Dawid Weiss]

 We have JEP 102 to update Process. Some of the low hanging fruit, like
 Process.destroyForcibly, went into jdk8 so they have been removed from the 
 JEP.

Thanks for the pointer, Alan. I fully agree with the rationale:

Long-standing shortcoming in the platform [...] Long requested by developers.

 [...] the hard part has always been consideration for environments where they 
 may
 not be a 1-1 mapping.

I think this should be handled by allowing those methods to throw
UnsupportedOperationException or some other form of signalling the
operation as unsupported. 99% of the operating systems in existence
will have (trivial?) support for these operations -- they are the very
core of what an operating system is (process management), right?.

Dawid

Re: bug 8014477 Race in String.contentEquals

[Alan Bateman]

On 15/05/2013 08:50, Peter Levart wrote:

Mike, David,

Could you try this variant of the test:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.05/ 




I tried it on 3 different machines (4-core i7 Linux, 8-core sparc T-2 
Solaris, 4-core amd64 Solaris) x 2 different JDK8 JVMs (64bit and 
32bit) and it fails immediately on all 6 of them.


Regards, Peter

The previous and new version both duplicate almost immediately for me 
when I tried on a 2 x 4-core Xeon and a 64 thread T4.


-Alan.

[PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[Eric Wang]

Hi,

Please help to review the fix for bug 8004177 
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178 
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make 
sure all child threads finished before main thread exits.


http://cr.openjdk.java.net/~ewang/8004177/webrev.00/ 
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/


For 8004178, the test StackTraces.java is same as 
GenerifyStackTraces.java, so just remove it.


Thanks,
Eric

Re: [PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[Chris Hegarty]

Looks fine to me Eric. Let me know if you need someone to sponsor this 
change for you.


-Chris.

On 15/05/2013 10:10, Eric Wang wrote:

Hi,

Please help to review the fix for bug 8004177
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make
sure all child threads finished before main thread exits.

http://cr.openjdk.java.net/~ewang/8004177/webrev.00/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/

For 8004178, the test StackTraces.java is same as
GenerifyStackTraces.java, so just remove it.

Thanks,
Eric

Re: [PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[Eric Wang]

Hi Chris,

Thanks you for the review and sponsor this change.

Regards,
Eric
On 2013/5/15 17:26, Chris Hegarty wrote:
Looks fine to me Eric. Let me know if you need someone to sponsor this 
change for you.


-Chris.

On 15/05/2013 10:10, Eric Wang wrote:

Hi,

Please help to review the fix for bug 8004177
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make
sure all child threads finished before main thread exits.

http://cr.openjdk.java.net/~ewang/8004177/webrev.00/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/

For 8004178, the test StackTraces.java is same as
GenerifyStackTraces.java, so just remove it.

Thanks,
Eric

Re: [PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[Alan Bateman]

On 15/05/2013 10:10, Eric Wang wrote:

Hi,

Please help to review the fix for bug 8004177 
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178 
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make 
sure all child threads finished before main thread exits.


http://cr.openjdk.java.net/~ewang/8004177/webrev.00/ 
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/


For 8004178, the test StackTraces.java is same as 
GenerifyStackTraces.java, so just remove it.


Thanks,
Eric
It's good to have fix this test so that it doesn't leave a thread behind 
(in this case slowing down the execution of subsequent tests by taking 
thread dumps every 2 seconds)


The change you propose is okay but a bit odd to have ThreadOne signal 
DumpThread to shutdown. An alternative would be to have the main thread 
signal DumpThread, as in:


  one.join();

  finished = true;
  dt.join();

Better still might be to move the flag into the DumpThread class and 
have it define a shutdown method so the main thread does:


  one.join();

  dt.shutdown();
  dt.join();

I think that would be a bit cleaner.

-Alan.

Re: RFR : 8004015 : (S) Additional Functional Interface instance and static methods

[Chris Hegarty]

All looks good to me, and nicely consistent.

-Chris.

On 15/05/2013 05:22, Mike Duigou wrote:

Hello all;

This a fairly small set of additional methods for the existing Java 8 
functional interfaces. These methods have been held back until now awaiting 
support for static interface methods.

http://cr.openjdk.java.net/~mduigou/JDK-8004015/0/webrev/

Mike

Re: RFR: JDK-8013900: More warnings compiling jaxp.

[Daniel Fuchs]

Hi,

Please find below a revised version of the fix for
   JDK-8013900: More warnings compiling jaxp.
http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.01/

Difference with the previous version [1] are in LocalVariableGen.java,
plus 4 additional files:
   BranchInstruction.java
   CodeExceptionGen.java
   LineNumberGen.java
   Select.java

This new set of changes fixes an additional issue I discovered in
LocalVariableGen.java - while running the JCK tests.

LocalVariableGen had a bug that was hidden by the fact
that hashCode() was not compliant with equals().
Changing hashCode() to be compliant with equals() uncovered
that issue.

The issue with LocalVariableGen is that the instance
variables used by equals/hashCode are mutable.
This is an issue because instances of LocalVariableGen are
stored in an HashSet (targeters) held from instances of
InstructionHandle.

Whenever the instruction handles in LocalVariableGen are changed,
then the instance of LocalVariableGen was removed from the 'old'
instruction handle targeters HashSet, and added to the 'new'
instruction handle targeters HashSet - (see LocalVariableGen
updateTargets(..), LocalVariableGen.setStart(...) 
LocalVariableGen.setEnd(...).

The issue here was that the instance of LocalVariableGen was
removed from the 'old' instruction handle targeters HashSet
before being modified (which was correct) - but was also
added to the 'new' instruction handle targeters HashSet
before being modified - which was incorrect because at
that time the hashCode() was still computed using the 'old'
instruction handle, and therefore had its 'old' value.
(this was done by BranchInstruction.notifyTarget(...))

The fix for this new issue is to split
BranchInstruction.notifyTarget(...) in two methods:
BranchInstruction.notifyTargetChanging(...) which is called
before the instruction handle pointer is changed, and remove
the LocalVariableGen from the old instruction handle targeters
HashSet, and BranchInstruction.notifyTargetChanged(...), which
is called after the instruction handle pointer is changed,
and adds the LocalVariableGen to the new instruction handle
targeters HashSet.
In LocalVariableGen - whenever one of the instruction handles
that take parts in the hashCode computation is changed, we
unregister 'this' from all targeters HashSets in which it
is registered, then modify the instruction handle pointer,
then add back 'this' to all targeters HashSets in which it
needs to be registered.

The 4 additional files in the webrev were also calling
BranchInstruction.notifyTarget - and I modified them to
call the two new methods instead of the old one - but without
going through the trouble of unregistering 'this' from any
known HashSet before modifictation - and adding it after,
since those other classes do not have mutable hashCode().

Note: I also took this opportunity to change the method
calling BranchInstruction.notifyTargetXxxx to be final, as
I think it's desirable that they not be overridden, and to
make the index in LocalVariableGen final - since it was
never changed after the instance construction (and takes
part in the HashCode computation).

best regards,

-- daniel

previous version:
[1] http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

On 5/13/13 4:36 PM, Daniel Fuchs wrote:

This is a fix for JDK-8013900: More warnings compiling jaxp.

http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

Although the title might suggest a trivial fix, it goes a bit
beyond a simple warning fix because the root cause of those warnings
is that some classes redefine equals without redefining
hashCode() - and devising a hashCode() method that respects
its contract with equals() is not always trivial.

The proposed fix adds hashCode() methods where necessary, ensuring
that:

if (o1.equals(o2) == true), then (o1.hashCode() == o2.hashCode())

The fix also contains some cosmetic/sanity changes - like:

1. adding @Override wherever NetBeans complained they were
missing - and
2. replacing StringBuffer with StringBuilder when
possible.
3. In one instance, AbstractDateTimeDV.java I also had
to reformat the whole file (due to its weird original formatting)
- apologies for the noise it causes in the webrev.
4. I also removed a couple of private isEqual(obj1, obj2) methods
replacing them by calls to Objects.equals(obj1, obj2) which did
the same thing.
5. finally I refactored some of the existing equals (e.g. replacing
try { (Sometype) other } catch (ClassCastException x) {
return false; } with use of 'other instanceof Sometype'...)

There are however a couple of more serious changes that could
deserve to be reviewed in more details:

a. The new implementation of hashCode() in
AbstractDateTimeDV.DateTimeData, where I had to figure
out a way to convert the date to UTC so that the hashCode() would
match equals:

AbstractDateTimeDV.java: lines 972-992

and b. in PrecisionDecimalDV.XPrecisionDecimal - where I had to
invent a canonical string 

Re: RFR: JDK-8013900: More warnings compiling jaxp.

[Chris Hegarty]

I skimmed over the hashCode/equals parts of the changes, and they look 
fine to me.


-Chris.

On 15/05/2013 10:42, Daniel Fuchs wrote:

Hi,

Please find below a revised version of the fix for
JDK-8013900: More warnings compiling jaxp.
http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.01/

Difference with the previous version [1] are in LocalVariableGen.java,
plus 4 additional files:
BranchInstruction.java
CodeExceptionGen.java
LineNumberGen.java
Select.java

This new set of changes fixes an additional issue I discovered in
LocalVariableGen.java - while running the JCK tests.

LocalVariableGen had a bug that was hidden by the fact
that hashCode() was not compliant with equals().
Changing hashCode() to be compliant with equals() uncovered
that issue.

The issue with LocalVariableGen is that the instance
variables used by equals/hashCode are mutable.
This is an issue because instances of LocalVariableGen are
stored in an HashSet (targeters) held from instances of
InstructionHandle.

Whenever the instruction handles in LocalVariableGen are changed,
then the instance of LocalVariableGen was removed from the 'old'
instruction handle targeters HashSet, and added to the 'new'
instruction handle targeters HashSet - (see LocalVariableGen
updateTargets(..), LocalVariableGen.setStart(...) 
LocalVariableGen.setEnd(...).

The issue here was that the instance of LocalVariableGen was
removed from the 'old' instruction handle targeters HashSet
before being modified (which was correct) - but was also
added to the 'new' instruction handle targeters HashSet
before being modified - which was incorrect because at
that time the hashCode() was still computed using the 'old'
instruction handle, and therefore had its 'old' value.
(this was done by BranchInstruction.notifyTarget(...))

The fix for this new issue is to split
BranchInstruction.notifyTarget(...) in two methods:
BranchInstruction.notifyTargetChanging(...) which is called
before the instruction handle pointer is changed, and remove
the LocalVariableGen from the old instruction handle targeters
HashSet, and BranchInstruction.notifyTargetChanged(...), which
is called after the instruction handle pointer is changed,
and adds the LocalVariableGen to the new instruction handle
targeters HashSet.
In LocalVariableGen - whenever one of the instruction handles
that take parts in the hashCode computation is changed, we
unregister 'this' from all targeters HashSets in which it
is registered, then modify the instruction handle pointer,
then add back 'this' to all targeters HashSets in which it
needs to be registered.

The 4 additional files in the webrev were also calling
BranchInstruction.notifyTarget - and I modified them to
call the two new methods instead of the old one - but without
going through the trouble of unregistering 'this' from any
known HashSet before modifictation - and adding it after,
since those other classes do not have mutable hashCode().

Note: I also took this opportunity to change the method
calling BranchInstruction.notifyTargetXxxx to be final, as
I think it's desirable that they not be overridden, and to
make the index in LocalVariableGen final - since it was
never changed after the instance construction (and takes
part in the HashCode computation).

best regards,

-- daniel

previous version:
[1] http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

On 5/13/13 4:36 PM, Daniel Fuchs wrote:

This is a fix for JDK-8013900: More warnings compiling jaxp.

http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

Although the title might suggest a trivial fix, it goes a bit
beyond a simple warning fix because the root cause of those warnings
is that some classes redefine equals without redefining
hashCode() - and devising a hashCode() method that respects
its contract with equals() is not always trivial.

The proposed fix adds hashCode() methods where necessary, ensuring
that:

if (o1.equals(o2) == true), then (o1.hashCode() == o2.hashCode())

The fix also contains some cosmetic/sanity changes - like:

1. adding @Override wherever NetBeans complained they were
missing - and
2. replacing StringBuffer with StringBuilder when
possible.
3. In one instance, AbstractDateTimeDV.java I also had
to reformat the whole file (due to its weird original formatting)
- apologies for the noise it causes in the webrev.
4. I also removed a couple of private isEqual(obj1, obj2) methods
replacing them by calls to Objects.equals(obj1, obj2) which did
the same thing.
5. finally I refactored some of the existing equals (e.g. replacing
try { (Sometype) other } catch (ClassCastException x) {
return false; } with use of 'other instanceof Sometype'...)

There are however a couple of more serious changes that could
deserve to be reviewed in more details:

a. The new implementation of hashCode() in
AbstractDateTimeDV.DateTimeData, where I had to figure
out a way to convert the date to UTC so that the hashCode() would
match equals:

AbstractDateTimeDV.java: lines 972-992

and 

hg: jdk8/tl/langtools: 3 new changesets

[maurizio . cimadamore]

Changeset: 05ec778794d0
Author:mcimadamore
Date:  2013-05-15 14:00 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/05ec778794d0

8012003: Method diagnostics resolution need to be simplified in some cases
Summary: Unfold method resolution diagnostics when they mention errors in poly 
expressions
Reviewed-by: jjg, vromero

! src/share/classes/com/sun/tools/javac/comp/Attr.java
! src/share/classes/com/sun/tools/javac/comp/Resolve.java
! src/share/classes/com/sun/tools/javac/main/JavaCompiler.java
! src/share/classes/com/sun/tools/javac/main/Option.java
! src/share/classes/com/sun/tools/javac/resources/compiler.properties
! src/share/classes/com/sun/tools/javac/resources/javac.properties
! src/share/classes/com/sun/tools/javac/util/JCDiagnostic.java
! src/share/classes/com/sun/tools/javac/util/List.java
! src/share/classes/com/sun/tools/javac/util/Log.java
+ test/tools/javac/Diagnostics/compressed/T8012003a.java
+ test/tools/javac/Diagnostics/compressed/T8012003a.out
+ test/tools/javac/Diagnostics/compressed/T8012003b.java
+ test/tools/javac/Diagnostics/compressed/T8012003b.out
+ test/tools/javac/Diagnostics/compressed/T8012003c.java
+ test/tools/javac/Diagnostics/compressed/T8012003c.out
! test/tools/javac/diags/examples/BadArgTypesInLambda.java
+ test/tools/javac/diags/examples/CompressedDiags.java
! test/tools/javac/diags/examples/KindnameConstructor.java
+ test/tools/javac/diags/examples/ProbFoundReqFragment.java
! test/tools/javac/lambda/TargetType66.out

Changeset: 33d1937af1a3
Author:mcimadamore
Date:  2013-05-15 14:02 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/33d1937af1a3

8012685: Spurious raw types warning when using unbound method references
Summary: Spurious raw type warning when unbound method reference qualifier 
parameter types are inferred from target
Reviewed-by: jjg, vromero

! src/share/classes/com/sun/tools/javac/comp/Attr.java
! src/share/classes/com/sun/tools/javac/comp/Check.java
+ test/tools/javac/lambda/MethodReference67.java
+ test/tools/javac/lambda/MethodReference67.out

Changeset: 78717f2d00e8
Author:mcimadamore
Date:  2013-05-15 14:03 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/78717f2d00e8

8013222: Javac issues spurious raw type warnings when lambda has implicit 
parameter types
Summary: Bad warnings and position for lambda inferred parameter types
Reviewed-by: jjg, vromero

! src/share/classes/com/sun/tools/javac/comp/Attr.java
+ test/tools/javac/lambda/NoWarnOnImplicitParams.java
+ test/tools/javac/lambda/NoWarnOnImplicitParams.out

Re: Time to put a stop to Thread.stop?

[Vitaly Davidovich]

There's also Unsafe.throwException which can be used to rethrow checked
exceptions.  Without getting into another debate over checked exceptions,
this approach is much better than Thread.stop.

Sent from my phone
On May 15, 2013 4:06 AM, David Holmes david.hol...@oracle.com wrote:

 On 15/05/2013 3:16 PM, Martin Buchholz wrote:

 On Tue, May 14, 2013 at 8:17 PM, David Holmes david.hol...@oracle.com
 mailto:david.holmes@oracle.**com david.hol...@oracle.com wrote:

 On 15/05/2013 2:57 AM, Martin Buchholz wrote:

 On Tue, May 14, 2013 at 7:45 AM, Jeroen Frijters
 jer...@sumatra.nl mailto:jer...@sumatra.nl wrote:

 IMO Thread.currentThread().stop(__**new Throwable()) should
 continue to work.
 It is not unsafe and it is probably used in a lot of code to
 workaround the
 madness that is checked exceptions.


 That is truly awful! Why wouldn't people just wrap in a runtime
 exception  Truly, truly awful. :(


 General purpose library code sometimes would like to rethrow an
 exception that was previously caught.
 How should it do that?


 Umm catch it and throw it. If it is a checked-exception that you want to
 propogate then you should have declared it on your method, else you are
 going to wrap it in a runtime exception or error. There is no need for such
 sleaze.

  I don't think there's a generally accepted
 solution, although there's more than one (sneaky) way to do it, and we
 could stop using Thread.stop for that purpose.



  If we had to we could special-case for currentThread. :(


 There are existing JDK tests that use currentThread().stop to
 implement the
 occasionally necessary sneakyThrow.

 I suspect there are important uses of unsafe otherThread.stop in
 the real
 world, where it is used as a last resort to shut down an
 application
 running within a java vm, and works reasonably well in practice.


 I would dispute that it can work reasonably well in practice given
 the near impossibility of writing async-exception-safe non-trivial
 Java code. That aside, the proposal is only for the stop(throwable)
 form which I would not expect to be used for the termination case.


 I agree it's unsafe.  But you have the same problem to a lesser extent
 with kill -9, which is also an indispensable part of every engineer's
 toolbox, and works well enough in practice.


 There is a huge difference between blowing away a complete process with
 kill and having a single thread starting to propagate an async exception,
 unwinding its stack and executing finally blocks with completely broken
 state invariants.

 David

Re: Time to put a stop to Thread.stop?

[Remi Forax]

On 05/15/2013 10:39 AM, Martin Desruisseaux wrote:

Le 15/05/13 10:05, David Holmes a écrit :
There is a huge difference between blowing away a complete process 
with kill and having a single thread starting to propagate an async 
exception, unwinding its stack and executing finally blocks with 
completely broken state invariants.


Given the risk, what about a mechanism similar to the one which 
currently hides the Sun internal packages from the compilation phase 
but not yet from the runtime phase? Would it be acceptable to have 
'javac' and 'javadoc' woking as if the 'Thread.stop(Throwable)' method 
did not existed anymore, while having the method still working (for 
now) at runtime for existing libraries? Maybe with an annotation yet 
stronger than @Deprecated.


Martin



yes, I propose @Retired.

Rémi

What's the correct repository on which to base changes to jdk?

[David Chase]

The changed files are these:

M src/share/classes/java/util/zip/Adler32.java
M src/share/classes/java/util/zip/CRC32.java
M src/share/classes/sun/misc/VM.java
M src/share/native/java/util/zip/CRC32.c
M test/java/util/zip/TimeChecksum.java
A test/java/util/zip/CRCandAdlerTest.java

This is for a performance RFE filed against compiler,

JDK-7088419: Use x86 Hardware CRC32 Instruction with java.util.zip.CRC32 and 
java.util.zip.Adler32

The nature of the changes is
1) adding fork-join parallelism for Adler32 and CRC32,
2) handling small cases in Java when that is cheaper than JNI overheads, 
3) on suitable Intel platforms, using a fancy instruction to make CRC go faster.

There's a companion patch for the hotspot side -- the two conspire to pass 
platform feature information
through a property sun.zip.clmulSupported.

And to which list should I post the request for reviews?

David

Re: Time to put a stop to Thread.stop?

[Mario Torre]

2013/5/15 Remi Forax fo...@univ-mlv.fr:
 On 05/15/2013 10:39 AM, Martin Desruisseaux wrote:

 Le 15/05/13 10:05, David Holmes a écrit :

 There is a huge difference between blowing away a complete process with
 kill and having a single thread starting to propagate an async exception,
 unwinding its stack and executing finally blocks with completely broken
 state invariants.


 Given the risk, what about a mechanism similar to the one which currently
 hides the Sun internal packages from the compilation phase but not yet from
 the runtime phase? Would it be acceptable to have 'javac' and 'javadoc'
 woking as if the 'Thread.stop(Throwable)' method did not existed anymore,
 while having the method still working (for now) at runtime for existing
 libraries? Maybe with an annotation yet stronger than @Deprecated.

 Martin


 yes, I propose @Retired.

+1

I think it should also blow at runtime unless people passes some fancy
argument (like -XX:recallRetired :), this way is easier for people to
fix their code, or, in case they can't, do workaround it.

Cheers,
Mario

--
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

IcedRobot: www.icedrobot.org
Proud GNU Classpath developer: http://www.classpath.org/
Read About us at: http://planet.classpath.org
OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards:
http://endsoftpatents.org/

Re: What's the correct repository on which to base changes to jdk?

[Alan Bateman]

On 15/05/2013 15:30, David Chase wrote:

The changed files are these:

M src/share/classes/java/util/zip/Adler32.java
M src/share/classes/java/util/zip/CRC32.java
M src/share/classes/sun/misc/VM.java
M src/share/native/java/util/zip/CRC32.c
M test/java/util/zip/TimeChecksum.java
A test/java/util/zip/CRCandAdlerTest.java

This is for a performance RFE filed against compiler,

JDK-7088419: Use x86 Hardware CRC32 Instruction with java.util.zip.CRC32 and 
java.util.zip.Adler32

The nature of the changes is
1) adding fork-join parallelism for Adler32 and CRC32,
2) handling small cases in Java when that is cheaper than JNI overheads,
3) on suitable Intel platforms, using a fancy instruction to make CRC go faster.

There's a companion patch for the hotspot side -- the two conspire to pass 
platform feature information
through a property sun.zip.clmulSupported.

And to which list should I post the request for reviews?

David


The core-libs-dev is best for the java.util.zip code.

You can push changes to the zip code via any of the integration forests 
but I think it would be best if you used jdk8/tl/jdk as that is the 
route for all changes to the core libraries.


-Alan

hg: jdk8/tl/jdk: 8013730: JSR 310 DateTime API Updates III

[xueming . shen]

Changeset: ef04044f77d2
Author:sherman
Date:  2013-05-15 07:48 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/ef04044f77d2

8013730: JSR 310 DateTime API Updates III
Summary: Integration of JSR310 Date/Time API update III
Reviewed-by: naoto
Contributed-by: scolebou...@joda.org, roger.ri...@oracle.com, 
masayoshi.oku...@oracle.com, patrick.zh...@oracle.com

! src/share/classes/java/time/Clock.java
! src/share/classes/java/time/DateTimeException.java
! src/share/classes/java/time/DayOfWeek.java
! src/share/classes/java/time/Duration.java
! src/share/classes/java/time/Instant.java
! src/share/classes/java/time/LocalDate.java
! src/share/classes/java/time/LocalDateTime.java
! src/share/classes/java/time/LocalTime.java
! src/share/classes/java/time/Month.java
! src/share/classes/java/time/MonthDay.java
! src/share/classes/java/time/OffsetDateTime.java
! src/share/classes/java/time/OffsetTime.java
! src/share/classes/java/time/Period.java
! src/share/classes/java/time/Ser.java
! src/share/classes/java/time/Year.java
! src/share/classes/java/time/YearMonth.java
! src/share/classes/java/time/ZoneId.java
! src/share/classes/java/time/ZoneOffset.java
! src/share/classes/java/time/ZoneRegion.java
! src/share/classes/java/time/ZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoDateImpl.java
! src/share/classes/java/time/chrono/ChronoLocalDate.java
! src/share/classes/java/time/chrono/ChronoLocalDateTime.java
! src/share/classes/java/time/chrono/ChronoLocalDateTimeImpl.java
! src/share/classes/java/time/chrono/ChronoZonedDateTime.java
! src/share/classes/java/time/chrono/ChronoZonedDateTimeImpl.java
! src/share/classes/java/time/chrono/Chronology.java
! src/share/classes/java/time/chrono/Era.java
! src/share/classes/java/time/chrono/HijrahChronology.java
! src/share/classes/java/time/chrono/HijrahDate.java
! src/share/classes/java/time/chrono/HijrahEra.java
! src/share/classes/java/time/chrono/IsoChronology.java
! src/share/classes/java/time/chrono/IsoEra.java
! src/share/classes/java/time/chrono/JapaneseChronology.java
! src/share/classes/java/time/chrono/JapaneseDate.java
! src/share/classes/java/time/chrono/JapaneseEra.java
! src/share/classes/java/time/chrono/MinguoChronology.java
! src/share/classes/java/time/chrono/MinguoDate.java
! src/share/classes/java/time/chrono/MinguoEra.java
! src/share/classes/java/time/chrono/Ser.java
! src/share/classes/java/time/chrono/ThaiBuddhistChronology.java
! src/share/classes/java/time/chrono/ThaiBuddhistDate.java
! src/share/classes/java/time/chrono/ThaiBuddhistEra.java
- src/share/classes/java/time/format/DateTimeFormatSymbols.java
! src/share/classes/java/time/format/DateTimeFormatter.java
! src/share/classes/java/time/format/DateTimeFormatterBuilder.java
! src/share/classes/java/time/format/DateTimeParseContext.java
! src/share/classes/java/time/format/DateTimeParseException.java
! src/share/classes/java/time/format/DateTimePrintContext.java
! src/share/classes/java/time/format/DateTimeTextProvider.java
+ src/share/classes/java/time/format/DecimalStyle.java
! src/share/classes/java/time/format/FormatStyle.java
! src/share/classes/java/time/format/Parsed.java
! src/share/classes/java/time/format/ResolverStyle.java
! src/share/classes/java/time/format/SignStyle.java
! src/share/classes/java/time/format/TextStyle.java
! src/share/classes/java/time/format/package-info.java
! src/share/classes/java/time/temporal/ChronoField.java
! src/share/classes/java/time/temporal/ChronoUnit.java
! src/share/classes/java/time/temporal/IsoFields.java
! src/share/classes/java/time/temporal/JulianFields.java
! src/share/classes/java/time/temporal/Temporal.java
! src/share/classes/java/time/temporal/TemporalAccessor.java
! src/share/classes/java/time/temporal/TemporalAdjuster.java
! src/share/classes/java/time/temporal/TemporalAmount.java
! src/share/classes/java/time/temporal/TemporalField.java
! src/share/classes/java/time/temporal/TemporalQuery.java
! src/share/classes/java/time/temporal/TemporalUnit.java
! src/share/classes/java/time/temporal/UnsupportedTemporalTypeException.java
! src/share/classes/java/time/temporal/ValueRange.java
! src/share/classes/java/time/temporal/WeekFields.java
! src/share/classes/java/time/zone/Ser.java
! src/share/classes/java/time/zone/ZoneOffsetTransition.java
! src/share/classes/java/time/zone/ZoneOffsetTransitionRule.java
! src/share/classes/java/time/zone/ZoneRules.java
! src/share/classes/java/time/zone/ZoneRulesException.java
! src/share/classes/java/time/zone/ZoneRulesProvider.java
! src/share/classes/java/util/JapaneseImperialCalendar.java
! src/share/classes/sun/util/calendar/LocalGregorianCalendar.java
! test/java/time/tck/java/time/TCKInstant.java
! test/java/time/tck/java/time/TCKLocalTime.java
! test/java/time/tck/java/time/TCKOffsetTime.java
! test/java/time/tck/java/time/TCKYear.java
! test/java/time/tck/java/time/TCKYearMonth.java
! test/java/time/tck/java/time/TCKZoneOffset.java
! 

RFR [8014657] CheckedInputStream.skip allocates temporary buffer on every call

[Ivan Gerasimov]

Hello!

Please have a chance to review a simple change proposal.

CheckedInputStream.skip() allocates temporary buffer on every call.
It's suggested to have a temporary buffer that is initialized on the 
first use and can be reused during subsequent calls to the skip() function.

Many other input streams already use the same approach.

http://cr.openjdk.java.net/~dmeetry/8014657/webrev.0/ 
http://cr.openjdk.java.net/%7Edmeetry/8014657/webrev.0/


Sincerely,
Ivan

Re: RFR [8014657] CheckedInputStream.skip allocates temporary buffer on every call

[Xueming Shen]

 public long skip(long n) throws IOException {
-byte[] buf = new byte[512];
+if (tmpbuf == null) {
+tmpbuf = new byte[512];
+}
 long total = 0;
 while (total  n) {
 long len = n - total;
-len = read(buf, 0, len  buf.length ? (int)len : buf.length);
+len = read(buf, 0, len  tmpbuf.length ? (int)len : tmpbuf.length);
 if (len == -1) {
 return total;
 }
 total += len;
 }

Shouldn't the  first param buf to be tmpbuf as well at ln#104, otherwise I 
guess
it will not pass the compiler?

-Sherman

On 05/15/2013 08:40 AM, Ivan Gerasimov wrote:

Hello!

Please have a chance to review a simple change proposal.

CheckedInputStream.skip() allocates temporary buffer on every call.
It's suggested to have a temporary buffer that is initialized on the first use 
and can be reused during subsequent calls to the skip() function.
Many other input streams already use the same approach.

http://cr.openjdk.java.net/~dmeetry/8014657/webrev.0/ 
http://cr.openjdk.java.net/%7Edmeetry/8014657/webrev.0/

Sincerely,
Ivan

Re: Code review request for JDK-8014365 Restore Objects.requireNonNull(T, SupplierString)

[Joe Darcy]

On 05/14/2013 06:32 AM, Alan Bateman wrote:

On 10/05/2013 22:01, Joe Darcy wrote:

Hello,

Please (re)review this change to introduce Objects.requireNonNull(T, 
SupplierString):


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

The original change had to be pulled out because of a build issue 
(8012343: Objects.requireNonNull(Object,Supplier) breaks genstubs 
build); I'll be asking for a review on build-dev of the build-related 
change in langtools. The test portion of the patch is slightly 
different than before because of the intervening changes made for


8013712: Add Objects.nonNull and Objects.isNull
I realize this has already been pushed but just to point out a missing 
parenthesis on line 272 in the javadoc, needs to be )}.




Sorry for introduce the javadoc issue.

Please review this patch

--- a/src/share/classes/java/util/Objects.javaMon May 13 22:16:55 
2013 -0700
+++ b/src/share/classes/java/util/Objects.javaWed May 15 09:43:16 
2013 -0700

@@ -269,7 +269,7 @@
  * Checks that the specified object reference is not {@code null} and
  * throws a customized {@link NullPointerException} if it is.
  *
- * pUnlike the method {@link requireNonNull(Object, String},
+ * pUnlike the method {@link #requireNonNull(Object, String)},
  * this method allows creation of the message to be deferred until
  * after the null check is made. While this may confer a
  * performance advantage in the non-null case, when deciding to

and I'll file a bug a push the fix.

Thanks,

-Joe

Re: Code review request for JDK-8014365 Restore Objects.requireNonNull(T, SupplierString)

[Alan Bateman]

On 15/05/2013 17:44, Joe Darcy wrote:

:

Sorry for introduce the javadoc issue.

Please review this patch

--- a/src/share/classes/java/util/Objects.javaMon May 13 22:16:55 
2013 -0700
+++ b/src/share/classes/java/util/Objects.javaWed May 15 09:43:16 
2013 -0700

@@ -269,7 +269,7 @@
  * Checks that the specified object reference is not {@code null} 
and

  * throws a customized {@link NullPointerException} if it is.
  *
- * pUnlike the method {@link requireNonNull(Object, String},
+ * pUnlike the method {@link #requireNonNull(Object, String)},
  * this method allows creation of the message to be deferred until
  * after the null check is made. While this may confer a
  * performance advantage in the non-null case, when deciding to

and I'll file a bug a push the fix.


Looks good to me.

-Alan.

hg: jdk8/tl/jdk: 8014677: Correct docs warning for Objects.requireNonNull(T, SupplierString)

[joe . darcy]

Changeset: bad8f5237f10
Author:darcy
Date:  2013-05-15 09:54 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/bad8f5237f10

8014677: Correct docs warning for Objects.requireNonNull(T, SupplierString)
Reviewed-by: alanb

! src/share/classes/java/util/Objects.java

Re: Code review request for JDK-8014365 Restore Objects.requireNonNull(T, SupplierString)

[Mike Duigou]

Looks fine.

Mike

On May 15 2013, at 09:44 , Joe Darcy wrote:

 On 05/14/2013 06:32 AM, Alan Bateman wrote:
 On 10/05/2013 22:01, Joe Darcy wrote:
 Hello,
 
 Please (re)review this change to introduce Objects.requireNonNull(T, 
 SupplierString):
 
http://cr.openjdk.java.net/~darcy/8014365.0/
 
 The original change had to be pulled out because of a build issue (8012343: 
 Objects.requireNonNull(Object,Supplier) breaks genstubs build); I'll be 
 asking for a review on build-dev of the build-related change in langtools. 
 The test portion of the patch is slightly different than before because of 
 the intervening changes made for
 
8013712: Add Objects.nonNull and Objects.isNull
 I realize this has already been pushed but just to point out a missing 
 parenthesis on line 272 in the javadoc, needs to be )}.
 
 
 Sorry for introduce the javadoc issue.
 
 Please review this patch
 
 --- a/src/share/classes/java/util/Objects.javaMon May 13 22:16:55 2013 
 -0700
 +++ b/src/share/classes/java/util/Objects.javaWed May 15 09:43:16 2013 
 -0700
 @@ -269,7 +269,7 @@
  * Checks that the specified object reference is not {@code null} and
  * throws a customized {@link NullPointerException} if it is.
  *
 - * pUnlike the method {@link requireNonNull(Object, String},
 + * pUnlike the method {@link #requireNonNull(Object, String)},
  * this method allows creation of the message to be deferred until
  * after the null check is made. While this may confer a
  * performance advantage in the non-null case, when deciding to
 
 and I'll file a bug a push the fix.
 
 Thanks,
 
 -Joe

hg: jdk8/tl/langtools: 8006879: Detection of windows in sjavac fails.

[jonathan . gibbons]

Changeset: 445b8b5ae9f4
Author:jjg
Date:  2013-05-15 10:39 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/445b8b5ae9f4

8006879: Detection of windows in sjavac fails.
Reviewed-by: jjg
Contributed-by: erik.joels...@oracle.com

! src/share/classes/com/sun/tools/sjavac/server/CompilerThread.java

Re: RFR: JDK-8011136 - FileInputStream.available and skip inconsistencies

[Dan Xu]

On 05/13/2013 06:25 AM, Alan Bateman wrote:

On 10/05/2013 22:20, Dan Xu wrote:

Hi,

The FileInputStream.available() method can return a negative value 
when the file position is beyond the endof afile. This is an 
unspecified behaviour that has the potential to break applications 
that expect available to return a value of 0 or greater. The 
FileInputStream.skip(long) method allows a negative value as its 
parameter. This conflicts with the specifications of InputStream and 
FileInputStream classes.


They are both long standing behaviours. In the fix, available() has 
been changed to only return 0 or positive values. And for the 
skip(long) method, due to the compatibility concern, its behaviour 
will not be changed. Instead, the related java specs are going to be 
changed to describe its current behaviour.


bug: http://bugs.sun.com/view_bug.do?bug_id=8011136
webrev: http://cr.openjdk.java.net/~dxu/8011136/webrev.00/

Thanks for your review!

-Dan
Thanks for following up on this one. Overall I agree with the 
approach, it specifies skip to match long standing behavior and fixes 
available to not return negative values.


Just on wording, it might be better if the new statement in 
InputStream.skip didn't start with But. How about Subclasses may 
... as this would be consistent with exiting wording in this class.


For FileInputStream then the statement on how available behaves 
should probably move to the available javadoc. Something like Returns 
0 when the file position is beyond EOF should be fine.


-Alan.




Thanks for your review, I have updated wordings in the java doc. The new 
webrev can be reviewed at 
http://cr.openjdk.java.net/~dxu/8011136/webrev.01/ 
http://cr.openjdk.java.net/%7Edxu/8011136/webrev.01/.


-Dan

Re: RFR: JDK-8013900: More warnings compiling jaxp.

[huizhe wang]

Excellent work, Daniel!

The only minor problem for me is that I can't apply the patch directly 
to jaxp standalone since it uses JDK7 feature. But then we probably 
don't need to do it after all.


Thanks,
Joe

On 5/15/2013 2:42 AM, Daniel Fuchs wrote:

Hi,

Please find below a revised version of the fix for
   JDK-8013900: More warnings compiling jaxp.
http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.01/

Difference with the previous version [1] are in LocalVariableGen.java,
plus 4 additional files:
   BranchInstruction.java
   CodeExceptionGen.java
   LineNumberGen.java
   Select.java

This new set of changes fixes an additional issue I discovered in
LocalVariableGen.java - while running the JCK tests.

LocalVariableGen had a bug that was hidden by the fact
that hashCode() was not compliant with equals().
Changing hashCode() to be compliant with equals() uncovered
that issue.

The issue with LocalVariableGen is that the instance
variables used by equals/hashCode are mutable.
This is an issue because instances of LocalVariableGen are
stored in an HashSet (targeters) held from instances of
InstructionHandle.

Whenever the instruction handles in LocalVariableGen are changed,
then the instance of LocalVariableGen was removed from the 'old'
instruction handle targeters HashSet, and added to the 'new'
instruction handle targeters HashSet - (see LocalVariableGen
updateTargets(..), LocalVariableGen.setStart(...) 
LocalVariableGen.setEnd(...).

The issue here was that the instance of LocalVariableGen was
removed from the 'old' instruction handle targeters HashSet
before being modified (which was correct) - but was also
added to the 'new' instruction handle targeters HashSet
before being modified - which was incorrect because at
that time the hashCode() was still computed using the 'old'
instruction handle, and therefore had its 'old' value.
(this was done by BranchInstruction.notifyTarget(...))

The fix for this new issue is to split
BranchInstruction.notifyTarget(...) in two methods:
BranchInstruction.notifyTargetChanging(...) which is called
before the instruction handle pointer is changed, and remove
the LocalVariableGen from the old instruction handle targeters
HashSet, and BranchInstruction.notifyTargetChanged(...), which
is called after the instruction handle pointer is changed,
and adds the LocalVariableGen to the new instruction handle
targeters HashSet.
In LocalVariableGen - whenever one of the instruction handles
that take parts in the hashCode computation is changed, we
unregister 'this' from all targeters HashSets in which it
is registered, then modify the instruction handle pointer,
then add back 'this' to all targeters HashSets in which it
needs to be registered.

The 4 additional files in the webrev were also calling
BranchInstruction.notifyTarget - and I modified them to
call the two new methods instead of the old one - but without
going through the trouble of unregistering 'this' from any
known HashSet before modifictation - and adding it after,
since those other classes do not have mutable hashCode().

Note: I also took this opportunity to change the method
calling BranchInstruction.notifyTargetXxxx to be final, as
I think it's desirable that they not be overridden, and to
make the index in LocalVariableGen final - since it was
never changed after the instance construction (and takes
part in the HashCode computation).

best regards,

-- daniel

previous version:
[1] http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

On 5/13/13 4:36 PM, Daniel Fuchs wrote:

This is a fix for JDK-8013900: More warnings compiling jaxp.

http://cr.openjdk.java.net/~dfuchs/JDK-8013900/webrev.00/

Although the title might suggest a trivial fix, it goes a bit
beyond a simple warning fix because the root cause of those warnings
is that some classes redefine equals without redefining
hashCode() - and devising a hashCode() method that respects
its contract with equals() is not always trivial.

The proposed fix adds hashCode() methods where necessary, ensuring
that:

if (o1.equals(o2) == true), then (o1.hashCode() == o2.hashCode())

The fix also contains some cosmetic/sanity changes - like:

1. adding @Override wherever NetBeans complained they were
missing - and
2. replacing StringBuffer with StringBuilder when
possible.
3. In one instance, AbstractDateTimeDV.java I also had
to reformat the whole file (due to its weird original formatting)
- apologies for the noise it causes in the webrev.
4. I also removed a couple of private isEqual(obj1, obj2) methods
replacing them by calls to Objects.equals(obj1, obj2) which did
the same thing.
5. finally I refactored some of the existing equals (e.g. replacing
try { (Sometype) other } catch (ClassCastException x) {
return false; } with use of 'other instanceof Sometype'...)

There are however a couple of more serious changes that could
deserve to be reviewed in more details:

a. The new implementation of hashCode() in

Re: bug 8014477 Race in String.contentEquals

[Mike Duigou]

Hi Peter;

I tried it on my i7 box and was able to get it to fail right away. I didn't try 
the previous test on this box so can't say whether it would have also failed.

Applying the String changes resolved the issue. I think this one is ready!

Mike

On May 15 2013, at 00:50 , Peter Levart wrote:

 Mike, David,
 
 Could you try this variant of the test:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.05/
 
 
 I tried it on 3 different machines (4-core i7 Linux, 8-core sparc T-2 
 Solaris, 4-core amd64 Solaris) x 2 different JDK8 JVMs (64bit and 32bit) and 
 it fails immediately on all 6 of them.
 
 Regards, Peter
 
 On 05/14/2013 05:39 PM, Mike Duigou wrote:
 Good to see the test added. I was also unable to reproduce the failure on my 
 Core 2 Duo Mac laptop but the test and fix match up.
 
 Mike
 
 
 On May 14 2013, at 04:34 , Peter Levart wrote:
 
 On 05/14/2013 01:17 PM, David Holmes wrote:
 Thanks Peter this looks good to me.
 
 One minor thing please set the correct copyright year in the test, it 
 should just be
 
 Copyright (c) 2013, Oracle ...
 Ok, fixed:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.04/
 
 
 I couldn't get the test to actually fail, but I can see that it could.
 Hm, do you have a multi-core chip? On my computer (i7 CPU) it fails 
 immediately and always. You could try varying the number of concurrent 
 threads and or the time to wait for exception to be thrown...
 
 David
 
 On 14/05/2013 9:04 PM, Peter Levart wrote:
 Ok, here's the corrected fix:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.03/
 
 I also added a reproducer for the bug.
 
 Regards, peter
 
 On 05/14/2013 07:53 AM, Peter Levart wrote:
 Right, sb.length() should be used. I will correct that as soon as I
 get to the keyboard.
 
 Regards, Peter
 
 On May 14, 2013 7:22 AM, David Holmes david.hol...@oracle.com
 mailto:david.hol...@oracle.com wrote:
 
Thanks Peter! I've filed
 
8014477
Race condition in String.contentEquals when comparing with
StringBuffer
 
On 14/05/2013 8:34 AM, Peter Levart wrote:
 
On 05/14/2013 12:09 AM, Peter Levart wrote:
 
I noticed a synchronization bug in String.contentEquals
method. If
called with a StringBuffer argument while concurrent thread is
modifying the StringBuffer, the method can either throw
ArrayIndexOutOfBoundsException or return true even though
the content
of the StringBuffer has never been the same as the String's.
 
Here's a proposed patch:
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.01/
 http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.01/
 
Regards, Peter
 
 
Or even better (with some code clean-up):
 
 http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.02/
 http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.02/
 
 
This part is not correct I believe:
 
1010 private boolean
nonSyncContentEquals(AbstractStringBuilder sb) {
1011 char v1[] = value;
1012 char v2[] = sb.getValue();
1013 int n = v1.length;
1014 if (n != v2.length) {
1015 return false;
1016 }
 
v2 can be larger than v1 if v2 is not being fully used (ie count 
length).
 
David
-
 
 

Re: RFR [8014657] CheckedInputStream.skip allocates temporary buffer on every call

[Ivan Gerasimov]

Hello Sherman!

On 15.05.2013 20:34, Xueming Shen wrote:

public long skip(long n) throws IOException {
-byte[] buf = new byte[512];
+if (tmpbuf == null) {
+tmpbuf = new byte[512];
+}
 long total = 0;
 while (total  n) {
 long len = n - total;
-len = read(buf, 0, len  buf.length ? (int)len : 
buf.length);
+len = read(buf, 0, len  tmpbuf.length ? (int)len : 
tmpbuf.length);

 if (len == -1) {
 return total;
 }
 total += len;
 }

Shouldn't the  first param buf to be tmpbuf as well at ln#104, 
otherwise I guess

it will not pass the compiler?


Of course you're right!
I should have waited for the compilation to finish before posting the 
message.
Here's the updated webrev: 
http://cr.openjdk.java.net/~dmeetry/8014657/webrev.1/ 
http://cr.openjdk.java.net/%7Edmeetry/8014657/webrev.1/


Thanks,
Ivan



-Sherman

On 05/15/2013 08:40 AM, Ivan Gerasimov wrote:

Hello!

Please have a chance to review a simple change proposal.

CheckedInputStream.skip() allocates temporary buffer on every call.
It's suggested to have a temporary buffer that is initialized on the 
first use and can be reused during subsequent calls to the skip() 
function.

Many other input streams already use the same approach.

http://cr.openjdk.java.net/~dmeetry/8014657/webrev.0/ 
http://cr.openjdk.java.net/%7Edmeetry/8014657/webrev.0/


Sincerely,
Ivan

RFR : 8007398 : (S) Performance improvements for Int/Long toString() at Radix 2, 8, 16

[Mike Duigou]

Hello all;

This issue was originally part of JDK-8006627 (improve performance of UUID 
parsing/formatting) but was split out because it could be split out. I've been 
working incrementally on pieces of 8006627 as my time permits.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/1/webrev/

I've done microbenchmarking of using digits table vs calculating digits, 
previously mentioned in 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/016526.html, 
and found that using the digits table was still faster.

I've also benchmarked removing the offset param from formatUnsignedLong() and 
simplifying the loop termination logic but neither of these turned out to have 
little benefit.

Since the last rev I have made a separate implementation 
Integer.formatUnsignedInteger for use by Integer rather than sharing the Long 
implementation because it's about 30% faster. I suspect the benefits would be 
even greater on a 32-bit VM or 32-bit native platform.

We'll get back to 8006627 soon enough. (I have been tempted to split it again 
into parsing and formatting).

Thanks,

Mike

hg: jdk8/tl/jdk: 5 new changesets

[valerie . peng]

Changeset: 2ec31660cc0e
Author:valeriep
Date:  2013-05-07 14:04 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2ec31660cc0e

8010134: A finalizer in sun.security.pkcs11.wrapper.PKCS11 perhaps should be 
protected
Summary: Change the finalize method of PKCS11 class to be protected.
Reviewed-by: xuelei

! src/share/classes/sun/security/pkcs11/wrapper/PKCS11.java

Changeset: 991420add35d
Author:valeriep
Date:  2013-05-07 14:06 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/991420add35d

7196009: SunPkcs11 provider fails to parse config path containing parenthesis
Summary: Enhanced to allow quoted string as library path values.
Reviewed-by: weijun

! src/share/classes/sun/security/pkcs11/Config.java
! test/sun/security/pkcs11/Provider/ConfigShortPath.java
+ test/sun/security/pkcs11/Provider/cspQuotedPath.cfg

Changeset: 804da1e9bd04
Author:ascarpino
Date:  2013-05-07 14:13 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/804da1e9bd04

8001284: Buffer problems with SunPKCS11-Solaris and CKM_AES_CTR
Summary: Changed output length calculation to include incomplete blocks for CTR 
mode.
Reviewed-by: valeriep

! src/share/classes/sun/security/pkcs11/P11Cipher.java
! test/sun/security/pkcs11/Cipher/TestSymmCiphersNoPad.java

Changeset: fc70416beef3
Author:valeriep
Date:  2013-05-13 16:52 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/fc70416beef3

Merge

- make/com/sun/script/Makefile
- make/sun/org/Makefile
- make/sun/org/mozilla/Makefile
- make/sun/org/mozilla/javascript/Makefile
- src/share/classes/com/sun/script/javascript/ExternalScriptable.java
- src/share/classes/com/sun/script/javascript/JSAdapter.java
- src/share/classes/com/sun/script/javascript/JavaAdapter.java
- 
src/share/classes/com/sun/script/javascript/META-INF/services/javax.script.ScriptEngineFactory
- src/share/classes/com/sun/script/javascript/RhinoClassShutter.java
- src/share/classes/com/sun/script/javascript/RhinoCompiledScript.java
- src/share/classes/com/sun/script/javascript/RhinoScriptEngine.java
- src/share/classes/com/sun/script/javascript/RhinoScriptEngineFactory.java
- src/share/classes/com/sun/script/javascript/RhinoTopLevel.java
- src/share/classes/com/sun/script/javascript/RhinoWrapFactory.java
- src/share/classes/com/sun/script/util/BindingsBase.java
- src/share/classes/com/sun/script/util/BindingsEntrySet.java
- src/share/classes/com/sun/script/util/BindingsImpl.java
- src/share/classes/com/sun/script/util/InterfaceImplementor.java
- src/share/classes/com/sun/script/util/ScriptEngineFactoryBase.java
- src/share/classes/java/beans/ReflectionUtils.java
- 
test/java/awt/Focus/OverrideRedirectWindowActivationTest/OverrideRedirectWindowActivationTest.java
- test/sun/security/provider/certpath/X509CertPath/ForwardBuildCompromised.java
- test/sun/security/provider/certpath/X509CertPath/ReverseBuildCompromised.java
- test/sun/security/provider/certpath/X509CertPath/ValidateCompromised.java

Changeset: 59357ea7f131
Author:valeriep
Date:  2013-05-15 18:38 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/59357ea7f131

Merge

- src/share/classes/java/time/format/DateTimeFormatSymbols.java
- 
src/share/classes/sun/nio/cs/ext/META-INF/services/java.nio.charset.spi.CharsetProvider
- test/java/time/tck/java/time/format/TCKDateTimeFormatSymbols.java
- test/java/time/test/java/time/format/TestDateTimeFormatSymbols.java

Re: RFR : 8007398 : (S) Performance improvements for Int/Long toString() at Radix 2, 8, 16

[Joseph Darcy]

Hi Mike,

Looks fine. Are you satisfied with the test coverage provided by the 
existing regression tests?


-Joe

On 5/15/2013 6:17 PM, Mike Duigou wrote:

Hello all;

This issue was originally part of JDK-8006627 (improve performance of UUID 
parsing/formatting) but was split out because it could be split out. I've been 
working incrementally on pieces of 8006627 as my time permits.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/1/webrev/

I've done microbenchmarking of using digits table vs calculating digits, previously 
mentioned in 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-April/016526.html, 
and found that using the digits table was still faster.

I've also benchmarked removing the offset param from formatUnsignedLong() and 
simplifying the loop termination logic but neither of these turned out to have 
little benefit.

Since the last rev I have made a separate implementation 
Integer.formatUnsignedInteger for use by Integer rather than sharing the Long 
implementation because it's about 30% faster. I suspect the benefits would be 
even greater on a 32-bit VM or 32-bit native platform.

We'll get back to 8006627 soon enough. (I have been tempted to split it again 
into parsing and formatting).

Thanks,

Mike

Re: RFR: 8013380 - Removal of stack walk to find resource bundle breaks Glassfish startup

[Mandy Chung]

On 5/15/2013 2:19 PM, Jim Gish wrote:

Please review http://cr.openjdk.java.net/~jgish/TestRB.7.1/


Looks fine.  This fix gets the Glassfish to run on jdk8 as an interim 
fix while allowing us to investigate a proper solution for jdk8.


Daniel mentioned the performance overhead of Reflection.getCallerClass() 
offline that does incur some overhead. Applications that create logger 
with no resource bundle likely call Logger.getLogger(String name) 
instead of Logger.getLogger(String name, String rbname).  In other 
words, when Logger.getLogger(name, rbname) is called, it's likely that 
rbname is non-null.   It might incur some performance overhead to 
applications who resource bundle is visible to TCCL or system class 
loader as Logger.getLogger(String, String) always obtains the immediate 
caller but not used.  In Glassfish and OSGi environment, there is no 
performance issue since it has been doing the stack walk in the past.  I 
think it's fine as it is.


Nits: L1639, 1712 - better to align with the line above.

Thanks for extending the test to cover various cases.

Thanks
Mandy

This is an update to the previous webrev.  This is a temporary fix to 
workaround a regression that causes Glassfish 4.0 to fail to startup.  
A proper fix will be investigated.


Thanks,
Jim

On 04/30/2013 08:13 PM, Jim Gish wrote:

Here's an update:
http://cr.openjdk.java.net/~jgish/TestRB.3/ 
http://cr.openjdk.java.net/%7Ejgish/TestRB.3/


Thanks,
Jim

On 04/30/2013 05:10 PM, Jim Gish wrote:


On 04/30/2013 04:29 PM, Alan Bateman wrote:

On 30/04/2013 17:48, Jim Gish wrote:
Please review http://cr.openjdk.java.net/~jgish/TestRB.2/ 
http://cr.openjdk.java.net/%7Ejgish/TestRB.2/


This fixes a regression caused by the removal of the search of the 
call stack for resource bundles by java.util.logging.  We have 
added a single-level search up the stack, i.e. just the immediate 
caller's ClassLoader, as an additional alternative to the 
specified method of using the thread context classloader if set 
and the system classloader if the TCCL is not set.  This is 
intended to handle cases such as Glassfish or other OSGi-based 
apps/3rd-party libs for which setting the TCCL is not feasible.
It's unfortunate that the stack walk was masking this issue. Are 
you planning an update to the javadoc to reconcile the spec vs. 
impl difference?



Yes

-Alan.

Re: bug 8014477 Race in String.contentEquals

[David Holmes]

Okay mea culpa - I was testing on the wrong JDK. I wrongly assumed this 
would impact 7 as well but it doesn't as the bug was introduced with the 
changes in 6914123.


Sorry about that.

Good to go. Has anyone offered to sponsor this yet?

David
-

On 15/05/2013 5:50 PM, Peter Levart wrote:

Mike, David,

Could you try this variant of the test:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.05/


I tried it on 3 different machines (4-core i7 Linux, 8-core sparc T-2
Solaris, 4-core amd64 Solaris) x 2 different JDK8 JVMs (64bit and 32bit)
and it fails immediately on all 6 of them.

Regards, Peter

On 05/14/2013 05:39 PM, Mike Duigou wrote:

Good to see the test added. I was also unable to reproduce the failure
on my Core 2 Duo Mac laptop but the test and fix match up.

Mike


On May 14 2013, at 04:34 , Peter Levart wrote:


On 05/14/2013 01:17 PM, David Holmes wrote:

Thanks Peter this looks good to me.

One minor thing please set the correct copyright year in the test,
it should just be

Copyright (c) 2013, Oracle ...

Ok, fixed:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.04/




I couldn't get the test to actually fail, but I can see that it could.

Hm, do you have a multi-core chip? On my computer (i7 CPU) it fails
immediately and always. You could try varying the number of
concurrent threads and or the time to wait for exception to be thrown...


David

On 14/05/2013 9:04 PM, Peter Levart wrote:

Ok, here's the corrected fix:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.03/


I also added a reproducer for the bug.

Regards, peter

On 05/14/2013 07:53 AM, Peter Levart wrote:

Right, sb.length() should be used. I will correct that as soon as I
get to the keyboard.

Regards, Peter

On May 14, 2013 7:22 AM, David Holmes david.hol...@oracle.com
mailto:david.hol...@oracle.com wrote:

Thanks Peter! I've filed

8014477
Race condition in String.contentEquals when comparing with
StringBuffer

On 14/05/2013 8:34 AM, Peter Levart wrote:

On 05/14/2013 12:09 AM, Peter Levart wrote:

I noticed a synchronization bug in String.contentEquals
method. If
called with a StringBuffer argument while concurrent
thread is
modifying the StringBuffer, the method can either throw
ArrayIndexOutOfBoundsException or return true even though
the content
of the StringBuffer has never been the same as the
String's.

Here's a proposed patch:

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.01/

http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.01/


Regards, Peter


Or even better (with some code clean-up):

http://cr.openjdk.java.net/~plevart/jdk8-tl/String.contentEquals/webrev.02/

http://cr.openjdk.java.net/%7Eplevart/jdk8-tl/String.contentEquals/webrev.02/



This part is not correct I believe:

1010 private boolean
nonSyncContentEquals(AbstractStringBuilder sb) {
1011 char v1[] = value;
1012 char v2[] = sb.getValue();
1013 int n = v1.length;
1014 if (n != v2.length) {
1015 return false;
1016 }

v2 can be larger than v1 if v2 is not being fully used (ie
count 
length).

David
-

Re: Time to put a stop to Thread.stop?

[David Holmes]

On 16/05/2013 12:36 AM, Mario Torre wrote:

2013/5/15 Remi Forax fo...@univ-mlv.fr:

On 05/15/2013 10:39 AM, Martin Desruisseaux wrote:


Le 15/05/13 10:05, David Holmes a écrit :


There is a huge difference between blowing away a complete process with
kill and having a single thread starting to propagate an async exception,
unwinding its stack and executing finally blocks with completely broken
state invariants.



Given the risk, what about a mechanism similar to the one which currently
hides the Sun internal packages from the compilation phase but not yet from
the runtime phase? Would it be acceptable to have 'javac' and 'javadoc'
woking as if the 'Thread.stop(Throwable)' method did not existed anymore,
while having the method still working (for now) at runtime for existing
libraries? Maybe with an annotation yet stronger than @Deprecated.

 Martin



yes, I propose @Retired.


+1

I think it should also blow at runtime unless people passes some fancy
argument (like -XX:recallRetired :), this way is easier for people to
fix their code, or, in case they can't, do workaround it.


Interesting suggestions but I for one do not think that after 15 years 
of deprecation we need to go to such lengths to keep stop(Throwable) on 
life-support - it is @Deceased in my opinion :)


Cheers,
David
-


Cheers,
Mario

--
pgp key: http://subkeys.pgp.net/ PGP Key ID: 80F240CF
Fingerprint: BA39 9666 94EC 8B73 27FA  FC7C 4086 63E3 80F2 40CF

IcedRobot: www.icedrobot.org
Proud GNU Classpath developer: http://www.classpath.org/
Read About us at: http://planet.classpath.org
OpenJDK: http://openjdk.java.net/projects/caciocavallo/

Please, support open standards:
http://endsoftpatents.org/

Re: [PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[David Holmes]

Hi Eric,

Cleanup of threads looks good.

The synchronization between waitForDump and finishDump is a bit dodgy 
but that is a different issue.


Cheers,
David

On 15/05/2013 9:07 PM, Eric Wang wrote:

On 2013/5/15 17:38, Alan Bateman wrote:

On 15/05/2013 10:10, Eric Wang wrote:

Hi,

Please help to review the fix for bug 8004177
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make
sure all child threads finished before main thread exits.

http://cr.openjdk.java.net/~ewang/8004177/webrev.00/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/

For 8004178, the test StackTraces.java is same as
GenerifyStackTraces.java, so just remove it.

Thanks,
Eric

It's good to have fix this test so that it doesn't leave a thread
behind (in this case slowing down the execution of subsequent tests by
taking thread dumps every 2 seconds)

The change you propose is okay but a bit odd to have ThreadOne signal
DumpThread to shutdown. An alternative would be to have the main
thread signal DumpThread, as in:

  one.join();

  finished = true;
  dt.join();

Better still might be to move the flag into the DumpThread class and
have it define a shutdown method so the main thread does:

  one.join();

  dt.shutdown();
  dt.join();

I think that would be a bit cleaner.

-Alan.




Hi Alan,

Thanks for the suggestion, I have updated the fix again, Can you please
help to take a look?
http://cr.openjdk.java.net/~ewang/8004177/webrev.01/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.01/

Regards,
Eric

Re: [PATCH] Review request: 8004177: test/java/lang/Thread/GenerifyStackTraces.java doesn't clean-up

[Eric Wang]

Hi David,

Thanks for review this issue, i'll file another bug to track this.

Regards,
Eric
On 2013/5/16 12:30, David Holmes wrote:

Hi Eric,

Cleanup of threads looks good.

The synchronization between waitForDump and finishDump is a bit dodgy 
but that is a different issue.


Cheers,
David

On 15/05/2013 9:07 PM, Eric Wang wrote:

On 2013/5/15 17:38, Alan Bateman wrote:

On 15/05/2013 10:10, Eric Wang wrote:

Hi,

Please help to review the fix for bug 8004177
https://jbs.oracle.com/bugs/browse/JDK-8004177 and 8004178
https://jbs.oracle.com/bugs/browse/JDK-8004178, this fix is to make
sure all child threads finished before main thread exits.

http://cr.openjdk.java.net/~ewang/8004177/webrev.00/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.00/

For 8004178, the test StackTraces.java is same as
GenerifyStackTraces.java, so just remove it.

Thanks,
Eric

It's good to have fix this test so that it doesn't leave a thread
behind (in this case slowing down the execution of subsequent tests by
taking thread dumps every 2 seconds)

The change you propose is okay but a bit odd to have ThreadOne signal
DumpThread to shutdown. An alternative would be to have the main
thread signal DumpThread, as in:

  one.join();

  finished = true;
  dt.join();

Better still might be to move the flag into the DumpThread class and
have it define a shutdown method so the main thread does:

  one.join();

  dt.shutdown();
  dt.join();

I think that would be a bit cleaner.

-Alan.




Hi Alan,

Thanks for the suggestion, I have updated the fix again, Can you please
help to take a look?
http://cr.openjdk.java.net/~ewang/8004177/webrev.01/
http://cr.openjdk.java.net/%7Eewang/8004177/webrev.01/

Regards,
Eric