Re: RFR: 8022446: Fix serial warnings in java.util.stream

2013-08-07 Thread Paul Sandoz

On Aug 6, 2013, at 10:28 PM, Henry Jen  wrote:

> Hi,
> 
> Please review a webrev[1] clean up on serial warning, as we don't intend
> to serialize those tasks, we simply suppress those warnings.
> 
> It would be nice if there is a way to undo serializable.
> 
> [1] http://cr.openjdk.java.net/~henryjen/tl/8022446/0/webrev/
> 

Looks good to me.

Paul.


Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

2013-08-07 Thread Peter Levart

On 08/07/2013 12:23 AM, Dan Smith wrote:

On Aug 6, 2013, at 2:43 PM, Remi Forax  wrote:


On 08/06/2013 11:11 PM, Dan Smith wrote:

Please review this warnings cleanup.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
visible)
Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/

—Dan

Hi Dan,
I've seen that you have introduce a common super interface for entry and tree 
node,
I suppose that  you check that there is no performance regression.
I wonder if an abstract class is not better than an interface because as far as 
I know
CHA implemented in hotspot doesn't work on interface
(but I may be wrong, there is perhaps a special optimization for arrays).

To make sure I understand: your concern is that an aastore will be more 
expensive when assigning to a KeyValueData[] than to an Object[] (or even to 
SomeOtherClass[])?

For what it's worth, all assignments to table[i] are statically known to be 
safe.  E.g.:

Entry next = (Entry) e.next;
...
table[i] = next;

So surely a smart VM only does the check once?

Here are some other things that might be concerns, but don't apply here:
- interface method invocations: there are no methods in the interface to invoke
- checkcast to an interface: all the casts are to concrete classes (Entry, 
TreeBin, TreeNode)

(There are some unchecked casts from KeyValueData to KeyValueData with 
different type parameters, but I assume these don't cause any checkcasts.)

—Dan

Hi,

FWIW, I compiled old and new HashMap.java and did a javap on the classes.

javap outputs: http://cr.openjdk.java.net/~plevart/misc/hm-8022442/
differences: 
http://cr.openjdk.java.net/~plevart/misc/hm-8022442/hm.javap.stripped.diff


Besides unneeded introduction of local variable discussed already, there 
seem to be 4 new checkcasts in the following locations (new HashMap.java):


  public java.util.HashMap(int, float);
Code:
   0: aload_0
   1: invokespecial #10 // Method 
java/util/AbstractMap."":()V

   4: aload_0
   5: getstatic #11 // Field 
EMPTY_TABLE:[Ljava/util/HashMap$KeyValueData;
*   8: checkcast #12 // class 
"[Ljava/util/HashMap$KeyValueData;"*
  11: putfield  #13 // Field 
table:[Ljava/util/HashMap$KeyValueData;

  14: aload_0
  15: aconst_null
...

  private void inflateTable(int);
Code:
...
  22: aload_0
  23: iload_2
  24: anewarray #44 // class 
java/util/HashMap$KeyValueData
*  27: checkcast #12 // class 
"[Ljava/util/HashMap$KeyValueData;"*
  30: putfield  #13 // Field 
table:[Ljava/util/HashMap$KeyValueData;

  33: return
...

  void resize(int);
Code:
...
  20: return
  21: iload_1
  22: anewarray #44 // class 
java/util/HashMap$KeyValueData
*  25: checkcast #12 // class 
"[Ljava/util/HashMap$KeyValueData;"*

  28: astore4
  30: aload_0
  31: aload 4
...

  private void readObject(java.io.ObjectInputStream) throws 
java.io.IOException, java.lang.ClassNotFoundException;

Code:
...
  82: invokevirtual #141// Method 
sun/misc/Unsafe.putIntVolatile:(Ljava/lang/Object;JI)V

  85: aload_0
  86: getstatic #11 // Field 
EMPTY_TABLE:[Ljava/util/HashMap$KeyValueData;
*  89: checkcast #12 // class 
"[Ljava/util/HashMap$KeyValueData;"*
  92: putfield  #13 // Field 
table:[Ljava/util/HashMap$KeyValueData;

  95: aload_1
  96: invokevirtual #142// Method 
java/io/ObjectInputStream.readInt:()I

...


...but they are all in the infrequently called methods/constructor.


Regards, Peter



hg: jdk8/tl/langtools: 8020997: TreeMaker.AnnotationBuilder creates broken element literals with repeating annotations

2013-08-07 Thread vicente . romero
Changeset: f3deeccbf4cf
Author:vromero
Date:  2013-08-07 10:41 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/f3deeccbf4cf

8020997: TreeMaker.AnnotationBuilder creates broken element literals with 
repeating annotations
Reviewed-by: jjg, jfranck

! src/share/classes/com/sun/tools/javac/tree/TreeMaker.java
+ test/tools/javac/T8020997/CannotCompileRepeatedAnnoTest.java



hg: jdk8/tl/langtools: 8008274: javac should not reference/use sample code

2013-08-07 Thread vicente . romero
Changeset: c7dcf899
Author:vromero
Date:  2013-08-07 11:04 +0100
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/c7dcf899

8008274: javac should not reference/use sample code
Reviewed-by: jjg

! src/share/classes/com/sun/tools/javac/Main.java



hg: jdk8/tl/jdk: 7151062: [macosx] SCDynamicStore prints error messages to stderr

2013-08-07 Thread weijun . wang
Changeset: 906dd23334c1
Author:weijun
Date:  2013-08-07 19:06 +0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/906dd23334c1

7151062: [macosx] SCDynamicStore prints error messages to stderr
Reviewed-by: xuelei

! src/macosx/native/java/util/SCDynamicStoreConfig.m



Re: JVM application shutdown hooks

2013-08-07 Thread Laurent Bourgès
Dear core-libs members,

I finally succeed in diagnosing my shutdown hook issue in Java Web Start
environment: see Bug ID: 9005822.

Could you please give ma feedback to my questions related to LogManager and
StreamCloser shutdown hooks ?


This library uses java.util.logging to log warnings and exceptions but none
> is logged (console or java trace files).
>
> The 'lost' log messages is related to the LogManager's shutdown hook:
> // This private class is used as a shutdown hook.
> // It does a "reset" to close all open handlers.
> private class Cleaner extends Thread {
>
> private Cleaner() {
> /* Set context class loader to null in order to avoid
>  * keeping a strong reference to an application classloader.
>  */
> this.setContextClassLoader(null);
> }
>
> public void run() {
> // This is to ensure the LogManager. is completed
> // before synchronized block. Otherwise deadlocks are possible.
> LogManager mgr = manager;
>
> // If the global handlers haven't been initialized yet, we
> // don't want to initialize them just so we can close them!
> synchronized (LogManager.this) {
> // Note that death is imminent.
> deathImminent = true;
> initializedGlobalHandlers = true;
> }
>
> // Do a reset to close all active handlers.
> reset();
> }
> }
>
> I think it is annoying as it avoids me getting log and exceptions ...
>

Exactly what happened to me in JavaWS env !! I had to patch JSamp library
to add System.out and printStackTrace statements whereas there are plenty
of j.u.l. log statements !!

Please postpone that f*** hook after 'true' application hooks !!!


Moreover, as a RFE, it could be useful for applications to be able to
> define hook priorities within an application:
> I did that several times registering only 1 shutdown hook and handling
> correct ordering on my own ... but could be supported by the JDK itself.
>

Do you want me to submit a patch ? I think that the JVM specs should be
modified to change the addShutdownHook() signature => RFE ?


> Finally, here are (below) the Open JDK8 usages of Runtime.addShutdownHook
> [19 occurrences] which are more system hooks than application hooks:
> com.sun.imageio.stream
> StreamCloser.java
> StreamCloser
> addToQueue
> 
> run
>   101:  Runtime.getRuntime().addShutdownHook(streamCloser);
> java.util.logging
> LogManager.java
> LogManager
> LogManager
>   255:  Runtime.getRuntime().addShutdownHook(new Cleaner());
> sun.font
> SunFontManager.java
> SunFontManager
> createFont2D
> 
> run
> 2538:  Runtime.getRuntime().addShutdownHook(fileCloser);
> sun.rmi.server
> Activation.java
> Activation
> init
>   240:  Runtime.getRuntime().addShutdownHook(shutdownHook);
> sun.tools.jstat
>
> sun.awt.shell
> Win32ShellFolderManager2.java
> Win32ShellFolderManager2
> ComInvoker
> ComInvoker
> >
> run
>   493:  Runtime.getRuntime().addShutdownHook(
> sun.awt.windows
> WToolkit.java
> WToolkit
> registerShutdownHook
> >
> run
>   285:  Runtime.getRuntime().addShutdownHook(shutdown);
> sun.java2d.d3d
> D3DScreenUpdateManager.java
> D3DScreenUpdateManager
> D3DScreenUpdateManager
> 
> run
>   112:  Runtime.getRuntime().addShutdownHook(shutdown);
> java.util.prefs
> FileSystemPreferences.java
> FileSystemPreferences
> >
> run
>   439:  Runtime.getRuntime().addShutdownHook(new Thread() {
> sun.awt.X11
> XToolkit.java
> XToolkit
> init
> >
> run
>   350:  Runtime.getRuntime().addShutdownHook(shutdownThread);
> sun.awt
> X11GraphicsDevice.java
> X11GraphicsDevice
> setDisplayMode
> >
> run
>   445:  Runtime.getRuntime().addShutdownHook(t);
> java.util.prefs
> MacOSXPreferencesFile.java
> MacOSXPreferencesFile
> timer
>   356:  Runtime.getRuntime().addShutdownHook(flushThread);
> sun.font
> CFontManager.java
> CFontManager
> createFont2D
> >
> run
>   232:  Runtime.getRuntime().addShutdownHook(fileCloser);
> sun.lwawt
> LWToolkit.java
> LWToolkit
> init
> 83:  Runtime.getRuntime().addShutdownHook(
>
> I hope that these hooks are already supporting concurrency execution (awt
> ?) but someone should check if there is no dependencies between them (like
> systemd or rc scripts does).
>

Could someone check if there is no dependencies between all these shutdown
hooks when they are executed in parallel with random order ?

Best regards,
Laurent


Re: RFR [6961766]: PrintStream.write() should flush at most once

2013-08-07 Thread David Holmes

Ivan,

On 7/08/2013 3:08 PM, Alan Bateman wrote:

On 06/08/2013 09:38, Ivan Gerasimov wrote:

Hello everybody!

Would you please review a simple fix, suggested by someone on
bugs.sun.com?

j.i.PrintStream#write(char[]) calls out.flush() on each '\n' character
found in the array.
It's sufficient to do it only once.

One thing to check is whether this will require any changes to the spec
(as it is currently specified to call flush when a newline is written).
I don't have time just at the moment to check this but that would be my
only concern with a change like this.


Yes - this is NOT A BUG this is the spec for this class:

"Optionally, a PrintStream can be created so as to flush automatically; 
this means that the flush method is automatically invoked after a byte 
array is written, one of the println methods is invoked, or a newline 
character or byte ('\n') is written. "


This bug report should be closed as "not an issue".

David
-



-Alan


hg: jdk8/tl/jdk: 8022483: Nashorn compatibility issues in jhat's OQL feature

2013-08-07 Thread sundararajan . athijegannathan
Changeset: 99f4319763a9
Author:sundar
Date:  2013-08-07 18:16 +0530
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/99f4319763a9

8022483: Nashorn compatibility issues in jhat's OQL feature
Reviewed-by: lagergren, attila, hannesw

! src/share/classes/com/sun/tools/hat/resources/hat.js
! src/share/classes/com/sun/tools/hat/resources/oqlhelp.html



Re: RFR 8021820: Number of opened files used in select() is limited to 1024 [macosx]

2013-08-07 Thread Aleksej Efimov

Stuart, thank you for you comments, responses are below.
New webrev:
http://cr.openjdk.java.net/~aefimov/8021820/webrev.02/ 




-Aleksej

On 08/06/2013 05:14 AM, Stuart Marks wrote:

Hi Aleksej,

Thanks for the update. I took a look at the revised test, and there 
are still some issues. (I didn't look at the build changes.)


1) System-specific resource limits.

I think the biggest issue is resource limits on the number of open 
files per process that might vary from system to system. On my Ubuntu 
system, the hard limit on the number of open files is 1,024. The test 
opens 1,023 files and then one more for the socket. Unfortunately the 
JVM and jtreg have several files open already, and the test crashes 
before the openFiles() method completes.


(Oddly it crashes with a NoClassDefFoundError from the main thread's 
uncaught exception handler, and then the test reports that it passed! 
Placing a try/catch of Throwable in main() or openFiles() doesn't 
catch this error. I have no explanation for this. When run standalone 
-- i.e., not from jtreg -- the test throws FileNotFoundException (too 
many open files) from openFiles(), which is expected.)


On my Mac (10.7.5) the soft limit is 256 files, but the hard limit is 
unlimited. The test succeeds in opening all its files but fails 
because of the select() bug you're fixing. (This is expected; I didn't 
rebuild my JDK with your patch.) I guess the soft limit doesn't do 
anything on Mac.


Amazingly, the test passed fine on both Windows XP and Windows 8.

I'm not entirely sure what to do about resource limits. Since the test 
is able to open >1024 files on Mac, Windows, and possibly other 
Linuxes, it seems reasonable to continue with this approach. If it's 
possible to catch the error that occurs if the test cannot open its 
initial 1,024 files, perhaps it should do this, log a message 
indicating what happened, and consider the test to have passed. I'm 
mystified by the uncaught/uncatchable NoClassDefFoundError though.
I wonder if this is a question of test environment required for JTREG 
tests: if we'll execute JTREG tests with low value assigned to fd hard 
limit (for example 10) we'll see a lot of unrelated test failures. So, I 
suggest that we can assume that there is no hard limits set (or at least 
default ones, i.e. default fd limit on Ubuntu is 4096) on test machine. 
But we should consider test as Failed if test failed to prepare it's 
environment because of some external limitations. The JTREG doesn't meet 
this criteria (log test as PASS and prints incorrect Exception). To 
illustrate it I have repeated your experiments on ubuntu linux: set fd 
hard limit to 1024 (ulimit -Hn 1024) and got this error by manual run of 
test:
Exception in thread "main" java.io.FileNotFoundException: testfile (Too 
many open files)

at java.io.FileInputStream.open(Native Method)
at java.io.FileInputStream.(FileInputStream.java:128)
at SelectFdsLimit.openFiles(SelectFdsLimit.java:63)
at SelectFdsLimit.main(SelectFdsLimit.java:81)

Seems correct to me.
An by JTREG (also with hard limit set to 1024):
--messages:(3/123)--
command: main SelectFdsLimit
reason: User specified action: run main/othervm SelectFdsLimit
elapsed time (seconds): 0.168
--System.out:(0/0)--
--System.err:(5/250)--

Exception: java.lang.NoClassDefFoundError thrown from the 
UncaughtExceptionHandler in thread "MainThread"

STATUS:Passed.
Exception in thread "main"
Exception: java.lang.NoClassDefFoundError thrown from the 
UncaughtExceptionHandler in thread "main"

result: Passed. Execution successful


test result: Passed. Execution successful


The results are identical to results mentioned by you. It seems to me 
that jtreg doesn't correctly processes such test error (at least it 
shouldn't be considered as Pass). And I suggest two ways of resolving it:
1. If we don't have official limitations (or default) on what resources 
test can use then we shouldn't do any modifications to test.
2. If there is some limitations that we should honor then we'll need to 
figure out what to do with NoClassDefFoundError exception in JTREG.




2) Closing files.

If an exception is thrown while opening the initial set of files, or 
sometime during the closing process, the test can still leak files.


One approach would be to keep a data structure representing the 
current set of open files, and close them all in a finally-block 
around all the test logic, and making sure that exceptions from the 
close() call are caught and do not prevent the rest of the files from 
being closed.


This seems like a lot of work. Perhaps a more effective approach would 
be to run the test in "othervm" mode, as follows:


@run main/othervm SelectFdsLimit

This will cause the test to run in a dedicated JVM, so all files will 
be closed automatically when it exits. (It would be good to add a 
comment explaining th

hg: jdk8/tl/jdk: 8013809: deadlock in SSLSocketImpl between between write and close

2013-08-07 Thread xuelei . fan
Changeset: 8c7cf4926157
Author:xuelei
Date:  2013-08-07 06:42 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8c7cf4926157

8013809: deadlock in SSLSocketImpl between between write and close
Reviewed-by: wetmore

! src/share/classes/sun/security/ssl/SSLSocketImpl.java



Re: RFR : 6799426 : (xs) Add constructor PriorityQueue(Comparator)

2013-08-07 Thread Doug Lea

On 07/25/13 10:15, Doug Lea wrote:

On 07/24/13 19:30, Martin Buchholz wrote:

PriorityQueue is unusual in that Doug maintains a copy in jsr166 CVS even though
it is a non-thread-safe collection.  I think it makes sense,
because PriorityQueue and PriorityBlockingQueue have parallel APIs and parallel
implementations.  Many changes to one file require a matching change to the
other.



The history is that we introduced java.util.concurrent.PriorityBlockingQueue,
for JDK1.5 which of course led to people wanting plain java.util.PriorityQueue,
which Josh started working on while at Sun. But for sake of JCP approval,
we pulled into jsr166 and have adopted ever since, because the two
classes share code. The capacity constructor was added in JDK1.6,
but I have no recollection why there is not one with comparator
but no capacity. It seems reasonable to add one.


Now I remember. Or rather javac forces me to remember.
Adding this constructor breaks source compatibility:

[javac] /home/dl/concurrent/src/test/tck/PriorityBlockingQueueTest.java:97: 
error: reference to PriorityBlockingQueue is ambiguous

[javac] new PriorityBlockingQueue(null);
[javac] ^
[javac]   both constructor PriorityBlockingQueue(Comparator) in 
PriorityBlockingQueue and constructor PriorityBlockingQueue(CollectionE>) in PriorityBlockingQueue match

[javac]   where E is a type-variable:
[javac] E extends Object declared in class PriorityBlockingQueue


I'm thinking to not include this at least in PriorityBlockingQueue.

-Doug







Regarding this particular change, if PriorityQueue gets a new constructor taking
a Comparator, then PriorityBlockingQueue probably should too


Yes.

*** PriorityBlockingQueue.java.~1.99.~Wed Jul 24 11:23:05 2013
--- PriorityBlockingQueue.javaThu Jul 25 10:13:47 2013
***
*** 200,205 
--- 200,219 
   }

   /**
+  * Creates a {@code PriorityBlockingQueue} with the default
+  * initial capacity that orders its elements according to the
+  * specified comparator.
+  *
+  * @param  comparator the comparator that will be used to order this
+  * priority queue.  If {@code null}, the {@linkplain Comparable
+  * natural ordering} of the elements will be used.
+  * @since 1.8
+  */
+ public PriorityBlockingQueue(Comparator comparator) {
+ this(DEFAULT_INITIAL_CAPACITY, comparator);
+ }
+
+ /**
* Creates a {@code PriorityBlockingQueue} containing the elements
* in the specified collection.  If the specified collection is a
* {@link SortedSet} or a {@link PriorityQueue}, this






RFR JDK-8011940 : java.lang.Class.getAnnotations() always enters synchronized method

2013-08-07 Thread Peter Levart

Hi,

I propose a patch for the following and related bugs:

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8011940

Here's the 1st webrev:

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

The patch eliminates classic synchronization by using optimistic 
concurrent construction. At the end of construction, the "winning" 
result is installed using CAS. The solution is modelled upon the similar 
solution used to cache reflection data. Both annotations Maps and a 
redefinedCount int are grouped in a container AnnotationData object 
which is referenced with a single volatile field. Not using any locks 
also solves potential deadlock situations described in a related bug:


http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7122142

That bug has already been closed with a fix for AnnotationType 
construction and caching:


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

So the alternative solution for scalability problem is using 
double-checked locking instead of CAS should concurrent initial requests 
for annotations be a problem (CPU usage).


Micro benchmark shows about 5x improvement in raw single-threaded speed 
of retrieving an annotation by type and linear scalability:


http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/test_results1.txt

The space consumption is different with the patch. Initially j.l.Class 
objects are smaller, since one int and one reference field are 
eliminated from the j.l.Class. If annotations are requested the overhead 
is the AnnotationData container object and the field to reference it. 
But I tried to be smarter than the original code and when there's no 
inherited annotations (common situation), the annotations and 
declaredAnnotations share the same Map instance (originally new HashMap 
instance was constructed and entries copied). When there's no 
annotations declared on the class and no inherited annotations, this 
shared Map instance is an Collections.emptyMap() singleton.


Regards, Peter



Re: RFR JDK-8011940 : java.lang.Class.getAnnotations() always enters synchronized method

2013-08-07 Thread Aleksey Shipilev
Hi Peter,

On 08/07/2013 08:18 PM, Peter Levart wrote:
> http://cr.openjdk.java.net/~plevart/jdk8-tl/AnnotationData/webrev.01/

Yeah, looks familiar. The install loop is very complicated though, can
we simplify it? It seems profitable to move the retry loop up into
annotationData(): you then don't have to pass the $annotationData, you
can merge the exit paths for $classRedefinedCount, etc.

-Aleksey.





Re: Remove superfluous @test tags from SpliteratorTraversingAndSplittingTest

2013-08-07 Thread Stuart Marks
Yes, this is quite easy to reproduce with jtreg-4.1-b06. I haven't tried the 
latest sources but I don't think there's been so much change that this would be 
different using the tip of the jtreg source tree.


It's not clear to me what the behavior ought to be if a single test file has 
more than one @test tag. The jtreg tag spec [1] requires that @test be the 
first token in the leading comment of the test file. This implies that @test 
occurring later in the file has no significance. That the test is executed 
twice under certain circumstances seems like a bug to me. Either the second 
@test should be ignored, or perhaps it should be an error, not sure.


I can file a bug on this, if someone hasn't already.

s'marks

[1] http://openjdk.java.net/jtreg/tag-spec.txt




On 8/6/13 2:29 AM, Chris Hegarty wrote:

Hi Iris,

I'm not saying it is right, but this is the situation we found ourselves in.

It might point to a bug in jtreg, or just "bad" jtreg meta-data.

 >: ls one
...   HelloWorld.java
 >: cat one/HelloWorld.java
/*
  * @test
  * @bug 8765432
  */

/*
  * @test
  */

public class HelloWorld {
 public static void main(String[] args) {
 System.out.println("Hello World.");
 }
}

 >: jtreg -v1 one/HelloWorld.java
Passed: one/HelloWorld.java
Test results: passed: 1
Report written to .../jdk/test/JTreport/html/report.html
Results written to .../jdk/test/JTwork
 >: jtreg -v1 one
Passed: one/HelloWorld.java
Passed: one/HelloWorld.java#id1
Test results: passed: 2
Report written to .../jdk/test/JTreport/html/report.html
Results written to .../jdk/test/JTwork

-Chris.

P.S. Note, the offending test has been fixed, and there is no issue in the jdk
regression tests affected by this.

On 05/08/2013 18:16, Iris Clark wrote:

Hi, Chris.


I also noticed this. Running the test explicitly seems to locate just the
first @test, while running in a batch (sometimes) finds the two! Not sure why.


If you execute jtreg with explicit file name(s), only those files will be
search for "@test" tags.  If you use anything else, e.g. directory names,
then jtreg will search all *.java, *.sh, and *.html files searching for
"@test" tags. For the "sometimes" is it possible that one of the tests has
been somehow excluded (e.g. on a problem list) but the other one wasn't?

Thanks,
iris

-Original Message-
From: Chris Hegarty
Sent: Friday, August 02, 2013 3:19 AM
To: Paul Sandoz
Cc: Core-Libs-Dev Core-Libs-Dev
Subject: Re: Remove superfluous @test tags from
SpliteratorTraversingAndSplittingTest

On 02/08/2013 10:52, Paul Sandoz wrote:


On Aug 2, 2013, at 9:59 AM, Chris Hegarty   wrote:


SpliteratorTraversingAndSplittingTest contains two @test tags. The second
of which does not specify that the test needs to run with testng. This
causes the test to fail, or have an error, when run as a batch with other
tests.

The second @test tag is just not needed, and the @bug should be moved to
the original @test tag.


Ooops that is my fault, thanks for sorting this out.


No Problem.


When i ran jtreg manually it did not barf, is there anyway for it to
validate the meta-data?


I also noticed this. Running the test explicitly seems to locate just the
first @test, while running in a batch (sometimes) finds the two! Not sure
why. It is also ok to have more than one @test, just in this case it would
need to specify testng.

-Chris.


hg: jdk8/tl/jdk: 8022454: Fixed various serializations and deprecation warnings in java.util, java.net and sun.tools

2013-08-07 Thread joe . darcy
Changeset: c1f129f62f36
Author:lagergren
Date:  2013-08-07 08:08 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/c1f129f62f36

8022454: Fixed various serializations and deprecation warnings in java.util, 
java.net and sun.tools
Reviewed-by: darcy
Contributed-by: marcus.lagerg...@oracle.com

! src/share/classes/java/net/SocketAddress.java
! src/share/classes/java/util/logging/XMLFormatter.java
! src/share/classes/sun/tools/jar/JarException.java



Re: RFR: 8022479: clean up warnings from sun.tools.asm

2013-08-07 Thread Lance Andersen - Oracle
Hi Stuart,

On the surface the changes look fine, including what you did for SwitchData.

Certainly another pair of eyes would be good also on this.

Best
Lance
On Aug 7, 2013, at 2:28 AM, Stuart Marks wrote:

> Hi,
> 
> Please review the fix for this warnings cleanup bug.
> 
> Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022479
>(not yet available publicly, but should be shortly)
> 
> Webrev: http://cr.openjdk.java.net/~smarks/reviews/8022479/webrev.0/
> 
> There are a few items of note.
> 
> This is *very* old code. You can see some of the authors' names in the code. 
> It's replete with uses of Vector, Hashtable, and Enumeration. It also has a 
> kind of musty style, which might have been reasonable from the standpoint of 
> C programmers (which we all were at the time!). For example, there are a 
> bunch of cases of direct field access to another class. There are also cases 
> where a class contains a Vector that's returned by a getter method. I guess 
> we just have to trust the caller not to modify it at the wrong time.
> 
> I've confined most of my cleanups to the addition of generics (which gets rid 
> of the rawtypes and unchecked warnings). There were a smattering of other 
> warnings I've cleaned up as well. I've also replaced few cases of calls to 
> "new" on boxed types like Integer and Long with calls to the respective 
> valueOf() methods. I didn't check all the code for this, though, just 
> instances I happened to run across.
> 
> There is much more cleanup that could be done that I've elected not to do, 
> such as replacing Vector, Hashtable, and Enumeration with ArrayList, HashMap, 
> and Iterator, and using the enhanced-for loop. These changes would *probably* 
> be safe, but they would require more analysis and testing than I'm willing to 
> do at the moment.
> 
> Two locations deserve a bit of scrutiny.
> 
> 1) There are four occurrences in Assembler.java of calls to 
> MemberDefinition.getArguments(). Unfortunately MemberDefinition resides in 
> the sun.tools.java package and getArguments() returns a raw Vector. This is 
> also overridden in a couple places spread across a couple sun.tools packages, 
> and there are hints that it being a LocalMember or MemberDefinition. It seems 
> out of scope to try to fix up the return value of getArguments() and its 
> various overrides.
> 
> Locally in the Assembler.java file, though, the contents of the returned 
> vector are always cast to MemberDefinition, so it seems sensible to make an 
> unchecked cast of the getArguments() return value to Vector 
> and to suppress warnings at that point. Usage beyond those points within 
> Assembler.java is filled with sweet and juicy generic goodness.
> 
> 2) SwitchData.java contains a field whereCaseTab which is a 
> Hashtable ... most of the time. In the addTableDefault() 
> method, a value is put into the Hashtable under the key "default" -- a 
> String. Ick.
> 
> To deal with this I declared it as Hashtable since that's the 
> typical case, and for the put(String, Long) case I cast through raw and 
> suppressed the warning. The get() method already takes an Object so we're OK 
> there. Note that "default" as a key for this Hashtable is used in a couple 
> other places in Assembler.java. It's not intractable to change these to some 
> other sentinel value, but, you know, where does one stop?
> 
> (I said "Ick" when encountering a mix-type collection, and in a generified 
> world we very rarely see this style anymore. Upon further reflection, though, 
> from the standpoint of a pre-generics world, this is pretty sensible. Using a 
> string as a sentinel key is guaranteed not to collide with any Integer keys. 
> Sometimes it's possible to use -1 or Integer.MAX_VALUE as a sentinel, but if 
> any integer value is permitted as a key, what does one do then?)
> 
> With this changeset, sun.tools.asm is reduced from about 76 to zero warnings.
> 
> Thanks,
> 
> s'marks


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



Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

2013-08-07 Thread Rémi Forax
On 08/07/2013 10:57 AM, Peter Levart wrote:

On 08/07/2013 12:23 AM, Dan Smith wrote:

On Aug 6, 2013, at 2:43 PM, Remi Forax 
 wrote:


 On 08/06/2013 11:11 PM, Dan Smith wrote:

 Please review this warnings cleanup.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not
yet visible)
Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/

—Dan

 Hi Dan,
I've seen that you have introduce a common super interface for entry
and tree node,
I suppose that  you check that there is no performance regression.
I wonder if an abstract class is not better than an interface because
as far as I know
CHA implemented in hotspot doesn't work on interface
(but I may be wrong, there is perhaps a special optimization for arrays).

 To make sure I understand: your concern is that an aastore will be
more expensive when assigning to a KeyValueData[] than to an Object[]
(or even to SomeOtherClass[])?

For what it's worth, all assignments to table[i] are statically known
to be safe.  E.g.:

Entry next = (Entry) e.next;
...
table[i] = next;

So surely a smart VM only does the check once?

Here are some other things that might be concerns, but don't apply here:
- interface method invocations: there are no methods in the interface to invoke
- checkcast to an interface: all the casts are to concrete classes
(Entry, TreeBin, TreeNode)

(There are some unchecked casts from KeyValueData to KeyValueData with
different type parameters, but I assume these don't cause any
checkcasts.)

—Dan

 Hi,


Hi Peter,


FWIW, I compiled old and new HashMap.java and did a javap on the classes.

javap outputs: http://cr.openjdk.java.net/~plevart/misc/hm-8022442/
differences:
http://cr.openjdk.java.net/~plevart/misc/hm-8022442/hm.javap.stripped.diff

Besides unneeded introduction of local variable discussed already, there
seem to be 4 new checkcasts in the following locations (new HashMap.java):

  public java.util.HashMap(int, float);
Code:
   0: aload_0
   1: invokespecial #10 // Method
java/util/AbstractMap."":()V
   4: aload_0
   5: getstatic #11 // Field
EMPTY_TABLE:[Ljava/util/HashMap$KeyValueData;
*   8: checkcast #12 // class
"[Ljava/util/HashMap$KeyValueData;"*
  11: putfield  #13 // Field
table:[Ljava/util/HashMap$KeyValueData;
  14: aload_0
  15: aconst_null
...

  private void inflateTable(int);
Code:
...
  22: aload_0
  23: iload_2
  24: anewarray #44 // class
java/util/HashMap$KeyValueData
*  27: checkcast #12 // class
"[Ljava/util/HashMap$KeyValueData;"*
  30: putfield  #13 // Field
table:[Ljava/util/HashMap$KeyValueData;
  33: return
...

  void resize(int);
Code:
...
  20: return
  21: iload_1
  22: anewarray #44 // class
java/util/HashMap$KeyValueData
*  25: checkcast #12 // class
"[Ljava/util/HashMap$KeyValueData;"*
  28: astore4
  30: aload_0
  31: aload 4
...

  private void readObject(java.io.ObjectInputStream) throws
java.io.IOException, java.lang.ClassNotFoundException;
Code:
...
  82: invokevirtual #141// Method
sun/misc/Unsafe.putIntVolatile:(Ljava/lang/Object;JI)V
  85: aload_0
  86: getstatic #11 // Field
EMPTY_TABLE:[Ljava/util/HashMap$KeyValueData;
*  89: checkcast #12 // class
"[Ljava/util/HashMap$KeyValueData;"*
  92: putfield  #13 // Field
table:[Ljava/util/HashMap$KeyValueData;
  95: aload_1
  96: invokevirtual #142// Method
java/io/ObjectInputStream.readInt:()I
...


...but they are all in the infrequently called methods/constructor.


Regards, Peter


and all these 'checkcast' should be removed at runtime.

I've no problem with the current patch but I think it can be improved by
declaring KeyValueData
as an abstract class (static!) instead as an interface because the VM will
be able to remove
all the instanceof/checkcast that were introduced to deal with the
red/black tree implementation.

By example, when a user calls get, it calls in fact getEntry which is
starts with this code:
   final Entry getEntry(Object key) {
if (size == 0) {
return null;
}
if (key == null) {
return nullKeyEntry;
}
int hash = hash(key);
int bin = indexFor(hash, table.length);

if (table[bin] instanceof Entry) {
Entry e = (Entry) table[bin];
for (; e != null; e = (Entry)e.next) {
Object k;
if (e.hash == hash &&
((k = e.key) == key || key.equals(k))) {
return e;
}
}
}
...

The interesting part is:
  ...
  if (table[bin] instanceof Entry) {
Entry e = (Entry) table[bin];
...
(and

RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Dan Xu

Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan


Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Lance Andersen - Oracle
looks fine Dan
On Aug 7, 2013, at 1:49 PM, Dan Xu wrote:

> Hi All,
> 
> Please review the simple warning fix in 
> src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.
> 
> webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/
> 
> Thanks,
> 
> -Dan


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



Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Mike Duigou
Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:

> Hi All,
> 
> Please review the simple warning fix in 
> src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.
> 
> webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/
> 
> Thanks,
> 
> -Dan



Re: RFR [6961766]: PrintStream.write() should flush at most once

2013-08-07 Thread Ivan Gerasimov

Hello David!

Thanks for review.


Yes - this is NOT A BUG this is the spec for this class:

"Optionally, a PrintStream can be created so as to flush 
automatically; this means that the flush method is automatically 
invoked after a byte array is written, one of the println methods is 
invoked, or a newline character or byte ('\n') is written. "



Sorry, I don't see how the proposed change would contradict the spec .
The code first writes a char[] buffer to an OutputStream, and then 
invokes flush() if the written array contained at least one '\n' char.


In addition to that, the documentation for OutputStream#flush() says: 
"Flushes this output stream and forces any buffered output bytes to be 
written out." Thus, there should be no point to have several subsequent 
calls to flush() with no data writes between them.


I don't insist on pushing this change, but I think it's harmless and may 
be useful.


Sincerely yours,
Ivan


This bug report should be closed as "not an issue".

David
-



-Alan







Re: RFR 8022126: Remove throws SocketException from DatagramPacket constructors accepting SocketAddress

2013-08-07 Thread Matthew Hall
On Tue, Aug 06, 2013 at 06:18:39PM +0100, Michael McMahon wrote:
> Documenting in release notes is okay too, but I suspect developers are not 
> likely to look there at first anyway. Thinking aloud, it would be nice if 
> some kind of annotation could be associated with the affected constructors 
> such that a more meaningful/customized error message could be emitted by 
> javac. But, perhaps there aren't sufficient other use cases to justify that.

Many of us use Eclipse, NetBeans, and JavaDoc.

So if we just had a comment in the JavaDoc, saying this was fixed, and what to 
do, that ought to be more than adequate, and would prevent missing it if you 
didn't see the relnotes.

Matthew.


FileVisitor / Are we feature frozen yet?

2013-08-07 Thread Ben Evans
Hi,

I have a suggestion for a point lambdafication change which I don't think
anyone's proposed yet. (As usual, if my Google-fu has failed me, please
point me at the relevant discussion).

In java.nio.file we have the FileVisitor interface, which defines 4 methods.

We also have the SimpleFileVisitor class which defines sane (basically
no-op) implementations of all the methods.

In using FileVisitor, I've noticed that visitFile() tends to be overridden
/ used far more often than the others. It would be nifty if it we could
lambdify in such a way that a lambda expression could be fed to
Files.walkFileTree().

Could we add default implementations to FileVisitor (those from
SimpleFileVisitor seem reasonable) for all methods except visitFile() ?

This would then make code like this possible:

Path homeDir = Paths.get("/Users/kittylyst");
Files.walkFileTree(homeDir, (p, attrs) -> {
System.out.println(p.getFileName()); return FileVisitResult.CONTINUE; });

I have a patch which demonstrates this - let me know if there's potential
here, and if anyone would be interested in sponsoring the patch.

Thanks,

Ben


Re: FileVisitor / Are we feature frozen yet?

2013-08-07 Thread Ben Evans
On Files?

Yes, that does indeed look suspiciously like it'll cover the current use
cases I have. Let me check & come back if not.

Ben


On Tue, Aug 6, 2013 at 3:15 PM, Alan Bateman wrote:

> On 06/08/2013 06:39, Ben Evans wrote:
>
>> Hi,
>>
>> I have a suggestion for a point lambdafication change which I don't think
>> anyone's proposed yet. (As usual, if my Google-fu has failed me, please
>> point me at the relevant discussion).
>>
>> In java.nio.file we have the FileVisitor interface, which defines 4
>> methods.
>>
>> We also have the SimpleFileVisitor class which defines sane (basically
>> no-op) implementations of all the methods.
>>
>> In using FileVisitor, I've noticed that visitFile() tends to be overridden
>> / used far more often than the others. It would be nifty if it we could
>> lambdify in such a way that a lambda expression could be fed to
>> Files.walkFileTree().
>>
>> Could we add default implementations to FileVisitor (those from
>> SimpleFileVisitor seem reasonable) for all methods except visitFile() ?
>>
>> This would then make code like this possible:
>>
>> Path homeDir = Paths.get("/Users/kittylyst");
>> Files.walkFileTree(homeDir, (p, attrs) ->  {
>> System.out.println(p.**getFileName()); return FileVisitResult.CONTINUE;
>> });
>>
>> I have a patch which demonstrates this - let me know if there's potential
>> here, and if anyone would be interested in sponsoring the patch.
>>
>>  Have you looked at the walk and find methods? (They currently return a
> CloseableStream but there is discussion on just returning a Stream).
>
> -Alan.
>


Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Joe Darcy

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.

webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan




Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Dan Xu

Thanks for your review!

I was thinking of that. But without Class[] on the left, the compiler 
just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are printed 
out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan






Re: RFR JDK-8022442: Fix unchecked warnings in HashMap

2013-08-07 Thread Brent Christian

Hi,

I did a before/after run with the changes using Doug Lea's MapCheck 
microbenchmark [1].  I tested java.util.HashMap with Object, String, and 
Integer types.


It should be mentioned this was a quick check for any major performance 
changes: 2 iterations, run by hand on my own (relatively quiet) Mac. 
The changes are small.  (A more serious run would be needed to 
accurately measure such a small performance difference, ideally using 
jmh, and a benchmark with more precise scoring.)


A brief summary of the scoring differences is below, by type of access 
and data type.  The benchmark scores are based on the length of time to 
perform the operation, so a lower score is better, and a positive 
percent change means the operation took longer with the warnings cleanup 
changes.


---
MapCheck, java.util.HashMap, trials: 1000, size: 36864
%Change No Changes vs HashMap Warnings Cleanup

 ObjectString  Integer
Access Present   0.00% 0.00%   6.67%
Add Absent   0.00% 0.76%  -1.27%
Modify Present   0.00%-1.16%   0.00%
Remove Present   0.00% 0.00%   0.00%
Search Absent0.00% 2.00%   3.03%
Traverse entry   0.00% 0.00%   0.00%
Traverse key/val 0.00% 2.86%   0.00%
---

One might think the 6.6% increase for Access Present/Integer is cause 
for concern, but I think this is more a reflection of the scores being 
fairly coarse.  Specifically, the two scores before the changes were 15 
& 15, and with the changes were 15 & 17 (a mean of 16, 6.67% change from 
15).


I think the change is fine to go back.

For reference, my spreadsheet of results is at [2].

Thanks,
-Brent

[1] 
http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/test/loops/MapCheck.java

[2] http://cr.openjdk.java.net/~bchristi/8022442/

On 8/6/13 3:23 PM, Dan Smith wrote:

On Aug 6, 2013, at 2:43 PM, Remi Forax  wrote:


On 08/06/2013 11:11 PM, Dan Smith wrote:

Please review this warnings cleanup.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022442 (not yet 
visible)
Webrev: http://cr.openjdk.java.net/~dlsmith/8022442/webrev.00/

—Dan


Hi Dan,
I've seen that you have introduce a common super interface for entry and tree 
node,
I suppose that  you check that there is no performance regression.
I wonder if an abstract class is not better than an interface because as far as 
I know
CHA implemented in hotspot doesn't work on interface
(but I may be wrong, there is perhaps a special optimization for arrays).


To make sure I understand: your concern is that an aastore will be more 
expensive when assigning to a KeyValueData[] than to an Object[] (or even to 
SomeOtherClass[])?

For what it's worth, all assignments to table[i] are statically known to be 
safe.  E.g.:

Entry next = (Entry) e.next;
...
table[i] = next;

So surely a smart VM only does the check once?

Here are some other things that might be concerns, but don't apply here:
- interface method invocations: there are no methods in the interface to invoke
- checkcast to an interface: all the casts are to concrete classes (Entry, 
TreeBin, TreeNode)

(There are some unchecked casts from KeyValueData to KeyValueData with 
different type parameters, but I assume these don't cause any checkcasts.)

—Dan



Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Joe Darcy

Hi Dan,

Even if the compiler does not complain, using "Class" or "Class[]" is 
using a raw type and raw types should generally be viewed as 
unacceptable in modern code.


Cheers,

-Joe

On 08/07/2013 11:36 AM, Dan Xu wrote:

Thanks for your review!

I was thinking of that. But without Class[] on the left, the 
compiler just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are 
printed out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan








Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Dan Xu
I see, Thanks! I have updated my changeto 
http://cr.openjdk.java.net/~dxu/8022554/webrev1/.


-Dan

On 08/07/2013 11:46 AM, Joe Darcy wrote:

Hi Dan,

Even if the compiler does not complain, using "Class" or "Class[]" is 
using a raw type and raw types should generally be viewed as 
unacceptable in modern code.


Cheers,

-Joe

On 08/07/2013 11:36 AM, Dan Xu wrote:

Thanks for your review!

I was thinking of that. But without Class[] on the left, the 
compiler just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are 
printed out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan










Re: RFR: 8022479: clean up warnings from sun.tools.asm

2013-08-07 Thread Joe Darcy

Hi Stuart,

I believe the code in its current state out in review is a good 
trade-off of effort needed to address the warnings vs improvement in the 
quality of the code.


Certainly the code would benefit from a larger cleanup, but I agree that 
is not warranted at this point. Longer term, say for JDK 9, we should 
probably look to providing this functionality in some other way.


Approved to go back.

Thanks,

-Joe

On 08/07/2013 10:01 AM, Lance Andersen - Oracle wrote:

Hi Stuart,

On the surface the changes look fine, including what you did for SwitchData.

Certainly another pair of eyes would be good also on this.

Best
Lance
On Aug 7, 2013, at 2:28 AM, Stuart Marks wrote:


Hi,

Please review the fix for this warnings cleanup bug.

Bug: http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022479
(not yet available publicly, but should be shortly)

Webrev: http://cr.openjdk.java.net/~smarks/reviews/8022479/webrev.0/

There are a few items of note.

This is *very* old code. You can see some of the authors' names in the code. 
It's replete with uses of Vector, Hashtable, and Enumeration. It also has a 
kind of musty style, which might have been reasonable from the standpoint of C 
programmers (which we all were at the time!). For example, there are a bunch of 
cases of direct field access to another class. There are also cases where a 
class contains a Vector that's returned by a getter method. I guess we just 
have to trust the caller not to modify it at the wrong time.

I've confined most of my cleanups to the addition of generics (which gets rid of the 
rawtypes and unchecked warnings). There were a smattering of other warnings I've cleaned 
up as well. I've also replaced few cases of calls to "new" on boxed types like 
Integer and Long with calls to the respective valueOf() methods. I didn't check all the 
code for this, though, just instances I happened to run across.

There is much more cleanup that could be done that I've elected not to do, such 
as replacing Vector, Hashtable, and Enumeration with ArrayList, HashMap, and 
Iterator, and using the enhanced-for loop. These changes would *probably* be 
safe, but they would require more analysis and testing than I'm willing to do 
at the moment.

Two locations deserve a bit of scrutiny.

1) There are four occurrences in Assembler.java of calls to 
MemberDefinition.getArguments(). Unfortunately MemberDefinition resides in the 
sun.tools.java package and getArguments() returns a raw Vector. This is also 
overridden in a couple places spread across a couple sun.tools packages, and 
there are hints that it being a LocalMember or MemberDefinition. It seems out 
of scope to try to fix up the return value of getArguments() and its various 
overrides.

Locally in the Assembler.java file, though, the contents of the returned vector are 
always cast to MemberDefinition, so it seems sensible to make an unchecked cast of 
the getArguments() return value to Vector and to suppress 
warnings at that point. Usage beyond those points within Assembler.java is filled 
with sweet and juicy generic goodness.

2) SwitchData.java contains a field whereCaseTab which is a Hashtable ... 
most of the time. In the addTableDefault() method, a value is put into the Hashtable under the 
key "default" -- a String. Ick.

To deal with this I declared it as Hashtable since that's the typical case, 
and for the put(String, Long) case I cast through raw and suppressed the warning. The get() 
method already takes an Object so we're OK there. Note that "default" as a key for 
this Hashtable is used in a couple other places in Assembler.java. It's not intractable to 
change these to some other sentinel value, but, you know, where does one stop?

(I said "Ick" when encountering a mix-type collection, and in a generified 
world we very rarely see this style anymore. Upon further reflection, though, from the 
standpoint of a pre-generics world, this is pretty sensible. Using a string as a sentinel 
key is guaranteed not to collide with any Integer keys. Sometimes it's possible to use -1 
or Integer.MAX_VALUE as a sentinel, but if any integer value is permitted as a key, what 
does one do then?)

With this changeset, sun.tools.asm is reduced from about 76 to zero warnings.

Thanks,

s'marks



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





Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Joe Darcy

Amended version approved to go back.

Thanks,

-Joe

On 08/07/2013 11:54 AM, Dan Xu wrote:
I see, Thanks! I have updated my changeto 
http://cr.openjdk.java.net/~dxu/8022554/webrev1/.


-Dan

On 08/07/2013 11:46 AM, Joe Darcy wrote:

Hi Dan,

Even if the compiler does not complain, using "Class" or "Class[]" is 
using a raw type and raw types should generally be viewed as 
unacceptable in modern code.


Cheers,

-Joe

On 08/07/2013 11:36 AM, Dan Xu wrote:

Thanks for your review!

I was thinking of that. But without Class[] on the left, the 
compiler just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are 
printed out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan












Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Remi Forax

On 08/07/2013 08:59 PM, Joe Darcy wrote:

Amended version approved to go back.

Thanks,

-Joe


As one of the guys involved in the design of this API :)
I'm Ok with this change.

Rémi



On 08/07/2013 11:54 AM, Dan Xu wrote:
I see, Thanks! I have updated my changeto 
http://cr.openjdk.java.net/~dxu/8022554/webrev1/.


-Dan

On 08/07/2013 11:46 AM, Joe Darcy wrote:

Hi Dan,

Even if the compiler does not complain, using "Class" or "Class[]" 
is using a raw type and raw types should generally be viewed as 
unacceptable in modern code.


Cheers,

-Joe

On 08/07/2013 11:36 AM, Dan Xu wrote:

Thanks for your review!

I was thinking of that. But without Class[] on the left, the 
compiler just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are 
printed out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan














Re: RFR: JDK-8022554 -- Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread Dan Xu

Thank you for your review!

-Dan

On 08/07/2013 12:08 PM, Remi Forax wrote:

On 08/07/2013 08:59 PM, Joe Darcy wrote:

Amended version approved to go back.

Thanks,

-Joe


As one of the guys involved in the design of this API :)
I'm Ok with this change.

Rémi



On 08/07/2013 11:54 AM, Dan Xu wrote:
I see, Thanks! I have updated my changeto 
http://cr.openjdk.java.net/~dxu/8022554/webrev1/.


-Dan

On 08/07/2013 11:46 AM, Joe Darcy wrote:

Hi Dan,

Even if the compiler does not complain, using "Class" or "Class[]" 
is using a raw type and raw types should generally be viewed as 
unacceptable in modern code.


Cheers,

-Joe

On 08/07/2013 11:36 AM, Dan Xu wrote:

Thanks for your review!

I was thinking of that. But without Class[] on the left, the 
compiler just worked fine.


Here is a simple example,

//Main.java

import java.util.*;

public class Main {
public static final Class[] TEST_CLASS = new Class[16];
public static final List[] TEST_MAP = new ArrayList[16];
}

After compiling with javac Xlint:all Main.java, no warnings are 
printed out. Is it a compiler issue? Thanks!


-Dan



On 08/07/2013 11:11 AM, Joe Darcy wrote:

I agree with Mike; Class[] should be used on both sides.

Cheers,

-Joe

On 08/07/2013 10:53 AM, Mike Duigou wrote:

Why not have Class[] on the left side as well?

Mike

On Aug 7 2013, at 10:49 , Dan Xu wrote:


Hi All,

Please review the simple warning fix in 
src/share/classes/sun/invoke/anon/ConstantPoolPatch.java.


webrev: http://cr.openjdk.java.net/~dxu/8022554/webrev/

Thanks,

-Dan
















hg: jdk8/tl/jdk: 8022554: Fix Warnings in sun.invoke.anon Package

2013-08-07 Thread dan . xu
Changeset: d1c82d5bee3f
Author:dxu
Date:  2013-08-07 12:13 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/d1c82d5bee3f

8022554: Fix Warnings in sun.invoke.anon Package
Reviewed-by: darcy, mduigou, lancea

! src/share/classes/sun/invoke/anon/ConstantPoolPatch.java



RFR: 8015068 : (m) Use jtreg -exclude for problemlist.txt processing

2013-08-07 Thread Mike Duigou
Hello all;

This changesest simplifies how the the jdk/test/Makefile processes excluded 
tests. Previously the test exclusions were pre-processed by scripts in the 
Makefile before being passed to JTReg. JTReg will now the all the processing. 
The change depends upon improvements in JTReg since the test exclude mechanism 
was originally defined.

There are some changes in the reporting. Most obvious is that the 
excludelist.txt output file is no longer produced. The 'excluded' count in the 
summary report may be removed in a future reporting depending on whether the 
value can be calculated some other way.

Additional to the exclusion list processing changes some other changes are 
possible as a result of the changes to exclusion processing. In particular the 
obsolete (and incorrect) logic for determining the build path included in the 
test/Makefile is now removed. If no output directory is provided via 
ALT_OUTPUTDIR then a default location in the current directory is used, 
"testoutput". At some point in the future this could be improved to get the 
CONF from the build configuration but that is not currently possible.

Testing of this patch requires using a source build of JTReg as it requires one 
fix that is not in the promoted builds. 

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

Mike

hg: jdk8/tl/langtools: 7198274: RFE : Javadoc Accessibility : Use CSS styles rather than or tags

2013-08-07 Thread bhavesh . x . patel
Changeset: 8c55df2442c1
Author:bpatel
Date:  2013-08-07 15:00 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/8c55df2442c1

7198274: RFE : Javadoc Accessibility : Use CSS styles rather than  or 
 tags
Reviewed-by: jjg

! 
src/share/classes/com/sun/tools/doclets/formats/html/AbstractExecutableMemberWriter.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/AnnotationTypeRequiredMemberWriterImpl.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/AnnotationTypeWriterImpl.java
! src/share/classes/com/sun/tools/doclets/formats/html/ClassWriterImpl.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/EnumConstantWriterImpl.java
! src/share/classes/com/sun/tools/doclets/formats/html/FieldWriterImpl.java
! src/share/classes/com/sun/tools/doclets/formats/html/HtmlDocletWriter.java
! src/share/classes/com/sun/tools/doclets/formats/html/MethodWriterImpl.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/NestedClassWriterImpl.java
! src/share/classes/com/sun/tools/doclets/formats/html/PackageFrameWriter.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/ProfilePackageFrameWriter.java
! src/share/classes/com/sun/tools/doclets/formats/html/PropertyWriterImpl.java
! 
src/share/classes/com/sun/tools/doclets/formats/html/SubWriterHolderWriter.java
! src/share/classes/com/sun/tools/doclets/formats/html/TagletWriterImpl.java
! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlStyle.java
! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlTree.java
! 
src/share/classes/com/sun/tools/doclets/internal/toolkit/resources/stylesheet.css
! test/com/sun/javadoc/testClassCrossReferences/TestClassCrossReferences.java
! 
test/com/sun/javadoc/testExternalOverridenMethod/TestExternalOverridenMethod.java
! test/com/sun/javadoc/testHtmlDefinitionListTag/TestHtmlDefinitionListTag.java
! test/com/sun/javadoc/testInterface/TestInterface.java
! test/com/sun/javadoc/testJavaFX/TestJavaFX.java
! test/com/sun/javadoc/testMemberInheritence/TestMemberInheritence.java
! test/com/sun/javadoc/testMemberSummary/TestMemberSummary.java
! test/com/sun/javadoc/testNewLanguageFeatures/TestNewLanguageFeatures.java
! test/com/sun/javadoc/testOverridenMethods/TestOverridenMethodDocCopy.java
! test/com/sun/javadoc/testOverridenMethods/TestOverridenPrivateMethods.java
! 
test/com/sun/javadoc/testOverridenMethods/TestOverridenPrivateMethodsWithPackageFlag.java
! 
test/com/sun/javadoc/testOverridenMethods/TestOverridenPrivateMethodsWithPrivateFlag.java
! test/com/sun/javadoc/testPackageDeprecation/TestPackageDeprecation.java
! test/com/sun/javadoc/testPrivateClasses/TestPrivateClasses.java
! 
test/com/sun/javadoc/testSerializedFormDeprecationInfo/TestSerializedFormDeprecationInfo.java



hg: jdk8/tl/langtools: 4749567: stddoclet: Add CSS style for setting header/footer to be italic

2013-08-07 Thread bhavesh . x . patel
Changeset: 33294f02c9a5
Author:bpatel
Date:  2013-08-07 16:09 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/langtools/rev/33294f02c9a5

4749567: stddoclet: Add CSS style for setting header/footer to be italic
Reviewed-by: jjg

! src/share/classes/com/sun/tools/doclets/formats/html/HelpWriter.java
! src/share/classes/com/sun/tools/doclets/formats/html/HtmlDocletWriter.java
! src/share/classes/com/sun/tools/doclets/formats/html/markup/HtmlTree.java
! 
src/share/classes/com/sun/tools/doclets/internal/toolkit/resources/stylesheet.css
+ test/com/sun/javadoc/testOptions/TestOptions.java
+ test/com/sun/javadoc/testOptions/pkg/Foo.java



Re: RFR: 8022479: clean up warnings from sun.tools.asm

2013-08-07 Thread Stuart Marks

Thanks for the reviews, guys.

I think you meant, long term we should look to providing this functionality in 
some other way **and throw all of these packages into the trash!** :-)


s'marks


On 8/7/13 11:56 AM, Joe Darcy wrote:

Hi Stuart,

I believe the code in its current state out in review is a good trade-off of
effort needed to address the warnings vs improvement in the quality of the code.

Certainly the code would benefit from a larger cleanup, but I agree that is not
warranted at this point. Longer term, say for JDK 9, we should probably look to
providing this functionality in some other way.

Approved to go back.

Thanks,

-Joe

On 08/07/2013 10:01 AM, Lance Andersen - Oracle wrote:

Hi Stuart,

On the surface the changes look fine, including what you did for SwitchData.

Certainly another pair of eyes would be good also on this.

Best
Lance
On Aug 7, 2013, at 2:28 AM, Stuart Marks wrote:


Hi,

Please review the fix for this warnings cleanup bug.

Bug:http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022479
(not yet available publicly, but should be shortly)

Webrev:http://cr.openjdk.java.net/~smarks/reviews/8022479/webrev.0/

There are a few items of note.

This is *very* old code. You can see some of the authors' names in the code. 
It's replete with uses of Vector, Hashtable, and Enumeration. It also has a 
kind of musty style, which might have been reasonable from the standpoint of C 
programmers (which we all were at the time!). For example, there are a bunch of 
cases of direct field access to another class. There are also cases where a 
class contains a Vector that's returned by a getter method. I guess we just 
have to trust the caller not to modify it at the wrong time.

I've confined most of my cleanups to the addition of generics (which gets rid of the 
rawtypes and unchecked warnings). There were a smattering of other warnings I've cleaned 
up as well. I've also replaced few cases of calls to "new" on boxed types like 
Integer and Long with calls to the respective valueOf() methods. I didn't check all the 
code for this, though, just instances I happened to run across.

There is much more cleanup that could be done that I've elected not to do, such 
as replacing Vector, Hashtable, and Enumeration with ArrayList, HashMap, and 
Iterator, and using the enhanced-for loop. These changes would *probably* be 
safe, but they would require more analysis and testing than I'm willing to do 
at the moment.

Two locations deserve a bit of scrutiny.

1) There are four occurrences in Assembler.java of calls to 
MemberDefinition.getArguments(). Unfortunately MemberDefinition resides in the 
sun.tools.java package and getArguments() returns a raw Vector. This is also 
overridden in a couple places spread across a couple sun.tools packages, and 
there are hints that it being a LocalMember or MemberDefinition. It seems out 
of scope to try to fix up the return value of getArguments() and its various 
overrides.

Locally in the Assembler.java file, though, the contents of the returned vector are 
always cast to MemberDefinition, so it seems sensible to make an unchecked cast of 
the getArguments() return value to Vector and to suppress 
warnings at that point. Usage beyond those points within Assembler.java is filled 
with sweet and juicy generic goodness.

2) SwitchData.java contains a field whereCaseTab which is a Hashtable ... 
most of the time. In the addTableDefault() method, a value is put into the Hashtable under the 
key "default" -- a String. Ick.

To deal with this I declared it as Hashtable since that's the typical case, 
and for the put(String, Long) case I cast through raw and suppressed the warning. The get() 
method already takes an Object so we're OK there. Note that "default" as a key for 
this Hashtable is used in a couple other places in Assembler.java. It's not intractable to 
change these to some other sentinel value, but, you know, where does one stop?

(I said "Ick" when encountering a mix-type collection, and in a generified 
world we very rarely see this style anymore. Upon further reflection, though, from the 
standpoint of a pre-generics world, this is pretty sensible. Using a string as a sentinel 
key is guaranteed not to collide with any Integer keys. Sometimes it's possible to use -1 
or Integer.MAX_VALUE as a sentinel, but if any integer value is permitted as a key, what 
does one do then?)

With this changeset, sun.tools.asm is reduced from about 76 to zero warnings.

Thanks,

s'marks



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





Re: Possible HashMap update

2013-08-07 Thread Brent Christian

Hi, Doug

I like this approach.

The subclassing used for nodes/entries and trees looks good, given the 
constraints.  The before/after references that HashMap.TreeNode inherits 
from LinkedHashMap.Entry are superfluous when trees are used by a 
HashMap, but IMO we can live with that as trees in general are the 
uncommon case.  And on the other hand, it's great that we can skip the 
instanceof check for put()/get() when the key is the first/only entry in 
a bin.


"Untreeify" and MIN_TREEIFY_CAPACITY are nice additions.

I patched jdk8/tl, and it tests out OK with the jdk_util jtreg tests.

I'm in favor of moving forward with this.

Thanks,
-Brent

On 7/3/13 3:55 PM, Doug Lea wrote:


A few weeks ago, in the course of doing a (hopefully-last) reworking of
JDK8 ConcurrentHashMap (CHM) internals based on experience with
preview versions, I noticed that the balanced tree scheme now in place
as a fallback in the case of usages with many keys all with the same
hashCode could also be profitably used in other cases: CHM (like
HashMap) previously performed some bit-scrambling of user hashCodes to
make it less likely that user hashCodes that were non-identical but
highly-non-random will collide in the same bins (thus with O(n) vs the
expected O(1) performance).  This bit-scrambling is an
anti-optimization in many usages that do not contain the kinds of
systematic hashCode bias that most hurt performance.  While it is
reasonably fast, it still places computation where you do not
want it: in front of the likely cache-miss to access an entry. Plus,
bit-scrambling is only statistically likely to help.  It is still
possible to encounter hashCodes that systematically collide. It would
be nice to provide O(log N) guarantees for these cases as well.

So, CHM now omits bit-scrambling (well, almost -- just a single xor to
avoid losing impact of top bits), and instead rolls over into using
trees for bins that contain a statistically unlikely number of keys,
and rolling back into simple bin form if/when they don't due to
deletions or resizings. ("Statistically unlikely" is a surprisingly
low number. More than 8 nodes are expected only about once per ten
million under perfectly random keys; this is also often (depending on
cost of equals() methods, additional dispatch overhead, etc), around
the break-even point for using trees vs lists).  The updated tree
mechanics now provide O(log N) worst-case performance in either of two
cases: (colliding non-identical-hashCodes), and (identical hashCodes
but mutually Comparable keys). And does so while also speeding up by a
little those typical usages that do not encounter systematic
collisions.  Also, there is no sense at all in using any form of
hashSeed in this scheme. It basically behaves the same whether or not
you use one.

The overall impact appears to be a net win.  Non-heavily-colliding
cases are a bit faster. Some colliding cases are a little slower and
use more memory (tree-based nodes are bigger than plain ones), but
have much better worst (and typical) case bounds unless people use
crazily-bad keys that all have same hashCodes but are never equal or
comparable to others, in which case treeification is wasted effort
(but only by a constant factor of about 2 in both time and space).

This seemed to be enough of an improvement in terms of both worst-case
and expected performance to try applying it to TreeBin-enabled
HashMap. So I gave it a try. To enable bidirectional tree<->plain node
conversion, I used the subclassing strategy mentioned during
discussion of the initial balanced tree version. This in turn requires
a completely different within-package internal API to allow
LinkedHashMap to continue to subclass HashMap.  A couple of internal
constructions exist solely to allow the same source code to be
identical in HashMap and ConcurrenHashMap (but not the same compiled
code, since the types they operate on are and must be unrelated in the
two classes, and because they live in different packages, there's no
nice way to express their commonalities without exposing.)

This note serves as a pre-proposal request for comment, as well a
request for anyone interested in trying it out, especially in any
weird/unusual use cases, to report experiences before seriously
considering it as a replacement. To simplify logistics, I checked in the
code in a workspace directory in jsr166:

http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/dl/java/util/HashMap.java?view=log


http://gee.cs.oswego.edu/cgi-bin/viewcvs.cgi/jsr166/src/dl/java/util/LinkedHashMap.java?view=log



RFR: 8022547: [verifier] move DefaultMethodRegressionTests.java to jdk

2013-08-07 Thread Kumar Srinivasan

Hello,

Testing this functionality in langtools does not seem to be the 
appropriate location,
and the teams have decided to move it to jdk/test/vm/verifier, which 
seems to be the

logical place.

I have modified the test to remove testng dependencies, also added a 
variant which
involves loading classes via an URLClassLoader which exposed a test bug 
wrt. the
class.version.major not being correct, therefore it is useful to have 
this variant anyway.


The webrev:
http://cr.openjdk.java.net/~ksrini/8022547/webrev.0/

The bug:
http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8022547

Thanks
Kumar






RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread Ivan Gerasimov

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary 
allocation?

http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan


Remove superfluous @test tags from SpliteratorTraversingAndSplittingTest

2013-08-07 Thread Jonathan Gibbons

Chris,


It might point to a bug in jtreg, or just "bad" jtreg meta-data.

  >: ls one
...   HelloWorld.java
  >: cat one/HelloWorld.java
/*
   * @test
   * @bug 8765432
   */

/*
   * @test
   */

public class HelloWorld {
  public static void main(String[] args) {
  System.out.println("Hello World.");
  }
}


This is a un-feature in jtreg.   Strictly speaking, the jtreg Tag Spec 
only allows one test description per file, and so if you follow the 
spec, the situation you describe cannot arise.


-- Jon



Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread Alan Bateman

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is to 
check the return from  GetStringUTFChars so that the name returns when 
it fails with NULL.


-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary 
allocation?

http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan




Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread Ivan Gerasimov

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return 
NULL.
For allocation of the result string it calls AllocateHeap() with the 
default EXIT_OOM fail strategy.

Thus, in case of being unable to allocate memory it simply stops VM.

Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is 
to check the return from  GetStringUTFChars so that the name returns 
when it fails with NULL.


-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary 
allocation?

http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan








Re: RFR: 8015068 : (m) Use jtreg -exclude for problemlist.txt processing

2013-08-07 Thread Alan Bateman

On 07/08/2013 14:44, Mike Duigou wrote:

Hello all;

This changesest simplifies how the the jdk/test/Makefile processes excluded 
tests. Previously the test exclusions were pre-processed by scripts in the 
Makefile before being passed to JTReg. JTReg will now the all the processing. 
The change depends upon improvements in JTReg since the test exclude mechanism 
was originally defined.

There are some changes in the reporting. Most obvious is that the 
excludelist.txt output file is no longer produced. The 'excluded' count in the 
summary report may be removed in a future reporting depending on whether the 
value can be calculated some other way.

Additional to the exclusion list processing changes some other changes are possible as a 
result of the changes to exclusion processing. In particular the obsolete (and incorrect) 
logic for determining the build path included in the test/Makefile is now removed. If no 
output directory is provided via ALT_OUTPUTDIR then a default location in the current 
directory is used, "testoutput". At some point in the future this could be 
improved to get the CONF from the build configuration but that is not currently possible.

Testing of this patch requires using a source build of JTReg as it requires one 
fix that is not in the promoted builds.

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

Mike
It's good to see this logic going away. Also defaulting the output 
directory to TEST_ROOT (= pwd) is an improvement.


One thing that isn't completely clear to be is the whether the multiple 
-exclude options work as advertised (because that hasn't worked for me 
in the past and I always ended cat'ing them as per the old make file 
logic). I assume you have tested with the Oracle jdk/test/closed repo 
present to verify this.


-Alan



hg: jdk8/tl/jdk: 8022479: clean up warnings from sun.tools.asm

2013-08-07 Thread stuart . marks
Changeset: 8c50c27418d3
Author:smarks
Date:  2013-08-07 16:29 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/8c50c27418d3

8022479: clean up warnings from sun.tools.asm
Reviewed-by: lancea, darcy

! src/share/classes/sun/tools/asm/Assembler.java
! src/share/classes/sun/tools/asm/ConstantPool.java
! src/share/classes/sun/tools/asm/Instruction.java
! src/share/classes/sun/tools/asm/SwitchData.java
! src/share/classes/sun/tools/asm/TryData.java



Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread David Holmes

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return
NULL.
For allocation of the result string it calls AllocateHeap() with the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw 
OutOfMemoryError if it has memory issues, not abort the VM!


I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is
to check the return from  GetStringUTFChars so that the name returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan








Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread Alan Bateman

On 07/08/2013 18:20, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot 
return NULL.
For allocation of the result string it calls AllocateHeap() with the 
default EXIT_OOM fail strategy.

Thus, in case of being unable to allocate memory it simply stops VM.

GetStringUTFChars is supposed to return NULL in OOM situations so this 
may be a bug (but if you can, it would be good to handle it in the 
NetworkInterface code).


-Alan



Re: Deadlock in clinit for sun.nio.ch.Util/IOUtil

2013-08-07 Thread Alan Bateman

On 07/08/2013 09:56, Jeremy Manson wrote:
Thanks, Alan.  Do please ping back when you've created the bug (I'm 
not on nio-dev), because I'd like to track it (so we can remove our 
workaround when the time comes).

The bug is:

8022594: Potential deadlock in  of sun.nio.ch.Util/IOUtil

A webrev coming soon, I just want to ensure that the tests pass on all 
platforms.


-Alan.


Re: RFR 8021820: Number of opened files used in select() is limited to 1024 [macosx]

2013-08-07 Thread Stuart Marks

Hi Aleksej,

Thanks for the update. The situation is a bit twisted.

I picked up a couple clues from David Holmes and Jon Gibbons. The 
NoClassDefFoundError occurs when the JVM has hit its resource limit for the 
number of open files, *and* it is being run in a development environment with 
individual class files in a hierarchy, e.g. within


ROOT/build//jdk/classes

In this case, since each class is in its own file, it's quite likely that the 
loading of an individual class will fail because of lack of file descriptors. 
If the JVM itself encounters this, it will generate a NoClassDefFoundError 
without a stack trace such as the ones we've seen.


If the test is being run against a fully built JDK image, classes are loaded 
from rt.jar. This is usually already open, so quite often classes can be loaded 
without having to open additional files. In this case we get the 
FileNotFoundException as expected.


The resource limits seem to vary from system to system, and even from one 
Ubuntu version to the next (mine has a default hard limit of 1024 open files). 
Unfortunately, while it might seem reasonable to have minimum specifications 
for systems on which we run tests, in practice this has proven very difficult. 
Since the bug being fixed is Mac-only, and the default open file limit for Mac 
seems to be unlimited, perhaps it makes most sense for this to be a Mac-only test.


From the discussion here [1] which refers to an Apple technote [2] it seems 
like the best way to test for being on a Mac is something like this:


if (! System.getProperty("os.name").contains("OS X")) {
return;
}

That is, report success if we're on anything other than a Mac.

Once we're sure we're on a Mac, having the test fail if it can't open enough 
files seems reasonable.


I'd suggest putting a comment at the top of the test class saying that this 
test *must* be run in othervm mode, to ensure that files are closed properly. 
That way, you can take out the cleanupFiles() method too, as well as avoiding 
lots of exception handling and related cleanup code.


Finally, a style point: try/catch statements are conventionally indented as a 
single multi-block, not as separate statements. I'd suggest something like this:


// The accept() call will throw SocketException if the
// select() has failed due to limitation on fds size,
// indicating test failure. A SocketTimeoutException
// is expected, so it is caught and ignored, and the test
// passes.

try {
socket.accept();
} catch (SocketTimeoutException e) { }

s'marks


[1] 
http://mail.openjdk.java.net/pipermail/macosx-port-dev/2012-November/005148.html


[2] https://developer.apple.com/library/mac/#technotes/tn2002/tn2110.html



On 8/7/13 6:01 AM, Aleksej Efimov wrote:

Stuart, thank you for you comments, responses are below.
New webrev:
http://cr.openjdk.java.net/~aefimov/8021820/webrev.02/



-Aleksej

On 08/06/2013 05:14 AM, Stuart Marks wrote:

Hi Aleksej,

Thanks for the update. I took a look at the revised test, and there are still
some issues. (I didn't look at the build changes.)

1) System-specific resource limits.

I think the biggest issue is resource limits on the number of open files per
process that might vary from system to system. On my Ubuntu system, the hard
limit on the number of open files is 1,024. The test opens 1,023 files and
then one more for the socket. Unfortunately the JVM and jtreg have several
files open already, and the test crashes before the openFiles() method
completes.

(Oddly it crashes with a NoClassDefFoundError from the main thread's uncaught
exception handler, and then the test reports that it passed! Placing a
try/catch of Throwable in main() or openFiles() doesn't catch this error. I
have no explanation for this. When run standalone -- i.e., not from jtreg --
the test throws FileNotFoundException (too many open files) from openFiles(),
which is expected.)

On my Mac (10.7.5) the soft limit is 256 files, but the hard limit is
unlimited. The test succeeds in opening all its files but fails because of
the select() bug you're fixing. (This is expected; I didn't rebuild my JDK
with your patch.) I guess the soft limit doesn't do anything on Mac.

Amazingly, the test passed fine on both Windows XP and Windows 8.

I'm not entirely sure what to do about resource limits. Since the test is
able to open >1024 files on Mac, Windows, and possibly other Linuxes, it
seems reasonable to continue with this approach. If it's possible to catch
the error that occurs if the test cannot open its initial 1,024 files,
perhaps it should do this, log a message indicating what happened, and
consider the test to have passed. I'm mystified by the uncaught/uncatchable
NoClassDefFoundError though.

I wonder if this is a question of test environment required for JTREG tests: if
we'll execute JTREG tests with low value assigned t

Re: RFR [6961766]: PrintStream.write() should flush at most once

2013-08-07 Thread David Holmes

Hi Ivan,

On 8/08/2013 3:53 AM, Ivan Gerasimov wrote:

Hello David!

Thanks for review.


Yes - this is NOT A BUG this is the spec for this class:

"Optionally, a PrintStream can be created so as to flush
automatically; this means that the flush method is automatically
invoked after a byte array is written, one of the println methods is
invoked, or a newline character or byte ('\n') is written. "


Sorry, I don't see how the proposed change would contradict the spec .
The code first writes a char[] buffer to an OutputStream, and then
invokes flush() if the written array contained at least one '\n' char.

In addition to that, the documentation for OutputStream#flush() says:
"Flushes this output stream and forces any buffered output bytes to be
written out." Thus, there should be no point to have several subsequent
calls to flush() with no data writes between them.


Sorry I misread the original code - it actually violates the spec as 
well in my opinion. As I read it there should be a flush as soon as the 
\n is written, not simply a flush at some later point in time if a \n 
happened to have been written. As written those additional flushes will 
be no-ops as you rightly point out. So your change is a more efficient 
version of the existing "incorrect" implementation.


That said the layering of the streams in this class is quite confusing 
to me and it seems odd that if both textOut and charOut are 
unconditionally flushed in this method, then why is 'out' only flushed 
based on autoflush and the presence of the \n ??


But I withdraw my objections as what you propose does not change the 
behaviour.


Thanks,
David


I don't insist on pushing this change, but I think it's harmless and may
be useful.

Sincerely yours,
Ivan


This bug report should be closed as "not an issue".

David
-



-Alan







Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread Ivan Gerasimov

David, Alan,

I added checking for NULL  results and throwing OOMException if necessary.
I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return
NULL.
For allocation of the result string it calls AllocateHeap() with the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw 
OutOfMemoryError if it has memory issues, not abort the VM!


I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is
to check the return from  GetStringUTFChars so that the name returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet 
available.)


Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan













Re: RFR: 8015068 : (m) Use jtreg -exclude for problemlist.txt processing

2013-08-07 Thread Mike Duigou




On 2013-08-07, at 18:22, Alan Bateman  wrote:

> On 07/08/2013 14:44, Mike Duigou wrote:
>> Hello all;
>> 
>> This changesest simplifies how the the jdk/test/Makefile processes excluded 
>> tests. Previously the test exclusions were pre-processed by scripts in the 
>> Makefile before being passed to JTReg. JTReg will now the all the 
>> processing. The change depends upon improvements in JTReg since the test 
>> exclude mechanism was originally defined.
>> 
>> There are some changes in the reporting. Most obvious is that the 
>> excludelist.txt output file is no longer produced. The 'excluded' count in 
>> the summary report may be removed in a future reporting depending on whether 
>> the value can be calculated some other way.
>> 
>> Additional to the exclusion list processing changes some other changes are 
>> possible as a result of the changes to exclusion processing. In particular 
>> the obsolete (and incorrect) logic for determining the build path included 
>> in the test/Makefile is now removed. If no output directory is provided via 
>> ALT_OUTPUTDIR then a default location in the current directory is used, 
>> "testoutput". At some point in the future this could be improved to get the 
>> CONF from the build configuration but that is not currently possible.
>> 
>> Testing of this patch requires using a source build of JTReg as it requires 
>> one fix that is not in the promoted builds.
>> 
>> http://cr.openjdk.java.net/~mduigou/JDK-8015068/0/webrev/
>> 
>> Mike
> It's good to see this logic going away. Also defaulting the output directory 
> to TEST_ROOT (= pwd) is an improvement.
> 
> One thing that isn't completely clear to be is the whether the multiple 
> -exclude options work as advertised (because that hasn't worked for me in the 
> past and I always ended cat'ing them as per the old make file logic). I 
> assume you have tested with the Oracle jdk/test/closed repo present to verify 
> this.
> 

Yes. This is the fix that requires the updated jtreg. I won't push this change 
until jtreg is next promoted (b07)

> -Alan
> 


hg: jdk8/tl/jdk: 8015986: Incorrect Localization of HijrahChronology

2013-08-07 Thread masayoshi . okutsu
Changeset: 0beaa65c90c8
Author:okutsu
Date:  2013-08-08 13:51 +0900
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/0beaa65c90c8

8015986: Incorrect Localization of HijrahChronology
Reviewed-by: naoto
Contributed-by: scolebou...@joda.org, roger.ri...@oracle.com

! make/tools/src/build/tools/cldrconverter/CLDRConverter.java
! make/tools/src/build/tools/cldrconverter/CalendarType.java
! src/share/classes/sun/text/resources/FormatData.java
! src/share/classes/sun/text/resources/ar/FormatData_ar.java
! test/java/time/test/java/time/format/TestNonIsoFormatter.java



Re: RFR [8022584] Memory leak in some NetworkInterface methods

2013-08-07 Thread David Holmes

Ivan,

On 8/08/2013 2:05 PM, Ivan Gerasimov wrote:

David, Alan,

I added checking for NULL  results and throwing OOMException if necessary.


You don't need to throw it yourself:

  JNU_ThrowOutOfMemoryError(env, NULL);

Assuming a correct VM implementation if NULL is returned then an OOME 
should already be pending and will be thrown as soon as your native code 
returns to Java.


Thanks,
David


I've also added some spaces along the code to improve indentation.

Would you please review the updated webrev?
http://washi.ru.oracle.com/~igerasim/webrevs/8022584/1/webrev/

Sincerely yours,
Ivan


On 08.08.2013 5:35, David Holmes wrote:

On 8/08/2013 11:20 AM, Ivan Gerasimov wrote:

Thanks Alan!

I've checked that and it turns out that GetStringUTFChars cannot return
NULL.
For allocation of the result string it calls AllocateHeap() with the
default EXIT_OOM fail strategy.
Thus, in case of being unable to allocate memory it simply stops VM.


Ouch! That is a hotspot bug. GetStringUTFChars is supposed to throw
OutOfMemoryError if it has memory issues, not abort the VM!

I recommend that you check for NULL and/or a pending exception.

David


Sincerely yours,
Ivan

On 08.08.2013 5:05, Alan Bateman wrote:

(cc'ing net-dev).

The change looks okay to me. One suggestion (while you are there) is
to check the return from  GetStringUTFChars so that the name returns
when it fails with NULL.

-Alan.

On 07/08/2013 17:46, Ivan Gerasimov wrote:

Hello everybody!

Some methods of NetworkInterface class were reported to leak memory.
http://bugs.sun.com/view_bug.do?bug_id=JDK-8022584 (not yet
available.)

Examples are isUp() and isLoopback().

Affected versions of jdk are 6, 7 and 8

Would you please review a simple fix that removes the unnecessary
allocation?
http://cr.openjdk.java.net/~igerasim/8022584/0/webrev/

Sincerely yours,
Ivan













hg: jdk8/tl/jdk: 7147084: (process) appA hangs when read output stream of appB which starts appC that runs forever

2013-08-07 Thread alexey . utkin
Changeset: 2c4f1081a0fa
Author:uta
Date:  2013-08-08 09:16 +0400
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/2c4f1081a0fa

7147084: (process) appA hangs when read output stream of appB which starts appC 
that runs forever
Reviewed-by: alanb, robm, martin

! src/windows/classes/java/lang/ProcessImpl.java
! src/windows/native/java/lang/ProcessImpl_md.c
+ test/java/lang/ProcessBuilder/InheritIOEHandle.java
+ test/java/lang/ProcessBuilder/SiblingIOEHandle.java