Re: Please review changes for JDK-8012975: Remove rhino from jdk8

[A. Sundararajan]

On Friday 03 May 2013 11:53 AM, Tim Bell wrote:

On 05/ 2/13 01:24 PM, I wrote:

Hi Sundar:

Oracle JDK includes Rhino based javax.script implementation (which 
lives mostly in "closed" code). Rhino is being removed from Oracle 
JDK builds and there are the changes to the jdk open repository as 
well like com.sun.script.javascript package, makefiles etc. Please 
review the open jdk changes here:


http://cr.openjdk.java.net/~sundar/8012975/


This looks good.  Approved.

Tim


Sundar - we have had some breakage in the build forest recently, so to 
be extra careful I created a forest and then added your changes. I 
also did some blasting away with 'find ... -print | xargs egrep ...' 
commands to look for traces of rhino or javascript.


I think you need to look at removing these files as well:

jdk/make/com/sun/script/Makefile
jdk/make/sun/org/mozilla/javascript/Makefile

Tim



Thanks. Looks like the first one has not been removed. But second one 
was removed: hg stat shows


R make/sun/org/mozilla/javascript/Makefile

(also the webrev shows it as removed). Perhaps patch does not take care 
of deleted files?? I am not sure. Also, build seems to go through 
without removing first one!!


I'll remove that, build/test again and send another webrev.

Thanks
-Sundar

Re: Please review changes for JDK-8012975: Remove rhino from jdk8

[Tim Bell]

On 05/ 2/13 01:24 PM, I wrote:

Hi Sundar:

Oracle JDK includes Rhino based javax.script implementation (which 
lives mostly in "closed" code). Rhino is being removed from Oracle 
JDK builds and there are the changes to the jdk open repository as 
well like com.sun.script.javascript package, makefiles etc. Please 
review the open jdk changes here:


http://cr.openjdk.java.net/~sundar/8012975/


This looks good.  Approved.

Tim


Sundar - we have had some breakage in the build forest recently, so to 
be extra careful I created a forest and then added your changes. I also 
did some blasting away with 'find ... -print | xargs egrep ...' commands 
to look for traces of rhino or javascript.


I think you need to look at removing these files as well:

jdk/make/com/sun/script/Makefile
jdk/make/sun/org/mozilla/javascript/Makefile

Tim

hg: jdk8/tl/jdk: 7199324: Connection ID for IPv6 addresses is not generated accordingly to the specification

[jaroslav . bachorik]

Changeset: 470f19b6bfdd
Author:jbachorik
Date:  2013-05-02 13:21 +0200
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/470f19b6bfdd

7199324: Connection ID for IPv6 addresses is not generated accordingly to the 
specification
Summary: RemoteServer.getClientHost is returning a String with an IPv6 literal 
address and we need to enclose it in [] when building the connection id
Reviewed-by: alanb, sjiang

! src/share/classes/javax/management/remote/rmi/RMIServerImpl.java
! test/javax/management/remote/mandatory/connection/ConnectionTest.java

RFR 8012326: Deadlock occurs when Charset.availableCharsets() is called by several threads at the same time

[Xueming Shen]

Hi,

Please help review the proposed fix for 8012326.

The direct trigger is the fix we put in #6898310 [1], in which we added the
synchronization to prevent a possible race condition as described in 
$4685305.


However it appears the added synchronization triggers another race 
condition as

showed in this stack trace [4] when run the test case [2][3].

The root cause here is

(1) The ExtendedCharsetProvider is intended to be used as a singleton 
(as the

probeExtendedProvider + lookupExtendedCharset mechanism in Charset.java),
however this is only true for the 
Charset.forName()->lookup()...scenario. Multiple
instances of ExtendedCharsetProvider is being created in 
Charset.availableCharset()

every time it is invoked, via providers()/ServiceLoader.load().

(2) ISO2022_JP_2 and MSISO2022JP are provided via ExtendedCharsetProvider,
however both of them have two static variable DEC02{12|08}/ENC02{12|08} that
need to be initialized during the "class initialization", which will 
indirectly trigger

the invocation of ExtendedCharsetProvider.alisesFor(...)

(3) static method ExtendedCharsets.aliaseFor() has a hacky 
implementation that
goes to AbstractCharsetProvider.alise() for lookup, which needs to 
obtain a lock
on its ExtendedCharesetProvider.instance. This will potential cause race 
condition
if the "instance" it tries to synchronize on is not its "own" instance. 
This is possible
because the constructor of ExtendedCharsetProvider overwrites static 
"instance"

variable with "this".

Unfortunately as demonstrated  in [4], this appears to be what is happening.
The Thread-1/#9 is trying to synchronize on someone else's 
ExtendedCharsetProvider

instance <0xa4824730> (its own instance should be <0xa4835328>, which it
holds the monitor in the iterator.next()), it is blocked because 
Thread-0 already holds
the monitor of <0xa4824730> (in its iteratior.next()), but Thread-0 is 
blocked at
Class.forName0(), which I think is because Thread-1 is inside it 
already...two theads

are deadlocked.

A "quick fix/solution" is to remove the direct trigger in ISO2022_JP_2 and
MSISO2022JP to lazily-initialize those static variables as showed in the
webrev. However while this probably will solve the race condition, the 
multiple

instances of ExtendedCharsetProvider is really not desired. And given the
fact we have already hardwired ExtendedCharsetProvider (as well as the
StandardCharset, for performance reason) for charset lookup/forName(), the
availableCharsets() should also utilize the "singleton" 
ExtendedCharsetProvider
instanc (extendedCharsetProvider) as well, instead of going to the 
ServiceLoader

every time for a new instance. The suggested change is to use Charset.
extendedCharsetProvide via probeExtendedProvider() for extended charset
iteration (availableCharset()). Meanwhile, since the 
ExtendedCharsetProvider/
charsets.jar should always be in the boot classpath (if installed, and 
we are
looking up via Class.forName("sun.nio.cs.ext.ExtededCharsetProvider")), 
there

is really no need for it to be looked up/loaded via the spi mechanism (in
fact it's redundant to load it again after we have lookup/iterate the
hardwired "extendedCharsetProvider". So I removed the spi description from
the charsts.jar as well.

An alternative is to really implement the singleton pattern in ECP, however
given the existing mechanism we have in Charset.java and the "hacky" init()
implementation we need in ECP, I go with the current approach.

http://cr.openjdk.java.net/~sherman/8012326/webrev

Thanks,
Sherman

[1] http://cr.openjdk.java.net/~sherman/6898310/webrev/
[2] http://cr.openjdk.java.net/~sherman/8012326/runtest.bat
[3] http://cr.openjdk.java.net/~sherman/8012326/Test.java
[4] http://cr.openjdk.java.net/~sherman/8012326/stacktrace

hg: jdk8/tl/jdk: 8013855: DigestMD5Client has not checked RealmChoiceCallback value

[weijun . wang]

Changeset: 81be41c7323f
Author:weijun
Date:  2013-05-03 10:43 +0800
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/81be41c7323f

8013855: DigestMD5Client has not checked RealmChoiceCallback value
Reviewed-by: xuelei, mullan

! src/share/classes/com/sun/security/sasl/digest/DigestMD5Client.java
+ test/com/sun/security/sasl/digest/AuthRealmChoices.java

Re: Review Request: BigInteger patch for efficient multiplication and division (#4837946)

[Brian Burkhalter]

On May 2, 2013, at 5:02 AM, Alan Bateman wrote:

> On 02/05/2013 02:34, Tim Buktu wrote:
>> :
>> 
>> Alan is working on an improved BigInteger.toString() that should be
>> dramatically faster for large numbers. What's the deadline for getting
>> this in?
>> Thanks,
>> 
>> Tim
> Here's the latest milestone dates:
> 
> http://openjdk.java.net/projects/jdk8/milestones
> 
> Given the size of the patch then it would be great to get it in as early as 
> possible. With the review effort then I assume Feature Complete is too tight 
> although I know Brian Burkhalter is anxious to get it in as soon as possible. 
> I can't comment on the BigInteger.toString work to know how big this is.

I think that's the best answer that can be given: as soon as possible. The 
4837946 patch is far from simple to comprehend to say the least and then there 
is the testing so it's not going to be fast. If the toString() work is ready 
early enough to review then perhaps we can handle it, but I don't want to make 
anyone hurry to complete something and then say we cannot do it.

Thanks,

Brian

Re: Review Request for JDK-8003992: File and other classes in java.io do not handle embedded nulls properly

[Dan Xu]

Hi All,

Thanks for all your comments. Based on the previous feedback, I have 
moved to the other approach, i.e., to fail file operations if the 
invalid NUL characher is found in a file path. As you know, due to the 
compatibility issue, we cannot throw an exception immediately in the 
File constructors. So the failure is delayed and only shown up when any 
file operation is triggered.


As for FileInputStream, FileOutputStream, and RandomAccessFile classes, 
the FileNotFoundException will be thrown right away since their spec 
allow this exception happen in the constructors. Thanks for your review!


webrev: http://cr.openjdk.java.net/~dxu/8003992/webrev.01/

-Dan

hg: jdk8/tl/jdk: 8013140: Heap corruption with NetworkInterface.getByInetAddress() and long i/f name

[kurchi . subhra . hazra]

Changeset: 3062bf908281
Author:khazra
Date:  2013-05-02 14:26 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/3062bf908281

8013140: Heap corruption with NetworkInterface.getByInetAddress() and long i/f 
name
Summary: Remove buffer overruns in native code
Reviewed-by: alanb, chegar

! src/solaris/native/java/net/NetworkInterface.c

Re: Please review changes for JDK-8012975: Remove rhino from jdk8

[Tim Bell]

Hi Sundar:

Oracle JDK includes Rhino based javax.script implementation (which 
lives mostly in "closed" code). Rhino is being removed from Oracle JDK 
builds and there are the changes to the jdk open repository as well 
like com.sun.script.javascript package, makefiles etc. Please review 
the open jdk changes here:


http://cr.openjdk.java.net/~sundar/8012975/


This looks good.  Approved.

Tim

Re: RFR : 7178639 : (XXS) Remove incorrect documentation from Deque.push(E e)

[Martin Buchholz]

My proposed changes are now in JSR166 CVS.
Syncing work is needed before jdk8 ships!

Re: RFR: 8011814/8013271/8013272: Three improvements to J2SE Netbeans project

[Mike Duigou]

As a followup to this issue it's been pointed out that I pushed this patch with 
source-level set to "1.8" which is not supported by Netbeans 7.2 or 7.3. Source 
1.8 is supported by the development version of Netbeans which is what I use 
primarily.

There's a problem here:

- The JDK repo now contains Java 8 source and over time will include much more 
Java 8 source (lambdas, default methods, static methods on interfaces, etc.).

- The release versions of Netbeans don't understand Java 8 source.


So we have to choose between using release versions of Netbeans and accept 
broken source parsing and using dev versions of Netbeans and the likely bugs 
(and missing extensions) that will be encountered.

Should we change the source level back to 1.7 for now?  My vote is "No", but 
I'm not the decider.

Mike

On Apr 29 2013, at 19:11 , Mike Duigou wrote:

> Hello All;
> 
> This is a review for three changes to the J2SE Netbeans project. If necessary 
> I can break this up into three separate patches but I would rather not if 
> possible.
> 
> 
> http://cr.openjdk.java.net/~mduigou/JDK-8011814/0/webrev/
> 
> 
> 8011814: Add testng.jar to Netbeans projects test compile classpath
> 
> An increasing number of jtreg tests now use TestNG. This change adds the 
> TestNG jar from you JTReg installation to the tests classpath. The location 
> of JTReg is specified in build.properties using jtreg.home or from the 
> environment via JT_HOME.
> 
> 
> 8013271: Add OS X sources to J2SE Netbeans project
> 
> Adds as source entry for Apple OS X sources to match the Unix and Windows 
> entries. I checked the trademark with the Apple Trademarks page to make sure 
> I got it correct.
> 
> 
> 8013272: JDK Netbeans projects should use ASCII encoding for sources
> 
> The build scripts compile all OpenJDK java sources using the US-ASCII 
> encoding. This change causes Netbeans to respect this encoding. Whether 
> US-ASCII is the awesomest encoding is certainly debatable, but all editors 
> and IDEs should use what the compiler uses.
> 
> 
> Thanks for reviewing,
> 
> Mike

Re: (Preliminary) RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[Peter Levart]

On 04/30/2013 04:57 PM, Thomas Schatzl wrote:

Hi all,

   the webrev at http://cr.openjdk.java.net/~tschatzl/7038914/webrev/
presents a first stab at the CR "7038914: VM could throw uncaught OOME
in ReferenceHandler thread".

The problem is that under very heavy memory pressure, there is the
reference handler throws an exception with the message "Exception:
java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in
thread "Reference Handler".

The change improves handling of out-of-memory conditions in the
ReferenceHandler thread. Instead of crashing the thread, and then
disabling reference processing, it catches this exception and continues.

I'd like to discuss the change as I'm not really familiar with JDK
coding style, handling of such situations and have some questions about
it.

Bugs.sun
http://bugs.sun.com/view_bug.do?bug_id=7038914

JBS:
https://jbs.oracle.com/bugs/browse/JDK-7038914

Proposed webrev:
http://cr.openjdk.java.net/~tschatzl/7038914/webrev/

- first, I could not reliably reproduce the issue using the information
in the CR. Only via code review (and an idea from Bengt Rutisson -
thanks!) I implemented a nice way to reproduce an OOME in the reference
handler. This involves implementing a custom
java.lang.ref.ReferenceQueue and overriding the enqueue() method, and
doing some allocation that causes an OOME within that method.
My current theory is that synchronization/locking allocates some objects
on the java heap, which are very small, so an OOME in that thread can be
caused. I walked the locking code, but could not find a java heap
allocation there (ObjectMonitor seems to be a C heap object) - maybe I
overlooked it. Probably somebody else knows?


Hi Tomas,

I don't know if this is the case here, but what if the ReferenceHandler 
thread is interrupted while wait()-ing and the construction of 
InterruptedException triggers OOME?


Regards, Peter


It cannot be the invocation of the Cleaner.clean() methods above the
enqueuing since it has it's own try-catch block already.
Anyway, since the reproducer I wrote shows the same symptoms as reported
in the CR, I hope that this test case is sufficient to be regarded as a
reproducer and the change as a fix.

- the actual change in java/lang/ref/Reference as mentioned involves
putting the entire main enqueuing procedure within a try-catch block.
It only catches OOME to decrease the possibility to catch anything that
should not be caught.
The problem is that this fix does not (and cannot) really fix bad
programming in anyone overriding java.lang.ref.ReferenceQueue.enqueue(),
i.e. if the OOME condition is before the actual execution of the
original enqueue() method, i.e. corruption of the queue may be still
possible.
On the other hand, since overriding ReferenceQueue.enqueue() requires
putting the custom ReferenceQueue into the boot class path, I assume
that people doing that are aware of possible issues.

- handling the OOME: in the catch block of the I put a block

 // avoid crashing the reference handler thread,
 // but provide for some diagnosability
 assert false : e.toString();

to provide some diagnosability in the case of an exception (when
running with assertions). I copied that from other code that tries to
catch similar problems in the clean() method of the Cleaners. There are
other variants of managing this in the jdk, some involving calling
system.exit(). I thought that was too drastic, so I didn't do that, but
what is the appropriate way to handle this situation?

- if the use of locks or the synchronization keyword is indeed the
problem, I think it is possible to use nonblocking synchronization that
is known to not allocate any memory for managing the reference queues
instead. However I think to guard against misbehaving ReferenceQueue
implementations you'd still want to have a try-catch block here.

- is the location of the test correct? I.e. in the jdk
test/java/lang/ref directory? Or is the correct place for that the
hotspot test directories?

Since this is (seems to be) a JDK only change, and this is my first time
changing the JDK, I hope core-libs-dev is the right mailing list.
Otherwise please direct me to the the appropriate one.

Thanks,
Thomas

Re: RFR: 7150256/8004095: Add back Remote Diagnostic Commands

[frederic parain]

Karen,

Thank you for the review.

1. 2. and 3. have been fixed.

Regarding 4.

Most (all?) permissions have a name argument.
However, if p._name is null, the string "(null)" is printed.
This is not wrong, but not very pretty:
Permission: MyPermission((null)).

I've changed the code (both 105 and 109) with a short conditional:
   ... p._name == NULL ? "null" : p._name ...

which gives a better output:

Permission: MyPermission(null)

Fred

On 02/05/2013 18:37, Karen Kinnear wrote:

Frederic,

Code looks good - actually it looks very clean. Ship it.

Couple of minor comments that don't require re-review:

1. nmtDCmd.hpp/cpp - copyrights 2012 -> 2012, 2013

2. jmm.h
   line 213: "True is" -> "True if"

3. diagnosticFramework.hpp
   Thank you for the comments!
   line 298 "rational" -> "rationale"

4. diagnosticCommand.cpp
   lines 105/109 - what prints if p._name is null?

thanks,
Karen

On Apr 30, 2013, at 12:26 PM, frederic parain wrote:


Hi all,

This is a second request for review to add back
Remote Diagnostic Commands.

This work adds a new platform MBean providing
remote access to the diagnostic command framework
via JMX (already accessible locally with the jcmd
tool).

There's two CR number because this work is made of two
parts pushed to two different repositories.

JDK changeset CR 7150256
http://cr.openjdk.java.net/~fparain/7150256/webrev.06/

HotSpot changeset: CR 8004095
http://cr.openjdk.java.net/~fparain/8004095/webrev.06/

Questions from previous review have been answered
in initial review threads. Changesets also include
some minor changes coming from internal audit and
feedback sent in private e-mails.

However, one issue is still pending: some unit tests
use a hard coded port number, which could cause test
failures if several instances of the same test are
run on the same machine. I propose to postpone the
fix of this issue after the JDK8 feature freeze
(leaving for vacations soon, I won't have time to
fix tests before the feature freeze).

Thanks,

Fred

--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com




--
Frederic Parain - Oracle
Grenoble Engineering Center - France
Phone: +33 4 76 18 81 17
Email: frederic.par...@oracle.com

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Martin Buchholz]

Oops.  Of course, that shouldn't have the trailing semicolon

#define NULL_CHECK_OR_RETURN(ncor_pointer_to_check, \
 ncor_failure_return_value) \
  do {  \
if ((ncor_pointer_to_check) == NULL) {  \
  JLI_ReportErrorMessage(JNI_ERROR);\
  return ncor_failure_return_value; \
}   \
  } while(0)



On Thu, May 2, 2013 at 11:16 AM, Martin Buchholz wrote:

> What would Martin actually do?
>
> #define NULL_CHECK_OR_RETURN(ncor_pointer_to_check, \
>  ncor_failure_return_value) \
>   do {  \
> if ((ncor_pointer_to_check) == NULL) {  \
>   JLI_ReportErrorMessage(JNI_ERROR);\
>   return ncor_failure_return_value; \
> }   \
>   } while(0);
>
>
>
>
> On Thu, May 2, 2013 at 10:45 AM, Kumar Srinivasan <
> kumar.x.sriniva...@oracle.com> wrote:
>
>>  On 5/2/2013 10:17 AM, Martin Buchholz wrote:
>>
>> This is global fix creep, but ...
>>
>>
>> :(
>>
>>
>>
>>  these macros are also found in the hotspot sources.
>> I would rewrite all the macros in the jdk to adopt the blessed style
>> do { ... } while(0)
>> and remove all comments in the jdk of the form
>>   /* next token must be ; */
>>  If you want a macro that does nothing at all, you should define it
>> do {} while (0)
>> instead of defining it to the empty string.
>>
>> I am not following, could you be more explicit on how this applies to
>>
>> -#define NULL_CHECK0(e) if ((e) == 0) { \+#define NULL_CHECK_RV(e, rv) if 
>> ((e) == 0) { \
>>  JLI_ReportErrorMessage(JNI_ERROR); \-return 0; \+return rv; \
>>}
>>  -#define NULL_CHECK(e) if ((e) == 0) { \-
>> JLI_ReportErrorMessage(JNI_ERROR); \-return; \-  }+#define 
>> NULL_CHECK0(e) NULL_CHECK_RV(e, 0)
>>  +#define NULL_CHECK(e) NULL_CHECK_RV(e, )+
>>
>>
>>
>>
>>  I would also be inclined to change
>>> == 0
>>> to
>>> == NULL
>>>
>>>
>> Yes will take care of this, as well as Alan suggestion added a check for
>> malloc return.
>>
>>
>>This seems like another occasion to use the weird
>>>
>>>  do { ... } while(0)
>>>
>>>  trick to make the macro behave more like a statement.
>>>
>>>  I might obfuscate the macro parameters to make collisions less likely,
>>> e.g. e => N_C_RV_e
>>>
>>
>> You want me to rename the macro parameter e to N_C_RV_e ? or something
>> else
>> say ncrve to avoid collision ?
>>
>> Kumar
>>
>>
>>
>>>
>>>
>>>
>>> On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan <
>>> kumar.x.sriniva...@oracle.com> wrote:
>>>
 Hi,

 Please review simple fixes for code correctness in the launcher.

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

 Thanks
 Kumar


>>>
>>
>>
>

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Martin Buchholz]

What would Martin actually do?

#define NULL_CHECK_OR_RETURN(ncor_pointer_to_check, \
 ncor_failure_return_value) \
  do {  \
if ((ncor_pointer_to_check) == NULL) {  \
  JLI_ReportErrorMessage(JNI_ERROR);\
  return ncor_failure_return_value; \
}   \
  } while(0);




On Thu, May 2, 2013 at 10:45 AM, Kumar Srinivasan <
kumar.x.sriniva...@oracle.com> wrote:

>  On 5/2/2013 10:17 AM, Martin Buchholz wrote:
>
> This is global fix creep, but ...
>
>
> :(
>
>
>
>  these macros are also found in the hotspot sources.
> I would rewrite all the macros in the jdk to adopt the blessed style
> do { ... } while(0)
> and remove all comments in the jdk of the form
>   /* next token must be ; */
>  If you want a macro that does nothing at all, you should define it
> do {} while (0)
> instead of defining it to the empty string.
>
> I am not following, could you be more explicit on how this applies to
>
> -#define NULL_CHECK0(e) if ((e) == 0) { \+#define NULL_CHECK_RV(e, rv) if 
> ((e) == 0) { \
>  JLI_ReportErrorMessage(JNI_ERROR); \-return 0; \+return rv; \
>}
>  -#define NULL_CHECK(e) if ((e) == 0) { \-
> JLI_ReportErrorMessage(JNI_ERROR); \-return; \-  }+#define NULL_CHECK0(e) 
> NULL_CHECK_RV(e, 0)
>  +#define NULL_CHECK(e) NULL_CHECK_RV(e, )+
>
>
>
>
>  I would also be inclined to change
>> == 0
>> to
>> == NULL
>>
>>
> Yes will take care of this, as well as Alan suggestion added a check for
> malloc return.
>
>
>This seems like another occasion to use the weird
>>
>>  do { ... } while(0)
>>
>>  trick to make the macro behave more like a statement.
>>
>>  I might obfuscate the macro parameters to make collisions less likely,
>> e.g. e => N_C_RV_e
>>
>
> You want me to rename the macro parameter e to N_C_RV_e ? or something else
> say ncrve to avoid collision ?
>
> Kumar
>
>
>
>>
>>
>>
>> On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan <
>> kumar.x.sriniva...@oracle.com> wrote:
>>
>>> Hi,
>>>
>>> Please review simple fixes for code correctness in the launcher.
>>>
>>> http://cr.openjdk.java.net/~ksrini/8013736/webrev.0/
>>>
>>> Thanks
>>> Kumar
>>>
>>>
>>
>
>

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Kumar Srinivasan]

On 5/2/2013 10:17 AM, Martin Buchholz wrote:

This is global fix creep, but ...


:(



these macros are also found in the hotspot sources.
I would rewrite all the macros in the jdk to adopt the blessed style
do { ... } while(0)
and remove all comments in the jdk of the form
 /* next token must be ; */
If you want a macro that does nothing at all, you should define it
do {} while (0)
instead of defining it to the empty string.

I am not following, could you be more explicit on how this applies to

-#define NULL_CHECK0(e) if ((e) == 0) { \
+#define NULL_CHECK_RV(e, rv) if ((e) == 0) { \
 JLI_ReportErrorMessage(JNI_ERROR); \
-return 0; \
+return rv; \
   }
 
-#define NULL_CHECK(e) if ((e) == 0) { \

-JLI_ReportErrorMessage(JNI_ERROR); \
-return; \
-  }
+#define NULL_CHECK0(e) NULL_CHECK_RV(e, 0)
 
+#define NULL_CHECK(e) NULL_CHECK_RV(e, )

+





I would also be inclined to change
== 0
to
== NULL



Yes will take care of this, as well as Alan suggestion added a check for 
malloc return.



This seems like another occasion to use the weird

do { ... } while(0)

trick to make the macro behave more like a statement.

I might obfuscate the macro parameters to make collisions less
likely, e.g. e => N_C_RV_e



You want me to rename the macro parameter e to N_C_RV_e ? or something else
say ncrve to avoid collision ?

Kumar






On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan
mailto:kumar.x.sriniva...@oracle.com>> wrote:

Hi,

Please review simple fixes for code correctness in the launcher.

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


Thanks
Kumar

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Martin Buchholz]

This is global fix creep, but ...

these macros are also found in the hotspot sources.
I would rewrite all the macros in the jdk to adopt the blessed style
do { ... } while(0)
and remove all comments in the jdk of the form
 /* next token must be ; */
If you want a macro that does nothing at all, you should define it
do {} while (0)
instead of defining it to the empty string.




On Thu, May 2, 2013 at 9:51 AM, Martin Buchholz  wrote:

> I would also be inclined to change
> == 0
> to
> == NULL
>
> This seems like another occasion to use the weird
>
> do { ... } while(0)
>
> trick to make the macro behave more like a statement.
>
> I might obfuscate the macro parameters to make collisions less likely,
> e.g. e => N_C_RV_e
>
>
>
>
> On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan <
> kumar.x.sriniva...@oracle.com> wrote:
>
>> Hi,
>>
>> Please review simple fixes for code correctness in the launcher.
>>
>> http://cr.openjdk.java.net/~**ksrini/8013736/webrev.0/
>>
>> Thanks
>> Kumar
>>
>>
>

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Martin Buchholz]

I would also be inclined to change
== 0
to
== NULL

This seems like another occasion to use the weird

do { ... } while(0)

trick to make the macro behave more like a statement.

I might obfuscate the macro parameters to make collisions less likely, e.g.
e => N_C_RV_e




On Wed, May 1, 2013 at 12:33 PM, Kumar Srinivasan <
kumar.x.sriniva...@oracle.com> wrote:

> Hi,
>
> Please review simple fixes for code correctness in the launcher.
>
> http://cr.openjdk.java.net/~**ksrini/8013736/webrev.0/
>
> Thanks
> Kumar
>
>

Re: Please review changes for JDK-8012975: Remove rhino from jdk8

[Alan Bateman]

On 02/05/2013 17:41, A. Sundararajan wrote:

Hi,

Oracle JDK includes Rhino based javax.script implementation (which 
lives mostly in "closed" code). Rhino is being removed from Oracle JDK 
builds and there are the changes to the jdk open repository as well 
like com.sun.script.javascript package, makefiles etc. Please review 
the open jdk changes here:


http://cr.openjdk.java.net/~sundar/8012975/
In profiles-rtjar-includes.txt then you also need to remove 
META-INF/services/javax.script.ScriptEngineFactory from 
PROFILE_3_INCLUDE_METAINF_SERVICES.


Otherwise good fine.

-Alan.

Please review changes for JDK-8012975: Remove rhino from jdk8

[A. Sundararajan]

Hi,

Oracle JDK includes Rhino based javax.script implementation (which lives 
mostly in "closed" code). Rhino is being removed from Oracle JDK builds 
and there are the changes to the jdk open repository as well like 
com.sun.script.javascript package, makefiles etc. Please review the open 
jdk changes here:


http://cr.openjdk.java.net/~sundar/8012975/

Thanks,
-Sundar

Re: RFR: 7150256/8004095: Add back Remote Diagnostic Commands

[Karen Kinnear]

Frederic,

Code looks good - actually it looks very clean. Ship it.

Couple of minor comments that don't require re-review:

1. nmtDCmd.hpp/cpp - copyrights 2012 -> 2012, 2013

2. jmm.h
  line 213: "True is" -> "True if"

3. diagnosticFramework.hpp
  Thank you for the comments!
  line 298 "rational" -> "rationale"

4. diagnosticCommand.cpp
  lines 105/109 - what prints if p._name is null?

thanks,
Karen

On Apr 30, 2013, at 12:26 PM, frederic parain wrote:

> Hi all,
> 
> This is a second request for review to add back
> Remote Diagnostic Commands.
> 
> This work adds a new platform MBean providing
> remote access to the diagnostic command framework
> via JMX (already accessible locally with the jcmd
> tool).
> 
> There's two CR number because this work is made of two
> parts pushed to two different repositories.
> 
> JDK changeset CR 7150256
> http://cr.openjdk.java.net/~fparain/7150256/webrev.06/
> 
> HotSpot changeset: CR 8004095
> http://cr.openjdk.java.net/~fparain/8004095/webrev.06/
> 
> Questions from previous review have been answered
> in initial review threads. Changesets also include
> some minor changes coming from internal audit and
> feedback sent in private e-mails.
> 
> However, one issue is still pending: some unit tests
> use a hard coded port number, which could cause test
> failures if several instances of the same test are
> run on the same machine. I propose to postpone the
> fix of this issue after the JDK8 feature freeze
> (leaving for vacations soon, I won't have time to
> fix tests before the feature freeze).
> 
> Thanks,
> 
> Fred
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: frederic.par...@oracle.com

Re: RFEs implementing JEP 170

[Lance @ Oracle]

Yes I am away right now but will follow up when I am back mid next week

Best
Lance


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

On May 2, 2013, at 11:45 AM, Alan Bateman  wrote:

> On 02/05/2013 12:13, Neil Richards wrote:
>> :
>> 
>> That conversation suggests there should be other RFEs concerned with
>> the remaining implementation of JEP 170, but I haven't been able to
>> determine which those are.
> I'm sure Lance will add the links but in the mean-time then "hg log 
> src/share/classes/java/sql" and "hg log src/share/classes/javax/sql" should 
> help you find the recent changes.
> 
> -Alan.

hg: jdk8/tl/jdk: 8012645: Stream methods on BitSet, Random, ThreadLocalRandom, ZipFile

[mike . duigou]

Changeset: 5045eb04a579
Author:mduigou
Date:  2013-05-02 09:18 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/5045eb04a579

8012645: Stream methods on BitSet, Random, ThreadLocalRandom, ZipFile
Reviewed-by: mduigou, henryjen, alanb, martin, psandoz
Contributed-by: akhil.ar...@oracle.com, brian.go...@oracle.com

! src/share/classes/java/util/BitSet.java
! src/share/classes/java/util/Random.java
! src/share/classes/java/util/concurrent/ThreadLocalRandom.java
! src/share/classes/java/util/jar/JarFile.java
! src/share/classes/java/util/zip/ZipFile.java
+ test/java/util/BitSet/BitSetStreamTest.java
+ test/java/util/Random/RandomStreamTest.java
+ test/java/util/zip/ZipFile/StreamZipEntriesTest.java

Re: RFR: JDK-8008738 - Issue in com.sun.org.apache.xml.internal.serializer.Encodings causes some JCK tests to fail intermittently

[Daniel Fuchs]

Hi,

Please find an updated webrev below:


Changes are:

catch IAE are removed - as suggested by Alan.
I didn't find traces of IAE being thrown by OutputStreamWriter
constructor in recent JDK - so I think it's safe to remove
those catch clauses.

changes in test Makefile have been removed: I rebased my patch on
jdk8 tip, which already has these test Makefile changes.

best regards,

-- daniel

On 4/30/13 11:18 AM, Alan Bateman wrote:

On 15/04/2013 15:09, Daniel Fuchs wrote:

Hi,

This a fix for:

JDK-8008738 - Issue in
com.sun.org.apache.xml.internal.serializer.Encodings causes some JCK
tests to fail intermittently.




I skimmed over the webrev and it looks reasonable to me.

In getWriter then I don't understand the "java 1.1.8" comment, I wonder
if the catching of IAE should just be removed.

For jdk_other in the test Makefile then it might be simpler to just add
javax/xml and drop the existing soap and ws directories. I suggest this
because there are only a tiny number of JAXP and JAX-WS tests in the jdk
repo and they are all run via the jdk_other target.

-Alan

Re: RFR JDK-8003258(2nd round): BufferedReader.lines()

[Henry Jen]

On 05/02/2013 05:08 AM, Alan Bateman wrote:
> On 02/05/2013 07:49, Henry Jen wrote:
>> Hi,
>>
>> Take feedbacks from previous round, the javadoc is updated
>>
>> http://cr.openjdk.java.net/~henryjen/ccc/8003258.2/webrev/
>> http://cr.openjdk.java.net/~henryjen/ccc/8003258.2/specdiff/
>>
> This looks good to me except for the last two sentences of the final
> paragraph:
> 
> "For example, when trying toread from the Stream after the
> BufferedReaderis closed, will throw an UncheckedIOException. Note
> thatmethod will return the Stream Stream even if this BufferedReader is
> closed, but the operation cause reading will throwUncheckedIOException."
> 
> How about:
> 
> "This method will return a Stream if invoked on a BufferedReader that is
> closed. Any operation on that stream requires reading from the
> BufferedReader after is it closed will cause an UncheckedIOException to
> be thrown".
> 

Updated as suggested.

> Otherwise I think we are at the finish line.
>

Cheers,
Henry

Re: RFR: JDK-8006884: (fs) Add Files.list, lines and find

[Henry Jen]

On May 2, 2013, at 8:52 AM, Henry Jen  wrote:

> 
> On May 2, 2013, at 8:22 AM, Alan Bateman  wrote:
> 
>> On 02/05/2013 07:52, Henry Jen wrote:
>>> Hi,
>>> 
>>> Please review a couple stream access API proposed for
>>> java.nio.file.Files and java.nio.file.DirectoryStream,
>>> 
>>> http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/webrev/
>>> http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/specdiff/
>>> 
>>> Cheers,
>>> Henry
>> One corner case that I meant to ask about is the expected behavior when 
>> someone attempts to do something on a CloseableStream after it is closed? 
>> It's not specified so it's not testable but I'm just wondering if the 
>> Iterator throwing ISE is right or whether this should be an 
>> UncheckedIOException. As I understand it, an ISE will be thrown if someone 
>> attempts to use a stream that already been operated on, so this really just 
>> leaves the uninteresting case where the stream is closed before using it.
> 
> I think UncheckedIOException is expected as read on a closed InputStream 
> should throw IOException.
> 
> A related question, what do we expect when iterate a DirectoryStream which is 
> closed after we obtain the iterator?

Answer myself, as end of stream expected.

Cheers,
Henry

Re: RFR: JDK-8006884: (fs) Add Files.list, lines and find

[Henry Jen]

On May 2, 2013, at 8:22 AM, Alan Bateman  wrote:

> On 02/05/2013 07:52, Henry Jen wrote:
>> Hi,
>> 
>> Please review a couple stream access API proposed for
>> java.nio.file.Files and java.nio.file.DirectoryStream,
>> 
>> http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/webrev/
>> http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/specdiff/
>> 
>> Cheers,
>> Henry
> One corner case that I meant to ask about is the expected behavior when 
> someone attempts to do something on a CloseableStream after it is closed? 
> It's not specified so it's not testable but I'm just wondering if the 
> Iterator throwing ISE is right or whether this should be an 
> UncheckedIOException. As I understand it, an ISE will be thrown if someone 
> attempts to use a stream that already been operated on, so this really just 
> leaves the uninteresting case where the stream is closed before using it.

I think UncheckedIOException is expected as read on a closed InputStream should 
throw IOException.

A related question, what do we expect when iterate a DirectoryStream which is 
closed after we obtain the iterator?

Cheers,
Henry

Re: RFEs implementing JEP 170

[Alan Bateman]

On 02/05/2013 12:13, Neil Richards wrote:

:

That conversation suggests there should be other RFEs concerned with
the remaining implementation of JEP 170, but I haven't been able to
determine which those are.

I'm sure Lance will add the links but in the mean-time then "hg log 
src/share/classes/java/sql" and "hg log src/share/classes/javax/sql" 
should help you find the recent changes.


-Alan.

Re: RFR: JDK-8006884: (fs) Add Files.list, lines and find

[Alan Bateman]

On 02/05/2013 07:52, Henry Jen wrote:

Hi,

Please review a couple stream access API proposed for
java.nio.file.Files and java.nio.file.DirectoryStream,

http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/webrev/
http://cr.openjdk.java.net/~henryjen/ccc/8006884.0/specdiff/

Cheers,
Henry
One corner case that I meant to ask about is the expected behavior when 
someone attempts to do something on a CloseableStream after it is 
closed? It's not specified so it's not testable but I'm just wondering 
if the Iterator throwing ISE is right or whether this should be an 
UncheckedIOException. As I understand it, an ISE will be thrown if 
someone attempts to use a stream that already been operated on, so this 
really just leaves the uninteresting case where the stream is closed 
before using it.


-Alan.

Re: RFR: 8013155: [pack200] improve performance of pack200

[Kumar Srinivasan]

Looks good.

Kumar


Kumar,

On 4/30/13 22:32, Kumar Srinivasan wrote:

Couple of nits:
I don't think you need the parens

j = (nextsemi < nextangl ? nextsemi : nextangl);
Here i tend to agree with John - the condition being a superposition 
of two really closely named
variables might look confusing and since the world supply of 
parentheses is not limited i will humbly

ask for permission to leave one extra pair of them there.


Coding conventions nits:
missing spaces for operators.

for (int i = firstl+1, j; i > 0; i = sig.indexOf('L', j)+1) {

Fixed.
I take it all the regression tests have been run ? in 
jdk/test/tools/pack200.
Sure - since it is not platform-specific code i took liberty to 
perform testing on Mac OS and Linux only - all the pack200

regression tests are passed.

The updated webrev can be seen here: 
http://cr.openjdk.java.net/~kizune/8013155/webrev.01


With best regards,
Alex

Re: RFR: 8013155: [pack200] improve performance of pack200

[Alexander Zuev]

Kumar,

On 4/30/13 22:32, Kumar Srinivasan wrote:

Couple of nits:
I don't think you need the parens

j = (nextsemi < nextangl ? nextsemi : nextangl);
Here i tend to agree with John - the condition being a superposition of 
two really closely named
variables might look confusing and since the world supply of parentheses 
is not limited i will humbly

ask for permission to leave one extra pair of them there.


Coding conventions nits:
missing spaces for operators.

for (int i = firstl+1, j; i > 0; i = sig.indexOf('L', j)+1) {

Fixed.
I take it all the regression tests have been run ? in 
jdk/test/tools/pack200.
Sure - since it is not platform-specific code i took liberty to perform 
testing on Mac OS and Linux only - all the pack200

regression tests are passed.

The updated webrev can be seen here: 
http://cr.openjdk.java.net/~kizune/8013155/webrev.01


With best regards,
Alex

hg: jdk8/tl/jdk: 8013225: Refresh jdk's private ASM to the latest.

[kumar . x . srinivasan]

Changeset: 167d2dca
Author:ksrini
Date:  2013-05-01 15:08 -0700
URL:   http://hg.openjdk.java.net/jdk8/tl/jdk/rev/167d2dca

8013225: Refresh jdk's private ASM to the latest.
Reviewed-by: mduigou, sundar

! src/share/classes/jdk/internal/org/objectweb/asm/AnnotationVisitor.java
! src/share/classes/jdk/internal/org/objectweb/asm/AnnotationWriter.java
! src/share/classes/jdk/internal/org/objectweb/asm/Attribute.java
! src/share/classes/jdk/internal/org/objectweb/asm/ByteVector.java
! src/share/classes/jdk/internal/org/objectweb/asm/ClassReader.java
! src/share/classes/jdk/internal/org/objectweb/asm/ClassVisitor.java
! src/share/classes/jdk/internal/org/objectweb/asm/ClassWriter.java
+ src/share/classes/jdk/internal/org/objectweb/asm/Context.java
! src/share/classes/jdk/internal/org/objectweb/asm/FieldVisitor.java
! src/share/classes/jdk/internal/org/objectweb/asm/FieldWriter.java
! src/share/classes/jdk/internal/org/objectweb/asm/Frame.java
! src/share/classes/jdk/internal/org/objectweb/asm/Handle.java
! src/share/classes/jdk/internal/org/objectweb/asm/Handler.java
! src/share/classes/jdk/internal/org/objectweb/asm/Item.java
! src/share/classes/jdk/internal/org/objectweb/asm/Label.java
! src/share/classes/jdk/internal/org/objectweb/asm/MethodVisitor.java
! src/share/classes/jdk/internal/org/objectweb/asm/MethodWriter.java
! src/share/classes/jdk/internal/org/objectweb/asm/Opcodes.java
! src/share/classes/jdk/internal/org/objectweb/asm/Type.java
+ src/share/classes/jdk/internal/org/objectweb/asm/TypePath.java
+ src/share/classes/jdk/internal/org/objectweb/asm/TypeReference.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/AdviceAdapter.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/AnalyzerAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/CodeSizeEvaluator.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/GeneratorAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/InstructionAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/JSRInlinerAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/LocalVariablesSorter.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/Method.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/Remapper.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/RemappingAnnotationAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/RemappingClassAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/RemappingFieldAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/RemappingMethodAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/RemappingSignatureAdapter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/SerialVersionUIDAdder.java
! src/share/classes/jdk/internal/org/objectweb/asm/commons/StaticInitMerger.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/TableSwitchGenerator.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/commons/TryCatchBlockSorter.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/signature/SignatureReader.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/signature/SignatureVisitor.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/signature/SignatureWriter.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/AbstractInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/AnnotationNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/ClassNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/FieldInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/FieldNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/FrameNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/IincInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/InnerClassNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/InsnList.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/InsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/IntInsnNode.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/tree/InvokeDynamicInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/JumpInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/LdcInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/LineNumberNode.java
+ 
src/share/classes/jdk/internal/org/objectweb/asm/tree/LocalVariableAnnotationNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/LocalVariableNode.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/tree/LookupSwitchInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/MethodInsnNode.java
! src/share/classes/jdk/internal/org/objectweb/asm/tree/MethodNode.java
! 
src/share/classes/jdk/internal/org/objectweb/asm/tree/MultiANewArrayInsnNode.java
+ src/share/classes/jdk/inte

Re: RFR: JDK-8013736: [launcher] cleanup code for correctness

[Alan Bateman]

On 01/05/2013 20:33, Kumar Srinivasan wrote:

Hi,

Please review simple fixes for code correctness in the launcher.

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

Thanks
Kumar

This looks okay to me although I wonder why the NULL_CHECK macros check 
for 0 rather than NULL. I don't know if jexec is still used but does the 
malloc return need to be checked?


-Alan

Re: RFR JDK-8003258(2nd round): BufferedReader.lines()

[Alan Bateman]

On 02/05/2013 07:49, Henry Jen wrote:

Hi,

Take feedbacks from previous round, the javadoc is updated

http://cr.openjdk.java.net/~henryjen/ccc/8003258.2/webrev/
http://cr.openjdk.java.net/~henryjen/ccc/8003258.2/specdiff/

This looks good to me except for the last two sentences of the final 
paragraph:


"For example, when trying toread from the Stream after the 
BufferedReaderis closed, will throw an UncheckedIOException. Note 
thatmethod will return the Stream Stream even if this BufferedReader is 
closed, but the operation cause reading will throwUncheckedIOException."


How about:

"This method will return a Stream if invoked on a BufferedReader that is 
closed. Any operation on that stream requires reading from the 
BufferedReader after is it closed will cause an UncheckedIOException to 
be thrown".


Otherwise I think we are at the finish line.

-Alan

Re: Review Request: BigInteger patch for efficient multiplication and division (#4837946)

[Alan Bateman]

On 02/05/2013 02:34, Tim Buktu wrote:

:

Alan is working on an improved BigInteger.toString() that should be
dramatically faster for large numbers. What's the deadline for getting
this in?
Thanks,

Tim

Here's the latest milestone dates:

http://openjdk.java.net/projects/jdk8/milestones

Given the size of the patch then it would be great to get it in as early 
as possible. With the review effort then I assume Feature Complete is 
too tight although I know Brian Burkhalter is anxious to get it in as 
soon as possible. I can't comment on the BigInteger.toString work to 
know how big this is.


-Alan.

Re: (Preliminary) RFC 7038914: VM could throw uncaught OOME in ReferenceHandler thread

[Thomas Schatzl]

Hi,


On Tue, 2013-04-30 at 16:44 +0100, Alan Bateman wrote:
> On 30/04/2013 15:57, Thomas Schatzl wrote:
> > Hi all,
> >
> >the webrev at http://cr.openjdk.java.net/~tschatzl/7038914/webrev/
> > presents a first stab at the CR "7038914: VM could throw uncaught OOME
> > in ReferenceHandler thread".
> >
> > The problem is that under very heavy memory pressure, there is the
> > reference handler throws an exception with the message "Exception:
> > java.lang.OutOfMemoryError thrown from the UncaughtExceptionHandler in
> > thread "Reference Handler".
> >
> > The change improves handling of out-of-memory conditions in the
> > ReferenceHandler thread. Instead of crashing the thread, and then
> > disabling reference processing, it catches this exception and continues.
>
> It's surprising to heard that the Reference Handler thread failed with 
> OOME. I wouldn't expect anything in this code path to throw OOME, except 
> maybe in fast-path for sun.misc.Cleaner but that will abort the VM be it 
> fails. The enqueue method that you override in the test to provoke this 
> is package-private so it's unlikely that the test or whatever that 
> resulted in this bug report is doing that.

The test is just that: a somewhat artificial way to reproduce the
problem always.

I tried some of the example programs listed below thousands of times,
sometimes without any issue. The developer previously working on that
also had severe problems reproducing it.

> So I'm again this proposed change, rather I'm just trying to understand 
> how it happened. Is there instrumentation involved by any chance? It the 
> OOME something other than "java heap" or do we know?

No instrumentation I can see of, but a whole set of weak reference
related nightly UTE tests fail at different times (I would suspect
nightly testing does not do any instrumentation). Here is a list with
exactly these errors:

gc/gctests/ReferencesGC
gc/gctests/WeakReference/weak003
gc/gctests/SoftReference/soft005
gc/gctests/LargeObjects/large002
nsk/jdi/ObjectReference/referringObjects/referringObjects002
gc/gctests/PhantomReference/PhantomReferenceTest
gc/gctests/OneeFinalizerTest
heapdump/JMapHeap
gc/gctests/FinalizeTest01
nsk/sysdict/vm/stress/btree/btree007

Apart from these failures, the more serious problem seems to be that the
reference handler thread silently dies. Which means that weak reference
processing is effectively disabled after such an error.

A VM abort like for the Cleaner processing would be lot preferable than
the current situation too.

Thanks,
Thomas

Re: RFR : 8013712 : (XS) Add Objects.nonNull and Objects.isNull

[Jeff Hain]

Hello.


nonNull could be renamed into isNonNull,
else people might use it instead of requireNonNull,
especially if they are already used to a pre-Java 7
requireNonNull that would be called nonNull.
(example:
http://grokbase.com/p/openjdk/core-libs-dev/111tbha823/code-review-7012540-java-util-objects-nonnull-incorrectly-named
)


It would also be more consistent with isNull.



-Jeff

RFEs implementing JEP 170

[Neil Richards]

Hi Lance,
I've been trying to identify the Java bug ids for the RFEs which
implement JEP 170 (which, from what I can tell, should be in OpenJDK 8
since milestone 6 [1]).

Unlike most JEPs, the entry for 170 does not seem to hold references to
these ids (only to the affected JSRs).

Searching the mailing archives for "jep 170", "jsr 114" & "jsr 221" also
didn't offer any greater insight, though by searching for "jdbc 4.2", I
did manage to find reference to 8005080 [2] [3].

That conversation suggests there should be other RFEs concerned with
the remaining implementation of JEP 170, but I haven't been able to
determine which those are.

Could you please tell me which RFEs implements this JEP?
It also would be good to update the document for JEP 170 [4] with there
references, and perhaps update its status to "Completed" if it has,
indeed, been completely delivered into openjdk 8?

Thanks,
Neil

[1] http://openjdk.java.net/projects/jdk8/milestones
[2] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-January/013569.html
[3] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/d3da0d29d7cd
[4] http://openjdk.java.net/jeps/170

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFEs implementing JEP 170

[Neil Richards]

Oops, messed up my reply-to address. 
(I blame my newly upgraded system).

Hopefully this should be better.

Regards,
Neil

On Thu, 2013-05-02 at 12:02 +0100, Neil Richards wrote:
> Hi Lance,
> I've been trying to identify the Java bug ids for the RFEs which
> implement JEP 170 (which, from what I can tell, should be in OpenJDK 8
> since milestone 6 [1]).
> 
> Unlike most JEPs, the entry for 170 does not seem to hold references to
> these ids (only to the affected JSRs).
> 
> Searching the mailing archives for "jep 170", "jsr 114" & "jsr 221" also
> didn't offer any greater insight, though by searching for "jdbc 4.2", I
> did manage to find reference to 8005080 [2] [3].
> 
> That conversation suggests there should be other RFEs concerned with
> the remaining implementation of JEP 170, but I haven't been able to
> determine which those are.
> 
> Could you please tell me which RFEs implements this JEP?
> It also would be good to update the document for JEP 170 [4] with there
> references, and perhaps update its status to "Completed" if it has,
> indeed, been completely delivered into openjdk 8?
> 
> Thanks,
> Neil
> 
> [1] http://openjdk.java.net/projects/jdk8/milestones
> [2] 
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-January/013569.html
> [3] http://hg.openjdk.java.net/jdk8/jdk8/jdk/rev/d3da0d29d7cd
> [4] http://openjdk.java.net/jeps/170
> 

-- 
Unless stated above:
IBM email: neil_richards at uk.ibm.com
IBM United Kingdom Limited - Registered in England and Wales with number 741598.
Registered office: PO Box 41, North Harbour, Portsmouth, Hampshire PO6 3AU

Re: RFR: 8012665: CharSequence.chars, CharSequence.codePoints

[Ulf Zibis]

Thanks for your opinion, Paul.

Looking into the existing sources, violation of the 80 character limit seems widely accepted, so to 
me it seems reasonable to continue this style in appropriate cases.


But I rarely see good reasons to violate the 8-spaces indentation rule for 
continuation lines.

-Ulf


Am 26.04.2013 16:48, schrieb Paul Benedict:
Ulf, I have my opinions too on code style. However, the published guidelines for Java code is what 
Oracle/Sun set out for themselves. AFAIK, it is what's expected for JDK source.



On Fri, Apr 26, 2013 at 7:29 AM, Ulf Zibis mailto:ulf.zi...@cosoco.de>> wrote:

I think, sometimes it is better to violate those 2 rules because:
- modern wide-screens have much horizontal, but less vertical space, 
especially on labtops
- line break for only one/few word(s) looks ugly, disturbs read-flow
- it's no problem, if e.g. 1 of 50 lines must be scrolled a little 
horizontally, but it's a
big problem if I have to vertically scroll twice often, when too much lines are 
"wasted".
Comparing and understanding code then becomes a nightmare.

Referring to your example, on the other hand, continuation lines should be 
indented 8 rather
than 4 spaces to separate them from logical nesting. Especially your last 
line looks like less
nested than the three before, which IMHO is a clear mistake.

-Ulf


Am 25.04.2013 22:57, schrieb Paul Benedict:

Henry,

I believe the coding standards require curly braces for any if-statement
and for-loop.

Also the return statements exceed the 80 character limit. It would be 
nice
to have them formatted across several lines like the following because 
it's
difficult to read going straight across:

return StreamSupport.intStream(() ->
 Spliterators.spliterator(
 new CharIterator(),
 length(),
 Spliterator.ORDERED),
 Spliterator.SUBSIZED | Spliterator.SIZED | 
Spliterator.ORDERED);

Paul