Re: RFR: JDK-8152690: main thread does not have native thread name

[David Holmes]

Hi Yasumasa,

Thanks for persevering with this to get it into the current form. Sorry 
I haven't been able to do a detailed review until now.


On 19/04/2016 9:28 AM, Yasumasa Suenaga wrote:

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" >:
 >
 > hi Yasumasa,
 >
 > Nice work. I have 2 questions:
 >
 > 
 > File: java.c
 >
 > #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)”
after every single JNI call? In this example instead of NULL_CHECK,
should we be using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because
we cannot set native thread name only.
So I think that we do not need to leave from launcher process.


I agree we do not need to abort if an exception occurs (and in fact I 
don't think an exception is even possible from this code), but we should 
ensure any pending exception is cleared before any futher JNI calls 
might be made. Note that NULL_CHECK is already used extensively 
throughout the launcher code - so if this usage is wrong then it is all 
wrong! More on this code below ...


Other comments:

hotspot/src/share/vm/prims/jvm.cpp

Please add a comment to the method now that you removed the internal 
comment:


// Sets the native thread name for a JavaThread. If specifically
// requested JNI-attached threads can also have their native name set;
// otherwise we do not modify JNI-attached threads as it may interfere
// with the application that created them.

---

jdk/src/java.base/share/classes/java/lang/Thread.java

Please add the following comments:

+// Don't modify JNI-attached threads
 setNativeName(name, false);

+ // May be called directly via JNI or reflection (when permitted) to
+ // allow JNI-attached threads to set their native name
 private native void setNativeName(String name, boolean 
allowAttachedThread);


---

jd/src/java.base/share/native/libjli/java.c

328 #define LEAVE() \
329 SetNativeThreadName(env, "DestroyJavaVM"); \

I was going to suggest this be set later, but realized we have to be 
attached to do this and that happens inside DestroyJavaVM. :)


+ /* Set native thread name. */
+ SetNativeThreadName(env, "main");

The comment is redundant given the name of the method. That aside I'm 
not sure why you do this so late in the process, I would have done it 
immediately after here:


 386 if (!InitializeJVM(, , )) {
 387 JLI_ReportErrorMessage(JVM_ERROR1);
 388 exit(1);
 389 }
 +   SetNativeThreadName(env, "main");


+ /**
+  * Set native thread name as possible.
+  */

Other than the as->if change I'm unclear where the "possible" bit comes 
into play - why would it not be possible?


1705 NULL_CHECK(cls = FindBootStrapClass(env, "java/lang/Thread"));
1706 NULL_CHECK(currentThreadID = (*env)->GetStaticMethodID(env, cls,
1707  "currentThread", 
"()Ljava/lang/Thread;"));

1708 NULL_CHECK(currentThread = (*env)->CallStaticObjectMethod(env, cls,
1709 
currentThreadID));

1710 NULL_CHECK(setNativeNameID = (*env)->GetMethodID(env, cls,
1711 "setNativeName", 
"(Ljava/lang/String;Z)V"));

1712 NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
1713 (*env)->CallVoidMethod(env, currentThread, setNativeNameID,
1714nameString, JNI_TRUE);

As above NULL_CHECK is fine here, but we should check for and clear any 
pending exception after CallVoidMethod.


One thing I dislike about the current structure is that we have to go 
from char* to java.lang.String to call setNativeName which then calls 
JVM_SetNativeThreadName which converts the j.l.String back to a char* ! 
Overall I wonder about the affect on startup cost. But if there is an 
issue we can revisit this.


Thanks,
David
-



 > #2 Should the comment for “SetNativeThreadName” be “Set native thread
name if possible.” not "Set native thread name as possible.”?

Sorry for my bad English :-)

Thanks,

Yasumasa

 > cheers
 >
 > > On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga > wrote:
 > >
 > > Hi David,
 > >
 > > I uploaded new webrev:
 > >
 > > - hotspot:
 > > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
 > >
 > > - jdk:
 > > http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
 > >
 > >
 > >> it won't work unless you change the semantics of setName so I'm
not sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???
 > >
 > > I added a flag for setting native thread name to JNI-attached thread.
 > > This change can set native thread name if main thread changes to
JNI-attached thread.
 > >
 > >
 > > Thanks,
 > >
 > > Yasumasa
 > >
 > >
 > > On 2016/04/16 16:11, David Holmes wrote:
 > >> On 16/04/2016 3:27 PM, Yasumasa 

Re: RFR: JDK-8154498: fix to 8154403 results in failure of UserModuleTest.java on all platforms

[Sundararajan Athijegannathan]

+1

-Sundar

On 4/19/2016 8:28 AM, Xueming Shen wrote:
> Hi,
>
> Please help review the test case only change for JDK-8154498
>
> issue: https://bugs.openjdk.java.net/browse/JDK-8154498
> webrev: http://cr.openjdk.java.net/~sherman/8154498/webrev/
>
> A "empty" map need to be passed in as the parameter for
> FileSystems.newFileSystem(),
> instead of "null".
>
> Thanks,
> Sherman

Re: RFR: JDK-8154498: fix to 8154403 results in failure of UserModuleTest.java on all platforms

[joe darcy]

Looks fine Sherman; thanks,

-Joe

On 4/18/2016 7:58 PM, Xueming Shen wrote:

Hi,

Please help review the test case only change for JDK-8154498

issue: https://bugs.openjdk.java.net/browse/JDK-8154498
webrev: http://cr.openjdk.java.net/~sherman/8154498/webrev/

A "empty" map need to be passed in as the parameter for 
FileSystems.newFileSystem(),

instead of "null".

Thanks,
Sherman

RFR: JDK-8154498: fix to 8154403 results in failure of UserModuleTest.java on all platforms

[Xueming Shen]

Hi,

Please help review the test case only change for JDK-8154498

issue: https://bugs.openjdk.java.net/browse/JDK-8154498
webrev: http://cr.openjdk.java.net/~sherman/8154498/webrev/

A "empty" map need to be passed in as the parameter for 
FileSystems.newFileSystem(),

instead of "null".

Thanks,
Sherman

Multi-Release JAR runtime support

[Hervé BOUTEMY]

Hi OpenJDK Core Developers,

Today, to prepare a conference talk on Java 9 (DevoxxFR, on wednesday 20th), I 
tested once again Maven proposed layout to produce Multi-Release JARs:
https://github.com/hboutemy/maven-jep238

And I added a little script (. run-demo.sh) to run the build then execute the 
resulting jar with JRE 7, 8 and 9.

As expected, JRE 7 and 8 gave the same result, since my JRE 8 isn't patched to 
support MR JARs (if there is a simple way to add support, don't hesitate to 
give me a pointer. Or if there is any standard Java 8 build that should 
support it).

But the unexpected part is that JRE 9, either classical or jigsaw, don't take 
advantage of the MR JAR: is this really expected, or did I do something wrong?

Regards,

Hervé

Here is the interesting part of the output of the script:

Archive:  multirelease/target/multirelease-0.8-SNAPSHOT.jar
testing: META-INF/MANIFEST.MF OK
testing: base/Base.class  OK
testing: mr/A.class   OK
testing: META-INF/versions/8/mr/A.class   OK
testing: META-INF/versions/9/mr/A.class   OK
No errors detected in compressed data of multirelease/target/multirelease-0.8-
SNAPSHOT.jar.
[...]
# Run Multi Release JAR with JRE 7
$ java -classpath multirelease/target/multirelease-0.8-SNAPSHOT.jar base.Base
1.7.0_71-b14
BASE

# Run Multi Release JAR with JRE 8
$ java -classpath multirelease/target/multirelease-0.8-SNAPSHOT.jar base.Base
1.8.0_25-b17
BASE

# Run Multi Release JAR with JRE 9 classic
$ java -classpath multirelease/target/multirelease-0.8-SNAPSHOT.jar base.Base
9-ea+114
BASE

# Run Multi Release JAR with JRE 9 Jigsaw
$ java -classpath multirelease/target/multirelease-0.8-SNAPSHOT.jar base.Base
9-ea+114-2016-04-15-162029.javare.4859.nc
BASE

Re: RFR: 8154470: defines.h confused about PROGNAME and JAVA_ARGS

[Kumar Srinivasan]

Hi Martin,

Tested with jprt, seems to be ok, the changes are ok with me. Unless
Alan has some reservations.

Thanks
Kumar


Hi Kumar and Alan,

I'd like you to do a code review.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/PROGNAME/
https://bugs.openjdk.java.net/browse/JDK-8154470

Sorry, I got carried away hacking on the tests.  Some changes could be
split out.
Aside from fixing the logic error in defines.h,

printing out tr.status is useless, because it just invokes
Object.toString on PrintWriter

-System.out.println(tr.status);
+System.out.println(tr);

It's complete coincidence this extends Kumar's recent test testJLDEnvWithTool.

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

Hi Gerard,

2016/04/19 3:14 "Gerard Ziemski" :
>
> hi Yasumasa,
>
> Nice work. I have 2 questions:
>
> 
> File: java.c
>
> #1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after
every single JNI call? In this example instead of NULL_CHECK, should we be
using CHECK_EXCEPTION_NULL_LEAVE macro?

It is not critical if we encounter error at JNI function call  because we
cannot set native thread name only.
So I think that we do not need to leave from launcher process.

> #2 Should the comment for “SetNativeThreadName” be “Set native thread
name if possible.” not "Set native thread name as possible.”?

Sorry for my bad English :-)

Thanks,

Yasumasa

> cheers
>
> > On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga 
wrote:
> >
> > Hi David,
> >
> > I uploaded new webrev:
> >
> > - hotspot:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> >
> > - jdk:
> >http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> >
> >
> >> it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java code
will call it . ???
> >
> > I added a flag for setting native thread name to JNI-attached thread.
> > This change can set native thread name if main thread changes to
JNI-attached thread.
> >
> >
> > Thanks,
> >
> > Yasumasa
> >
> >
> > On 2016/04/16 16:11, David Holmes wrote:
> >> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
> >>> Hi David,
> >>>
>  That change in behaviour may be a problem, it could be considered a
>  regression that setName stops setting the native thread main, even
>  though we never really intended it to work in the first place. :(
Such
>  a change needs to go through CCC.
> >>>
> >>> I understood.
> >>> Can I send CCC request?
> >>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
> >>
> >> Sorry you can't file a CCC request, you would need a sponsor for that.
But at this stage I don't think I agree with the proposed change because of
the change in behaviour - there's no way to restore the "broken" behaviour.
> >>
> >>> I want to continue to discuss about it on JDK-8154331 [1].
> >>
> >> Okay we can do that.
> >>
> >>>
>  Further, we expect the launcher to use the supported JNI interface
(as
>  other processes would), not the internal JVM interface that exists
for
>  the JDK sources to communicate with the JVM.
> >>>
> >>> I think that we do not use JVM interface if we add new method in
> >>> LauncherHelper as below:
> >>> 
> >>> diff -r f02139a1ac84
> >>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Wed Apr 13 14:19:30 2016 +
> >>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
> >>> Sat Apr 16 11:25:53 2016 +0900
> >>> @@ -960,4 +960,8 @@
> >>>  else
> >>>  return md.toNameAndVersion() + " (" + loc + ")";
> >>>  }
> >>> +
> >>> +static void setNativeThreadName(String name) {
> >>> +Thread.currentThread().setName(name);
> >>> +}
> >>
> >> You could also make that call via JNI directly, so not sure the helper
adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take advantage of
an arg taking JVM_SetNativThreadName you would need to call it directly as
no Java code will call it . ???
> >>
> >> David
> >> -
> >>
> >>>  }
> >>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
> >>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
> >>> 2016 +
> >>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
> >>> 2016 +0900
> >>> @@ -125,6 +125,7 @@
> >>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
> >>>  static void ShowSettings(JNIEnv* env, char *optString);
> >>>  static void ListModules(JNIEnv* env, char *optString);
> >>> +static void SetNativeThreadName(JNIEnv* env, char *name);
> >>>
> >>>  static void SetPaths(int argc, char **argv);
> >>>
> >>> @@ -325,6 +326,7 @@
> >>>   * mainThread.isAlive() to work as expected.
> >>>   */
> >>>  #define LEAVE() \
> >>> +SetNativeThreadName(env, "DestroyJavaVM"); \
> >>>  do { \
> >>>  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
> >>>  JLI_ReportErrorMessage(JVM_ERROR2); \
> >>> @@ -488,6 +490,9 @@
> >>>  mainArgs = CreateApplicationArgs(env, argv, argc);
> >>>  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
> >>>
> >>> +/* Set native thread name. */
> >>> +SetNativeThreadName(env, "main");
> >>> +
> >>>  /* Invoke main method. */
> >>>  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
> >>>
> >>> @@ -1686,6 +1691,22 @@
> >>>   joptString);
> >>>  }
> >>>
> >>> +/**
> >>> + * Set 

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Oops on my part, Gerard is right. We need to make SetNativeThreadName exit,
if there is an error, otherwise it would enter the VM with an exception 
at the

call site.

So I think CHECK_EXCEPTION_NULL_LEAVE is the right thing to do, or we
need to have a check and exit at the call site.

Kumar


hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after every 
single JNI call? In this example instead of NULL_CHECK, should we be using 
CHECK_EXCEPTION_NULL_LEAVE macro?

#2 Should the comment for “SetNativeThreadName” be “Set native thread name if 
possible.” not "Set native thread name as possible.”?


cheers


On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga  wrote:

Hi David,

I uploaded new webrev:

- hotspot:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

- jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/



it won't work unless you change the semantics of setName so I'm not sure what 
you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to JNI-attached 
thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,


That change in behaviour may be a problem, it could be considered a
regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :( Such
a change needs to go through CCC.

I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)

Sorry you can't file a CCC request, you would need a sponsor for that. But at this stage 
I don't think I agree with the proposed change because of the change in behaviour - 
there's no way to restore the "broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].

Okay we can do that.


Further, we expect the launcher to use the supported JNI interface (as
other processes would), not the internal JVM interface that exists for
the JDK sources to communicate with the JVM.

I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}

You could also make that call via JNI directly, so not sure the helper adds 
much here. But it won't work unless you change the semantics of setName so I'm 
not sure what you were thinking here. To take advantage of an arg taking 
JVM_SetNativThreadName you would need to call it directly as no Java code will 
call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);

@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
+"setNativeThreadName", "(Ljava/lang/String;)V"));
+NULL_CHECK(nameString = (*env)->NewStringUTF(env, name));
+(*env)->CallStaticVoidMethod(env, cls, setNativeThreadNameID,
nameString);
+}
+
  /*
   * Prints default usage or 

Re: JDK 9 pre-review of JDK-6850612: Deprecate Class.newInstance since it violates the checked exception language contract

[Stuart Marks]

On 4/17/16 10:31 AM, joe darcy wrote:

With talk of deprecation in the air [1], I thought it would be a fine time to


"In the Spring a young man's fancy lightly turns to thoughts of deprecation."
-- apologies to Tennyson


examine one of the bugs on my list

 JDK-6850612: Deprecate Class.newInstance since it violates the checked
exception language contract

As the title of the bug implies, The Class.newInstance method knowingly violates
the checking exception contract. This has long been documented in its
specification: [...]

Comments on the bug have suggested that besides deprecating the method, a new
method on Class could be introduced, newInstanceWithProperExceptionBehavior,
that had the same signature but wrapped exceptions thrown by the constructor
call in the same way Constructor.newInstance does.


Deprecating Class.newInstance() seems reasonable to me, for the reasons you've 
stated.


It's not clear to me that a replacement method adds much value. I believe it's 
possible to replace a call


clazz.newInstance()  // 1

with the call

clazz.getConstructor().newInstance()  // 2

which is only a bit longer. Both snippets are declared to throw 
InstantiationException and IllegalAccessException. But snippet 2 also is 
declared to throw InvocationTargetException and NoSuchMethodException.


This would seem to be an inconvenience, but *all* of these exceptions are 
subtypes of ReflectiveOperationException. It seems pretty likely to me that most 
code handles these different exception types the same way. So it's fairly low 
cost to replace snippet 1 with snippet 2, and to adjust the exception handling 
to deal with ReflectiveOperationException. Thus I don't see much value in adding 
a new method such as Class.newInstanceWithProperExceptionBehavior().


s'marks

Re: RFR(m): 8145468u1 deprecations for java.lang

[Steven Schlansker]

> On Apr 18, 2016, at 3:30 PM, Stuart Marks  wrote:
> 
> On 4/17/16 7:06 AM, Dave Brosius wrote:
>> Along these lines, is there a reason not to deprecate the
>> 
>> String(String s)
>> 
>> constructor? Now that substring doesn't glom off the original string, i see 
>> no
>> reason for this constructor.
> 
> It's a fair point. But I think that historically there's been much greater 
> awareness of Strings' identity than that of boxed primitives.
> 
> At issue is string interning. When you compile a Java program, a string 
> literal like "foo" is unavoidably interned. This is wired deeply into the 
> language, compiler, and JVM, and has been so since 1.0.
> 
> With boxed primitives, there is autoboxing, but it's only been around since 
> Java 5. ("Only" 11 years.) There is a cache, and although this is mandated by 
> the JLS, it's actually maintained only by the library.
> 
> The notion of identity of strings seems stronger, thus there's a greater need 
> for new String(String) if you want to guarantee a string has a unique 
> identity.
> 
> It also seems much more likely for us to be able to turn boxed primitives 
> into values than to turn strings into values. (One issue is that strings can 
> be of all different sizes, whereas all instances/values of a boxed primitive 
> are of the same size.) Thus there appears to be a greater benefit to 
> migrating code away from the box constructors than from the String(String) 
> constructor.
> 
> This is probably something that should be revisited at some point, though. 
> There are probably more misuses of String(String) out there than there are 
> legitimate uses.

As an example of a useful use of the new String(String) constructor, I once 
wrote (pseudo) code:

WeakHashMap memoTable = new WeakHashMap<>();
public String memoTransform(String key) {
String value = memoTable.get(key);
if (value != null) return value;
value = transform(key);  // Might be identity function and return 'key'
memoTable.put(key, (key == value) ? new String(value) : value);
}

If you aren't careful to copy your value in this sort of situation, you can end 
with
never-collectible entries and therefore memory leaks in your weak hash table.

If substring were specified to always return unique objects, that could work as 
well --
but in my opinion the copy constructor is clearer when object identity is 
important,
and it looks like String sensibly avoids unnecessary copying:

String.java:1933  return (beginIndex == 0) ? this : new String(value, 
beginIndex, subLen);

Re: JDK 9 proposal: allocating ByteBuffers on heterogeneous memory

[mark . reinhold]

2016/4/8 1:41:47 -0700, paul.san...@oracle.com:
> On 8 Apr 2016, at 00:03, Dohrmann, Steve  wrote:
>> Just to clarify, it is incidental that the proposed Memory interface
>> has only one method.  We see the value of the interface as
>> nominative; a new type that can be passed around to abstract various
>> sources of ByteBuffer memory.
> 
> I suspected as much, but would prefer that we gain more experience on
> what this interface should be, and how it intersects with other
> efforts, rather than introducing a skeletal version now.

I agree with Paul on this.  It seems premature to introduce some kind of
grand "Memory" abstraction.  Without actual experience with a broad set
of use cases we're almost certain to get it wrong.  If what you want to
do now can be expressed via IntFunction then that seems a
good basis for further experimentation.

- Mark

Re: RFR(m): 8145468u1 deprecations for java.lang

[Stuart Marks]

On 4/17/16 7:06 AM, Dave Brosius wrote:

Along these lines, is there a reason not to deprecate the

String(String s)

constructor? Now that substring doesn't glom off the original string, i see no
reason for this constructor.


It's a fair point. But I think that historically there's been much greater 
awareness of Strings' identity than that of boxed primitives.


At issue is string interning. When you compile a Java program, a string literal 
like "foo" is unavoidably interned. This is wired deeply into the language, 
compiler, and JVM, and has been so since 1.0.


With boxed primitives, there is autoboxing, but it's only been around since Java 
5. ("Only" 11 years.) There is a cache, and although this is mandated by the 
JLS, it's actually maintained only by the library.


The notion of identity of strings seems stronger, thus there's a greater need 
for new String(String) if you want to guarantee a string has a unique identity.


It also seems much more likely for us to be able to turn boxed primitives into 
values than to turn strings into values. (One issue is that strings can be of 
all different sizes, whereas all instances/values of a boxed primitive are of 
the same size.) Thus there appears to be a greater benefit to migrating code 
away from the box constructors than from the String(String) constructor.


This is probably something that should be revisited at some point, though. There 
are probably more misuses of String(String) out there than there are legitimate 
uses.


s'marks

Re: RFR(m): 8145468 deprecations for java.lang

[Stuart Marks]

On 4/17/16 4:05 PM, David Holmes wrote:

On 14/04/2016 11:50 AM, Stuart Marks wrote:

The methods being deprecated with forRemoval=true are listed below. All
of these methods have already been deprecated. They are all ill-defined,
or they don't work, or they don't do anything useful.


Surprised Thread.suspend/resume are not marked for removal given they are
effectively unusable.


There's some rationale here. These methods (along with the no-arg Thread.stop()) 
have been deprecated for many years, since JDK 1.2 according to my research. 
Isn't it time to remove them?


Well, the fact is that they do perform their intended functions: suspend() and 
resume() really do suspend and resume the target thread, and stop() really 
causes the target thread to throw ThreadDeath. No doubt using these is unsafe. 
Using them involves either some risk of deadlock or data corruption, or using 
them in such a restricted fashion that the risk is mitigated.


In fact they do get a fair amount of use. It's not difficult to find a bunch of 
usages of them on grepcode.com. For example, Hadoop among other projects uses 
suspend/resume to test timeout handling code. I'm aware of frameworks that use 
stop() on runaway plugins in an attempt to initiate error recovery and an 
orderly shutdown. Clearly such usage isn't guaranteed to work, but it *might* 
work, and if so, it seems preferable to forcible termination of the entire JVM. 
That risk tradeoff is for the applications and frameworks to make.


With this in mind it's not obvious to me that suspend/resume/stop ought to be 
deprecated for removal.


This is certainly a worthwhile discussion to have. If someone has a pressing 
case to get rid of these, or perhaps just suspend/resume, by all means make it. 
I do think this should be considered separately from JDK-8145468, though.


s'marks

Re: JDK 9 RFR of 8154183: (spec) {Data, }InputStream.read(byte[], int, int) - spec of offset argument confusing

[Roger Riggs]

HI Brian,

Looks good,

Thanks for making them consistent,  Roger


On 4/18/2016 4:53 PM, Brian Burkhalter wrote:

A new patch generated using the latest version of webrev is here:

http://cr.openjdk.java.net/~bpb/8154183/webrev.01/ 



I have addressed the various editorial comments except with respect to 
capitalizing the first word after the exception name and ending the 
@throws description with a period: I have made the new @throws clauses 
consistent with other such clauses already present in the same method. 
In some case this means starting with lower case, in others upper 
case; likewise in some cases there are periods at the end, in other 
cases not.


Thanks,

Brian

On Apr 18, 2016, at 12:45 PM, Roger Riggs > wrote:



- In the new @throws NullPointerException and IndexOutOfBoundExceptions,
  the first word after the exception should not be capitalized.
  For example  "if" instead of "If"  makes it consistent with the 
existing doc


ObjectInputStream.java:  line 1012+
- usually the @throws description is not a complete sentence and does 
not deserve a "."

 per-file-consistency...
 for example,
+ * @throws  NullPointerException If {@code buf} is {@code null}*.

RE: RFR: JDK-8153781 Issue in XMLScanner: EXPECTED_SQUARE_BRACKET_TO_CLOSE_INTERNAL_SUBSET when skipping large DOCTYPE section with CRLF at wrong place

[Langer, Christoph]

Hi Joe,

here is the updated webrev where I incorporated your suggestions:

http://cr.openjdk.java.net/~clanger/webrevs/8153781.1/

I also added a testcase. As for the message " DoctypedeclNotClosed ": I did it 
in several languages but there are some languages where I don't have the 
knowledge to create a localized string. Is there some process for this or would 
we just be ok with reverting to the English text for that particular message?

Thanks in advance for your review :)

All the best
Christoph

> -Original Message-
> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> Sent: Dienstag, 12. April 2016 23:28
> To: Langer, Christoph 
> Cc: core-libs-dev@openjdk.java.net
> Subject: Re: RFR: JDK-8153781 Issue in XMLScanner:
> EXPECTED_SQUARE_BRACKET_TO_CLOSE_INTERNAL_SUBSET when skipping
> large DOCTYPE section with CRLF at wrong place
>
>
> On 4/12/2016 11:50 AM, Langer, Christoph wrote:
> > Hi Joe,
> >
> > thanks as always for your suggestions and I'll try to work it in. It will 
> > probably
> take me a little while as I'm currently busy with another thing. I'll update 
> my
> webrev eventually and add a testcase, too.
>
> Ok.
> >
> > But one question: I understand that the fix in skipDTD will be sufficient 
> > to fix
> the issue. Nevetheless, can you explain me why the change in scanData is not
> beneficial or could even cause issues? There are several places in scanData
> when further loads are done. But only at this point where there's exactly one
> character after CRLF at the end of the buffer the method just returns without
> further load. If it wouldn't leave here it seems to me as if it would continue
> correctly and load more data. That would probably also be better in the sense
> of performance I guess??
>
> It's there to handle the situation where a surrogate pair got split in
> between buffers.
>
> -Joe
>
> >
> > Thanks
> > Christoph
> >
> >> -Original Message-
> >> From: huizhe wang [mailto:huizhe.w...@oracle.com]
> >> Sent: Dienstag, 12. April 2016 19:54
> >> To: Langer, Christoph 
> >> Cc: core-libs-dev@openjdk.java.net
> >> Subject: Re: RFR: JDK-8153781 Issue in XMLScanner:
> >> EXPECTED_SQUARE_BRACKET_TO_CLOSE_INTERNAL_SUBSET when
> skipping
> >> large DOCTYPE section with CRLF at wrong place
> >>
> >> Also, EXPECTED_SQUARE_BRACKET_TO_CLOSE_INTERNAL_SUBSET was a
> >> wrong msg
> >> id. It would be good to change that to DoctypedeclNotClosed and add a
> >> message to XMLMessages.properties right before
> DoctypedeclUnterminated,
> >> sth. like the following:
> >>
> >> DoctypedeclNotClosed = The document type declaration for root element
> >> type \"{0}\" must be closed with '']''.
> >>
> >> Thanks,
> >> Joe
> >>
> >> On 4/11/2016 5:49 PM, huizhe wang wrote:
> >>> On 4/7/2016 1:45 PM, Langer, Christoph wrote:
>  Hi,
> 
> 
> 
>  I've run into a peculiar issue with Xerces.
> 
> 
> 
>  The problem is happening when a DTD shall be skipped, the DTD is
>  larger than the buffer of the current entity and a CRLF sequence
>  occurs just one char before the buffer end.
> 
> 
> 
>  The reason is that method skipDTD of class XMLDTDScannerImpl (about
>  line 389) calls XMLEntityScanner.scanData() to scan for the next
>  occurrence of ']'. The scanData method might return true which
>  indicates that the delimiter ']' was not found yet and more data is
>  to scan. Other users of scanData would handle this and call this
>  method in a loop until it returns false or some other condition
>  happens. So I've fixed that place like at the other callers of scanData.
> >>> This part of the change looks good.
> 
> 
>  Nevertheless, the scanData method would usually load more data when
>  it is at the end of the buffer. But in the special case when CRLF is
>  found at the end of buffer - 1, scanData would just return true. So I
>  also removed that check at line 1374 in XMLEntityScanner. Do you see
>  any problem with that? Is there any reason behind it which I'm
>  overseeing?
> >>> No need to remove this after the above change. The parser needs to
> >>> retain what's in the xml, e.g., not removing new lines.
>  Furthermore I took the chance for further little cleanups. I've added
>  the new copyright header to the files... is that the correct one?
> >>> Yes, that's the right license header. However,
> 
>  I also aligned the calls to invokeListeners(position) in
>  XMLEntityScanner to always pass the actual position from which the
>  load is started. Do you think this is correct?
> >>> Yes.
> 
> 
>  Here is the bug:
> 
>  https://bugs.openjdk.java.net/browse/JDK-8153781
> 
> 
> 
>  Here is the webrev:
> 
>  http://cr.openjdk.java.net/~clanger/webrevs/8153781.0/
> 
> 
> 
>  Please give me some comments before I finalize my change including a
> 

Re: JDK 9 RFR of 8154183: (spec) {Data, }InputStream.read(byte[], int, int) - spec of offset argument confusing

[Brian Burkhalter]

A new patch generated using the latest version of webrev is here:

http://cr.openjdk.java.net/~bpb/8154183/webrev.01/

I have addressed the various editorial comments except with respect to 
capitalizing the first word after the exception name and ending the @throws 
description with a period: I have made the new @throws clauses consistent with 
other such clauses already present in the same method. In some case this means 
starting with lower case, in others upper case; likewise in some cases there 
are periods at the end, in other cases not.

Thanks,

Brian

On Apr 18, 2016, at 12:45 PM, Roger Riggs  wrote:

> - In the new @throws NullPointerException and IndexOutOfBoundExceptions,
>   the first word after the exception should not be capitalized.
>   For example  "if" instead of "If"  makes it consistent with the existing doc
> 
> ObjectInputStream.java:  line 1012+
> - usually the @throws description is not a complete sentence and does not 
> deserve a "."
>  per-file-consistency...
>  for example,
> + * @throws  NullPointerException If {@code buf} is {@code null}*.

RFR [9] 8148863: Remove sun.misc.ManagedLocalsThread from corba

[Chris Hegarty]

This change updates the code in the cobra module to use the new
Thread constructor that suppresses inheritable-thread-local initial
values.

http://cr.openjdk.java.net/~chegar/8148863/
https://bugs.openjdk.java.net/browse/JDK-8148863

I’m open to suggestions for better names for the Threads.

-Chris.

Re: JDK 9 RFR of 8154183: (spec) {Data, }InputStream.read(byte[], int, int) - spec of offset argument confusing

[Brian Burkhalter]

Hi Roger,

On Apr 18, 2016, at 12:45 PM, Roger Riggs  wrote:

> Editorial cleanup.
> - In the new @throws NullPointerException and IndexOutOfBoundExceptions,
>   the first word after the exception should not be capitalized.
>   For example  "if" instead of "If"  makes it consistent with the existing doc

I noticed that but it was inconsistent throughout the files. Will Change.

> DataInputStream:
> - "the start offset in*to* the"  ;  "in" preferred over "into" consistent 
> with previous descriptions
> 
> ObjectInputStream.java:  line 1012+
> - usually the @throws description is not a complete sentence and does not 
> deserve a "."
>  per-file-consistency...
>  for example,
> + * @throws  NullPointerException If {@code buf} is {@code null}*.
> 
> *RandomAccessFile: 433+
> 
> - keep the alignment of the @param lines
> - @throws NPE; remvoe the training “."

Will update those also.

> p.s.  There is a new version of webrev that generates convenient next and 
> prev file links.

I noticed that in, I believe, Claes’s webrevs but had not updated mine yet; 
thanks for mentioning: it is convenient.

Thanks,

Brian

Re: JDK 9 RFR of 8154183: (spec) {Data, }InputStream.read(byte[], int, int) - spec of offset argument confusing

[Roger Riggs]

Hi Brian,

Editorial cleanup.
 - In the new @throws NullPointerException and IndexOutOfBoundExceptions,
   the first word after the exception should not be capitalized.
   For example  "if" instead of "If"  makes it consistent with the 
existing doc



DataInputStream:
 - "the start offset in*to* the"  ;  "in" preferred over "into" 
consistent with previous descriptions


ObjectInputStream.java:  line 1012+
 - usually the @throws description is not a complete sentence and does 
not deserve a "."

  per-file-consistency...
  for example,
+ * @throws  NullPointerException If {@code buf} is {@code null}*.

*RandomAccessFile: 433+

 - keep the alignment of the @param lines
 - @throws NPE; remvoe the training "."

Roger

p.s.  There is a new version of webrev that generates convenient next 
and prev file links.



On 4/18/2016 2:37 PM, Brian Burkhalter wrote:

The patch has been updated in place to replace var with {@code 
var}, @exception with @throws, and align the text where needed only for the methods in 
question.

Brian

On Apr 15, 2016, at 3:46 PM, Brian Burkhalter  
wrote:


Re-posted with correct subject line.

On Apr 15, 2016, at 3:35 PM, Brian Burkhalter  
wrote:


Please review at your convenience.

Issue:  https://bugs.openjdk.java.net/browse/JDK-8154183
Patch:  http://cr.openjdk.java.net/~bpb/8154183/webrev.00/

Summary:
1) Reinstate the ObjectInputStream part of the 
fixforhttps://bugs.openjdk.java.net/browse/JDK-4150728 which was inadvertently 
reverted in a subsequent merge.
2) Apply the same clarifications and addition of missing exception/throws tags 
to both variants of readFully().

Re: JDK 9 RFR of 8154183: (spec) {Data, }InputStream.read(byte[], int, int) - spec of offset argument confusing

[Brian Burkhalter]

The patch has been updated in place to replace var with {@code 
var}, @exception with @throws, and align the text where needed only for the 
methods in question.

Brian

On Apr 15, 2016, at 3:46 PM, Brian Burkhalter  
wrote:

> Re-posted with correct subject line.
> 
> On Apr 15, 2016, at 3:35 PM, Brian Burkhalter  
> wrote:
> 
>> Please review at your convenience.
>> 
>> Issue:   https://bugs.openjdk.java.net/browse/JDK-8154183
>> Patch:   http://cr.openjdk.java.net/~bpb/8154183/webrev.00/
>> 
>> Summary:
>> 1) Reinstate the ObjectInputStream part of the 
>> fixforhttps://bugs.openjdk.java.net/browse/JDK-4150728 which was 
>> inadvertently reverted in a subsequent merge.
>> 2) Apply the same clarifications and addition of missing exception/throws 
>> tags to both variants of readFully().

Re: RFR: JDK-8152690: main thread does not have native thread name

[Gerard Ziemski]

hi Yasumasa,

Nice work. I have 2 questions:


File: java.c

#1 Shouldn’t we be checking for “(*env)->ExceptionOccurred(env)” after every 
single JNI call? In this example instead of NULL_CHECK, should we be using 
CHECK_EXCEPTION_NULL_LEAVE macro?

#2 Should the comment for “SetNativeThreadName” be “Set native thread name if 
possible.” not "Set native thread name as possible.”?


cheers

> On Apr 16, 2016, at 4:29 AM, Yasumasa Suenaga  wrote:
> 
> Hi David,
> 
> I uploaded new webrev:
> 
> - hotspot:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
> 
> - jdk:
>http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/
> 
> 
>> it won't work unless you change the semantics of setName so I'm not sure 
>> what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
> 
> I added a flag for setting native thread name to JNI-attached thread.
> This change can set native thread name if main thread changes to JNI-attached 
> thread.
> 
> 
> Thanks,
> 
> Yasumasa
> 
> 
> On 2016/04/16 16:11, David Holmes wrote:
>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>> Hi David,
>>> 
 That change in behaviour may be a problem, it could be considered a
 regression that setName stops setting the native thread main, even
 though we never really intended it to work in the first place. :( Such
 a change needs to go through CCC.
>>> 
>>> I understood.
>>> Can I send CCC request?
>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>> 
>> Sorry you can't file a CCC request, you would need a sponsor for that. But 
>> at this stage I don't think I agree with the proposed change because of the 
>> change in behaviour - there's no way to restore the "broken" behaviour.
>> 
>>> I want to continue to discuss about it on JDK-8154331 [1].
>> 
>> Okay we can do that.
>> 
>>> 
 Further, we expect the launcher to use the supported JNI interface (as
 other processes would), not the internal JVM interface that exists for
 the JDK sources to communicate with the JVM.
>>> 
>>> I think that we do not use JVM interface if we add new method in
>>> LauncherHelper as below:
>>> 
>>> diff -r f02139a1ac84
>>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Wed Apr 13 14:19:30 2016 +
>>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Sat Apr 16 11:25:53 2016 +0900
>>> @@ -960,4 +960,8 @@
>>>  else
>>>  return md.toNameAndVersion() + " (" + loc + ")";
>>>  }
>>> +
>>> +static void setNativeThreadName(String name) {
>>> +Thread.currentThread().setName(name);
>>> +}
>> 
>> You could also make that call via JNI directly, so not sure the helper adds 
>> much here. But it won't work unless you change the semantics of setName so 
>> I'm not sure what you were thinking here. To take advantage of an arg taking 
>> JVM_SetNativThreadName you would need to call it directly as no Java code 
>> will call it . ???
>> 
>> David
>> -
>> 
>>>  }
>>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
>>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13 14:19:30
>>> 2016 +
>>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16 11:25:53
>>> 2016 +0900
>>> @@ -125,6 +125,7 @@
>>>  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
>>>  static void ShowSettings(JNIEnv* env, char *optString);
>>>  static void ListModules(JNIEnv* env, char *optString);
>>> +static void SetNativeThreadName(JNIEnv* env, char *name);
>>> 
>>>  static void SetPaths(int argc, char **argv);
>>> 
>>> @@ -325,6 +326,7 @@
>>>   * mainThread.isAlive() to work as expected.
>>>   */
>>>  #define LEAVE() \
>>> +SetNativeThreadName(env, "DestroyJavaVM"); \
>>>  do { \
>>>  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
>>>  JLI_ReportErrorMessage(JVM_ERROR2); \
>>> @@ -488,6 +490,9 @@
>>>  mainArgs = CreateApplicationArgs(env, argv, argc);
>>>  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);
>>> 
>>> +/* Set native thread name. */
>>> +SetNativeThreadName(env, "main");
>>> +
>>>  /* Invoke main method. */
>>>  (*env)->CallStaticVoidMethod(env, mainClass, mainID, mainArgs);
>>> 
>>> @@ -1686,6 +1691,22 @@
>>>   joptString);
>>>  }
>>> 
>>> +/**
>>> + * Set native thread name as possible.
>>> + */
>>> +static void
>>> +SetNativeThreadName(JNIEnv *env, char *name)
>>> +{
>>> +jmethodID setNativeThreadNameID;
>>> +jstring nameString;
>>> +jclass cls = GetLauncherHelperClass(env);
>>> +NULL_CHECK(cls);
>>> +NULL_CHECK(setNativeThreadNameID = (*env)->GetStaticMethodID(env, cls,
>>> +"setNativeThreadName", "(Ljava/lang/String;)V"));
>>> +NULL_CHECK(nameString = (*env)->NewStringUTF(env, 

Re: RFR: JDK-815440: JRT filesystem loaded by JDK8 with URLClassLoader is not closable since JDK-8147460

[Alan Bateman]

On 18/04/2016 18:14, Xueming Shen wrote:

Hi,

Please help review the change for JDK-8154403

issue: https://bugs.openjdk.java.net/browse/JDK-8154403
webrev: http://cr.openjdk.java.net/~sherman/8154403/webrev/

The direct trigger of the failure is that the test case is passing in 
a null as parameter
"env" to create a new jrt filesystem. The nio spec specifies that it 
will trigger NPE. The
existing implementation does not check the "null" and passes it 
directly into
JrtFileSystem's constructor, in which the "null" is interpreted as "a 
not closable jrt file

system" (A implementation detail).

The fix is to add the "null check" in the provider and pass in "empty 
map" in the test case.


Alan, the original jrtfs implementation throws UOE in 
JrtFileSystemProvider.getTheFileSystem(),
is it better to just "silently" ignore the close request for the 
"not-closable" jrt? I'm file with

either way though.

L107 checks if env is null so that is not needed. Otherwise looks okay.

If someone is attempting to close the file system returned by 
FileSystem.getFileSystem(URI.create("jrt:/")) then something is wrong. I 
may have suggested to Sundar that this throw UOE because this method is 
specified to throw this for the default file system. It should be okay 
to keep that behavior.


-Alan

RFR: 8154470: defines.h confused about PROGNAME and JAVA_ARGS

[Martin Buchholz]

Hi Kumar and Alan,

I'd like you to do a code review.

http://cr.openjdk.java.net/~martin/webrevs/openjdk9/PROGNAME/
https://bugs.openjdk.java.net/browse/JDK-8154470

Sorry, I got carried away hacking on the tests.  Some changes could be
split out.
Aside from fixing the logic error in defines.h,

printing out tr.status is useless, because it just invokes
Object.toString on PrintWriter

-System.out.println(tr.status);
+System.out.println(tr);

It's complete coincidence this extends Kumar's recent test testJLDEnvWithTool.

Re: RFR:JDK-8154050:java.time.format.DateTimeFormatter can't parse localized zone-offset

[nadeesh tv]

Hi Roger/Stephen,

On 4/18/2016 2:40 AM, Stephen Colebourne wrote:

The LDML spec indicates that the "GMT" string should be localized:
http://unicode.org/repos/cldr/trunk/specs/ldml/tr35-dates.html#Using_Time_Zone_Names
The text of appendLocalizedOffset() is written with the intention of
the output being localized (otherwise, what is the point of the
method!)

I assume this was something that got missed when implementing
appendLocalizedOffset(). It may require additional localized data from
the LDML data files.

If OK, I will create a new enhancement request  for this in JBS

Regards,
Nadeesh


This webrev looks fine.
Stephen



On 13 April 2016 at 16:56, Roger Riggs  wrote:

Hi Nadeesh,

The bugfix looks fine.

The TODO comment on the "GMT" raises the question (as a separate issue)
about implementing the TODO or removing the TODO comment.

I'm not sure where the localized string for "GMT" would come from but it
might be a useful improvement
unless it was judged to a compatibility issue.

Roger




On 4/13/2016 10:19 AM, nadeesh tv wrote:

HI all,

Bug Id -  https://bugs.openjdk.java.net/browse/JDK-8154050

Issue - java.time.format.DateTimeFormatter can't parse localized zone-offset

Solution - Corrected the mistake in calculating parse end position  and
removed an unnecessary null check


webrev - http://cr.openjdk.java.net/~ntv/8154050/webrev.00/

PS: TCKOffsetPrinterParser.test_print_localized() already contain some test
cases related to parsing and formatting. therefore did not repeat in the new
test cases file

--
Thanks and Regards,
Nadeesh TV




--
Thanks and Regards,
Nadeesh TV

RFR: JDK-815440: JRT filesystem loaded by JDK8 with URLClassLoader is not closable since JDK-8147460

[Xueming Shen]

Hi,

Please help review the change for JDK-8154403

issue: https://bugs.openjdk.java.net/browse/JDK-8154403
webrev: http://cr.openjdk.java.net/~sherman/8154403/webrev/

The direct trigger of the failure is that the test case is passing in a 
null as parameter
"env" to create a new jrt filesystem. The nio spec specifies that it 
will trigger NPE. The
existing implementation does not check the "null" and passes it directly 
into
JrtFileSystem's constructor, in which the "null" is interpreted as "a 
not closable jrt file

system" (A implementation detail).

The fix is to add the "null check" in the provider and pass in "empty 
map" in the test case.


Alan, the original jrtfs implementation throws UOE in 
JrtFileSystemProvider.getTheFileSystem(),
is it better to just "silently" ignore the close request for the 
"not-closable" jrt? I'm file with

either way though.

Thanks,
Sherman

Project Jigsaw, Apache Tomcat and RMI related memory leaks

[Mark Thomas]

Hi,

The Apache Tomcat community was asked by Rory O'Donnell for feedback on
JDK 9 + Project Jigsaw. Having provided that feedback we were directed
here so I have reproduced that feedback below.


I've started testing Tomcat trunk with JDK 9 + Project Jigsaw and it
looks like we are going to hit a bunch of problems related to Tomcat's
memory leak protection.

The short version is there are lots of JDK calls that can result in a
reference being retained to the current class loader. If that class
loader is the web application class loader it often ends up being pinned
in memory. This triggers a memory leak when the web application is
reloaded since the web application class loader is not eligible for GC.

Tomcat generally uses reflection to find these problems. It then does
one of two things:
- If the JRE provides an API the application developer should have used
to clean up the reference, Tomcat does this for them and then logs a
very loud error message telling the developer they need to fix their app.
- If there is nothing the developer could have done to avoid the
problem, Tomcat cleans it up. Usually this is via reflection again.

Reporting this second class of issues as JRE bugs has been on my TODO
list for a long time. It looks like Java 9 is going to bump this to the
top of the list.

The first problem we have hit is related to RMI. The memory leak is
triggered by a call to:
java.rmi.registry.Registry.bind() or
java.rmi.registry.Registry.rebind()

The problem occurs when an object of a class loaded by the web
application class loader is bound to the RMI registry.

There is no standard API (that I have found) that completely removes all
references. In particular
java.rmi.registry.Registry.unbind()
doesn't help.

The code Tomcat uses to clean this up is at [1]. Essentially, we need to
remove any reference to the web application's class loader from the RMI
caches.

With JDK 9 this fails with:
java.lang.reflect.InaccessibleObjectException: Unable to make member of
class sun.rmi.transport.Target accessible:  module java.rmi does not
export sun.rmi.transport to unnamed module ...

I took a look at the JDK 9 API but I could not find a way to bypass
this.

Possible solutions include:
1. Some way that allows us to continue to use reflection as per the
current code.

2. A new method somewhere in the RMI API that clears all references
associated with a given class loader from the cache.

3. Modify Registry.unbind() so it removes all references.

4. Something else...

I do have a concern with 3 on its own that, while that would allow
applications to clear their own references, it would mean Tomcat would
have no way to check for the problem.

Ideally, I'd like to see the API extended so a) applications are able to
clean up after themselves and b) Tomcat can check for leaked references
and generate error messages for the ones found.

Any and all suggestions gratefully received.

Thanks,

Mark

[1]
http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/loader/WebappClassLoaderBase.java?view=annotate#l2214

Re: 8154159: rmic should not have a supported entry point

[Chris Hegarty]

On 18/04/16 15:57, Alan Bateman wrote:


When we brought the module system into JDK 9 then it included a new
entry point for the RMI compiler (rmic). This was a mistake (conflicts
with the policy for root modules that we have in JEP 261) and should not
have been brought into JDK 9.  The following patch removes it, it has
already been removed from the jake forest:

 http://cr.openjdk.java.net/~alanb/8154159/webrev/


To satisfy myself, I checked the history of the files in the
webrev, and your changes look like an accurate reversal of the
mistaken changes in this area. Reviewed.

-Chris.

Re: 8154159: rmic should not have a supported entry point

[Roger Riggs]

Look fine.

On 4/18/2016 10:57 AM, Alan Bateman wrote:


When we brought the module system into JDK 9 then it included a new 
entry point for the RMI compiler (rmic). This was a mistake (conflicts 
with the policy for root modules that we have in JEP 261) and should 
not have been brought into JDK 9.  The following patch removes it, it 
has already been removed from the jake forest:


http://cr.openjdk.java.net/~alanb/8154159/webrev/

Thanks,

Alan

8154159: rmic should not have a supported entry point

[Alan Bateman]

When we brought the module system into JDK 9 then it included a new 
entry point for the RMI compiler (rmic). This was a mistake (conflicts 
with the policy for root modules that we have in JEP 261) and should not 
have been brought into JDK 9.  The following patch removes it, it has 
already been removed from the jake forest:


http://cr.openjdk.java.net/~alanb/8154159/webrev/

Thanks,

Alan

Re: RFR: JDK-8152690: main thread does not have native thread name

[Kumar Srinivasan]

Hi,

Looks ok to me.

Thanks
Kumar


On 17/04/2016 8:38 PM, David Holmes wrote:

On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:

Hi David,


Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.


I'm waiting more reviewer(s) .

BTW, Should I make patches which are based on jdk9/dev repos?
My proposal includes hotspot changes.
So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
after reviewing.


No, jdk9/hs please.


And it will need to go via JPRT so I will sponsor it for you.

Thanks,
David


Thanks,
David


I can update my webrev to adapt to jdk9/dev repos if need.


Thanks,

Yasumasa


On 2016/04/17 7:20, David Holmes wrote:

Hi Yasumasa,

On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:

Hi David,

I uploaded new webrev:

  - hotspot:

http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/

  - jdk:
http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


Ah sneaky! :) Using JNI to by-pass access control so we can set up a
private method to do the name setting, yet avoid any API change that
would require a CCC request. I think I like it. :)

Need to hear from core-libs and/or launcher folk as this touches a
number of pieces of code.

Thanks,
David
-




it won't work unless you change the semantics of setName so I'm not
sure what you were thinking here. To take advantage of an arg taking
JVM_SetNativThreadName you would need to call it directly as no Java
code will call it . ???


I added a flag for setting native thread name to JNI-attached thread.
This change can set native thread name if main thread changes to
JNI-attached thread.


Thanks,

Yasumasa


On 2016/04/16 16:11, David Holmes wrote:

On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:

Hi David,

That change in behaviour may be a problem, it could be 
considered a

regression that setName stops setting the native thread main, even
though we never really intended it to work in the first place. :(
Such
a change needs to go through CCC.


I understood.
Can I send CCC request?
(I'm jdk 9 commiter, but I'm not employee at Oracle.)


Sorry you can't file a CCC request, you would need a sponsor for 
that.

But at this stage I don't think I agree with the proposed change
because of the change in behaviour - there's no way to restore the
"broken" behaviour.


I want to continue to discuss about it on JDK-8154331 [1].


Okay we can do that.




Further, we expect the launcher to use the supported JNI interface
(as
other processes would), not the internal JVM interface that exists
for
the JDK sources to communicate with the JVM.


I think that we do not use JVM interface if we add new method in
LauncherHelper as below:

diff -r f02139a1ac84
src/java.base/share/classes/sun/launcher/LauncherHelper.java
--- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Wed Apr 13 14:19:30 2016 +
+++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
Sat Apr 16 11:25:53 2016 +0900
@@ -960,4 +960,8 @@
  else
  return md.toNameAndVersion() + " (" + loc + ")";
  }
+
+static void setNativeThreadName(String name) {
+Thread.currentThread().setName(name);
+}


You could also make that call via JNI directly, so not sure the 
helper

adds much here. But it won't work unless you change the semantics of
setName so I'm not sure what you were thinking here. To take 
advantage

of an arg taking JVM_SetNativThreadName you would need to call it
directly as no Java code will call it . ???

David
-


  }
diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
--- a/src/java.base/share/native/libjli/java.cWed Apr 13 
14:19:30

2016 +
+++ b/src/java.base/share/native/libjli/java.cSat Apr 16 
11:25:53

2016 +0900
@@ -125,6 +125,7 @@
  static void PrintUsage(JNIEnv* env, jboolean doXUsage);
  static void ShowSettings(JNIEnv* env, char *optString);
  static void ListModules(JNIEnv* env, char *optString);
+static void SetNativeThreadName(JNIEnv* env, char *name);

  static void SetPaths(int argc, char **argv);

@@ -325,6 +326,7 @@
   * mainThread.isAlive() to work as expected.
   */
  #define LEAVE() \
+SetNativeThreadName(env, "DestroyJavaVM"); \
  do { \
  if ((*vm)->DetachCurrentThread(vm) != JNI_OK) { \
  JLI_ReportErrorMessage(JVM_ERROR2); \
@@ -488,6 +490,9 @@
  mainArgs = CreateApplicationArgs(env, argv, argc);
  CHECK_EXCEPTION_NULL_LEAVE(mainArgs);

+/* Set native thread name. */
+SetNativeThreadName(env, "main");
+
  /* Invoke main method. */
  (*env)->CallStaticVoidMethod(env, mainClass, mainID, 
mainArgs);


@@ -1686,6 +1691,22 @@
   joptString);
  }

+/**
+ * Set native thread name as possible.
+ */
+static void
+SetNativeThreadName(JNIEnv *env, char *name)
+{
+jmethodID setNativeThreadNameID;
+jstring nameString;
+jclass cls = GetLauncherHelperClass(env);
+NULL_CHECK(cls);
+

Re: RFR [9] 8147553: Remove sun.misc.ManagedLocalsThread from java.management

[Daniel Fuchs]

Hi Chris,

The changes look good to me.

best regards,

-- daniel

On 18/04/16 12:37, Chris Hegarty wrote:

8056152 added a new constructor to java.lang.Thread to constructing
Threads that do not  inherit inheritable-thread-local initial values.
Given there is now a supported API for creating such threads, other
areas of the JDK should be updated to use it.

This change updates the code in java.management to use the new Thread
constructor.

http://cr.openjdk.java.net/~chegar/8147553/
https://bugs.openjdk.java.net/browse/JDK-8147553

-Chris.

Re: RFR [9] 8153158: Remove sun.misc.ManagedLocalsThread from java.logging

[Daniel Fuchs]

On 18/04/16 08:01, Chris Hegarty wrote:

8056152 added a new constructor to java.lang.Thread to constructing Threads that
do not  inherit inheritable-thread-local initial values. Given there is now a 
supported
API for creating such threads, other areas of the JDK should be updated to use 
it.

This change updates the code in java.logging to use the new Thread constructor.


Hi Chris,

Looks good to me.

best regards

-- daniel



--- a/src/java.logging/share/classes/java/util/logging/LogManager.java
+++ b/src/java.logging/share/classes/java/util/logging/LogManager.java
@@ -42,7 +42,6 @@
 import java.util.stream.Stream;
 import jdk.internal.misc.JavaAWTAccess;
 import jdk.internal.misc.SharedSecrets;
-import sun.misc.ManagedLocalsThread;
 import sun.util.logging.internal.LoggingProviderImpl;

 /**
@@ -254,9 +253,10 @@

 // This private class is used as a shutdown hook.
 // It does a "reset" to close all open handlers.
-private class Cleaner extends ManagedLocalsThread {
+private class Cleaner extends Thread {

 private Cleaner() {
+super(null, null, "Logging-Cleaner", 0, false);
 /* Set context class loader to null in order to avoid
  * keeping a strong reference to an application classloader.
  */
diff --git a/src/java.logging/share/classes/module-info.java 
b/src/java.logging/share/classes/module-info.java
--- a/src/java.logging/share/classes/module-info.java
+++ b/src/java.logging/share/classes/module-info.java
@@ -24,8 +24,6 @@
  */

 module java.logging {
-// 8153158
-requires jdk.unsupported;
 exports java.util.logging;
 provides jdk.internal.logger.DefaultLoggerFinder with
 sun.util.logging.internal.LoggingProviderImpl;

-Chris.

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Paul Sandoz]

> On 18 Apr 2016, at 14:35, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> Thank you for review!
> 
> PS>  913 UnorderedSliceSpliterator(T_SPLITR s, long skip, long limit) 
> {
> PS>  914 this.s = s;
> PS>  915 this.unlimited = limit < 0;
> PS>  916 this.skipThreshold = limit >= 0 ? limit : 0;
> PS>  917 this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
> PS>  918 (skip + limit) /
> PS> ForkJoinPool.getCommonPoolParallelism() + 1) : CHUNK_SIZE;
> PS>  919 this.permits = new AtomicLong(limit >= 0 ? skip + limit 
> : skip);
> PS>  920 }
> PS>  921
> 
> PS> Note the common pool parallelism can never be 0. I dunno if you
> PS> added 1 for that or another reason.
> 
> It's actually
> ((skip + limit) / ForkJoinPool.getCommonPoolParallelism()) + 1
> Not
> (skip + limit) / (ForkJoinPool.getCommonPoolParallelism() + 1)
> 
> Probably  I should add explicit parentheses to make this clear. One is
> added exactly to make chunkSize at least 1.
> 

Doh. I think i need glasses…

A comment on the range might help too.


> PS> Did you consider:
> 
> PS>   (skip + limit) / AbstractTask.LEAF_TARGET
> 
> PS> ?
> 
> It should not make drastic changes in my test, but I will try.
> 

Ok.


> PS> What if chunkSize is zero? should it be a minimum of 1?
> 
> PS> Testing wise i think our existing tests cover things ok.
> 
> PS> Performance-wise looks good. Probable primes are my favourite way
> PS> of easily increasing Q (cost per op) :-)
> 
> PS> Can you run the stream tests and the perf tests with parallelism disabled:
> 
> PS>   -Djava.util.concurrent.ForkJoinPool.common.parallelism=1
> 
> Ok. I think I should also test the performance for some high-N low-Q
> task to check whether it not degrades. Will perform all the tests
> later this week.
> 
> By the way is these some special place to commit/store JMH tests
> (except CodeReview server), so they could be reused later?
> 

Not that i am aware of. The best thing for the moment is to place ‘em on 
cr.openjdk as you have done.

Paul.

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Tagir F. Valeev]

Hello!

Thank you for review!

PS>  913 UnorderedSliceSpliterator(T_SPLITR s, long skip, long limit) {
PS>  914 this.s = s;
PS>  915 this.unlimited = limit < 0;
PS>  916 this.skipThreshold = limit >= 0 ? limit : 0;
PS>  917 this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
PS>  918 (skip + limit) /
PS> ForkJoinPool.getCommonPoolParallelism() + 1) : CHUNK_SIZE;
PS>  919 this.permits = new AtomicLong(limit >= 0 ? skip + limit : 
skip);
PS>  920 }
PS>  921

PS> Note the common pool parallelism can never be 0. I dunno if you
PS> added 1 for that or another reason.

It's actually
((skip + limit) / ForkJoinPool.getCommonPoolParallelism()) + 1
Not
(skip + limit) / (ForkJoinPool.getCommonPoolParallelism() + 1)

Probably  I should add explicit parentheses to make this clear. One is
added exactly to make chunkSize at least 1.

PS> Did you consider:

PS>   (skip + limit) / AbstractTask.LEAF_TARGET

PS> ?

It should not make drastic changes in my test, but I will try.

PS> What if chunkSize is zero? should it be a minimum of 1?

PS> Testing wise i think our existing tests cover things ok.

PS> Performance-wise looks good. Probable primes are my favourite way
PS> of easily increasing Q (cost per op) :-)

PS> Can you run the stream tests and the perf tests with parallelism disabled:

PS>   -Djava.util.concurrent.ForkJoinPool.common.parallelism=1

Ok. I think I should also test the performance for some high-N low-Q
task to check whether it not degrades. Will perform all the tests
later this week.

By the way is these some special place to commit/store JMH tests
(except CodeReview server), so they could be reused later?

With best regards,
Tagir Valeev.

PS> ?

PS> Thanks,
PS> Paul.


>> The rationale is to speed-up the parallel processing for unordered
>> streams with low limit value. Such problems occur when you want to
>> perform expensive filtering and select at most x elements which pass
>> the filter (order does not matter). Currently unordered limit
>> operation buffers up to 128 elements for each parallel task before it
>> checks whether limit is reached. This is actually harmful when
>> requested limit is lower: much more elements are requested from the
>> upstream than necessary. Here's simple JMH test which illustrates the
>> problem:
>> 
>> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/jmh/
>> It extracts the requested number of probable-primes from the list of
>> 1 BigInteger numbers. The results with 9ea+111:
>> 
>> Benchmark(limit)  Mode  Cnt  Score  Error  Units
>> LimitTest.parLimit 2  avgt   30108,971 ±0,643  us/op
>> LimitTest.parLimit20  avgt   30934,176 ±   14,003  us/op
>> LimitTest.parLimit   200  avgt   30   8772,417 ±  190,609  us/op
>> LimitTest.parLimit  2000  avgt   30  41775,463 ± 1800,537  us/op
>> LimitTest.parUnorderedLimit2  avgt   30   2557,798 ±   13,161  us/op
>> LimitTest.parUnorderedLimit   20  avgt   30   2578,283 ±   23,547  us/op
>> LimitTest.parUnorderedLimit  200  avgt   30   4577,318 ±   40,793  us/op
>> LimitTest.parUnorderedLimit 2000  avgt   30  12279,346 ±  523,823  us/op
>> LimitTest.seqLimit 2  avgt   30 34,831 ±0,190  us/op
>> LimitTest.seqLimit20  avgt   30369,729 ±1,427  us/op
>> LimitTest.seqLimit   200  avgt   30   3690,544 ±   13,907  us/op
>> LimitTest.seqLimit  2000  avgt   30  36681,637 ±  156,538  us/op
>> 
>> When the limit is 2 or 20, parallel unordered version is slower than
>> parallel ordered! Even for limit = 200 it's still slower than
>> sequential operation.
>> 
>> The idea of the patch is to tweak the CHUNK_SIZE using the given limit and
>> parallelism level. I used the following formula:
>> 
>> this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
>> (skip + limit) / ForkJoinPool.getCommonPoolParallelism() + 1) : 
>> CHUNK_SIZE;
>> 
>> This does not affect cases when limit is big or not set at all (in
>> skip mode). However it greatly improves cases when limit is small:
>> 
>> Benchmark(limit)  Mode  Cnt  Score  Error  Units
>> LimitTest.parLimit 2  avgt   30109,502 ±0,750  us/op
>> LimitTest.parLimit20  avgt   30954,716 ±   39,276  us/op
>> LimitTest.parLimit   200  avgt   30   8706,226 ±  184,330  us/op
>> LimitTest.parLimit  2000  avgt   30  42126,346 ± 3163,444  us/op
>> LimitTest.parUnorderedLimit2  avgt   30 39,303 ±0,177  us/op 
>> !!!
>> LimitTest.parUnorderedLimit   20  avgt   30266,107 ±0,492  us/op 
>> !!!
>> LimitTest.parUnorderedLimit  200  avgt   30   2547,177 ±   58,538  us/op 
>> !!!
>> LimitTest.parUnorderedLimit 2000  avgt   30  12216,402 ±  430,574  us/op
>> LimitTest.seqLimit 2  avgt   30 

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Paul Sandoz]

Hi Tagir,

> On 16 Apr 2016, at 15:05, Tagir F. Valeev  wrote:
> 
> Hello!
> 
> Please review and sponsor the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8154387

Thanks for looking at this, it’s something i intended to get around to but 
never found the time. I closed JDK-8072841 as a dup of this.


> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/r1/
> 

 913 UnorderedSliceSpliterator(T_SPLITR s, long skip, long limit) {
 914 this.s = s;
 915 this.unlimited = limit < 0;
 916 this.skipThreshold = limit >= 0 ? limit : 0;
 917 this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
 918 (skip + limit) / ForkJoinPool.getCommonPoolParallelism() + 
1) : CHUNK_SIZE;
 919 this.permits = new AtomicLong(limit >= 0 ? skip + limit : 
skip);
 920 }
 921

Note the common pool parallelism can never be 0. I dunno if you added 1 for 
that or another reason.

Did you consider:

  (skip + limit) / AbstractTask.LEAF_TARGET

?

What if chunkSize is zero? should it be a minimum of 1?

Testing wise i think our existing tests cover things ok.

Performance-wise looks good. Probable primes are my favourite way of easily 
increasing Q (cost per op) :-)

Can you run the stream tests and the perf tests with parallelism disabled:

  -Djava.util.concurrent.ForkJoinPool.common.parallelism=1

?

Thanks,
Paul.


> The rationale is to speed-up the parallel processing for unordered
> streams with low limit value. Such problems occur when you want to
> perform expensive filtering and select at most x elements which pass
> the filter (order does not matter). Currently unordered limit
> operation buffers up to 128 elements for each parallel task before it
> checks whether limit is reached. This is actually harmful when
> requested limit is lower: much more elements are requested from the
> upstream than necessary. Here's simple JMH test which illustrates the
> problem:
> 
> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/jmh/
> It extracts the requested number of probable-primes from the list of
> 1 BigInteger numbers. The results with 9ea+111:
> 
> Benchmark(limit)  Mode  Cnt  Score  Error  Units
> LimitTest.parLimit 2  avgt   30108,971 ±0,643  us/op
> LimitTest.parLimit20  avgt   30934,176 ±   14,003  us/op
> LimitTest.parLimit   200  avgt   30   8772,417 ±  190,609  us/op
> LimitTest.parLimit  2000  avgt   30  41775,463 ± 1800,537  us/op
> LimitTest.parUnorderedLimit2  avgt   30   2557,798 ±   13,161  us/op
> LimitTest.parUnorderedLimit   20  avgt   30   2578,283 ±   23,547  us/op
> LimitTest.parUnorderedLimit  200  avgt   30   4577,318 ±   40,793  us/op
> LimitTest.parUnorderedLimit 2000  avgt   30  12279,346 ±  523,823  us/op
> LimitTest.seqLimit 2  avgt   30 34,831 ±0,190  us/op
> LimitTest.seqLimit20  avgt   30369,729 ±1,427  us/op
> LimitTest.seqLimit   200  avgt   30   3690,544 ±   13,907  us/op
> LimitTest.seqLimit  2000  avgt   30  36681,637 ±  156,538  us/op
> 
> When the limit is 2 or 20, parallel unordered version is slower than
> parallel ordered! Even for limit = 200 it's still slower than
> sequential operation.
> 
> The idea of the patch is to tweak the CHUNK_SIZE using the given limit and
> parallelism level. I used the following formula:
> 
> this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
> (skip + limit) / ForkJoinPool.getCommonPoolParallelism() + 1) : 
> CHUNK_SIZE;
> 
> This does not affect cases when limit is big or not set at all (in
> skip mode). However it greatly improves cases when limit is small:
> 
> Benchmark(limit)  Mode  Cnt  Score  Error  Units
> LimitTest.parLimit 2  avgt   30109,502 ±0,750  us/op
> LimitTest.parLimit20  avgt   30954,716 ±   39,276  us/op
> LimitTest.parLimit   200  avgt   30   8706,226 ±  184,330  us/op
> LimitTest.parLimit  2000  avgt   30  42126,346 ± 3163,444  us/op
> LimitTest.parUnorderedLimit2  avgt   30 39,303 ±0,177  us/op 
> !!!
> LimitTest.parUnorderedLimit   20  avgt   30266,107 ±0,492  us/op 
> !!!
> LimitTest.parUnorderedLimit  200  avgt   30   2547,177 ±   58,538  us/op 
> !!!
> LimitTest.parUnorderedLimit 2000  avgt   30  12216,402 ±  430,574  us/op
> LimitTest.seqLimit 2  avgt   30 34,993 ±0,704  us/op
> LimitTest.seqLimit20  avgt   30369,497 ±1,754  us/op
> LimitTest.seqLimit   200  avgt   30   3716,059 ±   61,054  us/op
> LimitTest.seqLimit  2000  avgt   30  36814,356 ±  161,531  us/op
> 
> Here you can see that unordered cases are significantly improved. Now
> they are always faster than parallel ordered and faster than
> sequential for 

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Stefan Zobel]

2016-04-18 14:01 GMT+02:00 Tagir F. Valeev :
> Hello!
>
> It was just a quick test not in the clean environment, so you should
> not draw any conclusions from the error numbers. It's quite expected
> that for limit = 2000 the performance is the same as I have 4 CPU
> machine and 2000 is greater than 128*4. On the other hand, 200 is less
> than 128*4, so this case is also improved (though not so drastically
> as less limits).

Thanks for the clarification Tagir. Makes sense now.
I missed the "commonPoolParallelism + 1" bit ...

Regards, Stefan

>
> With best regards,
> Tagir Valeev.
>

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Tagir F. Valeev]

Hello!

SZ> I'm a bit surprised about the JMH results for limits 200 and 2000.

SZ> limit = 200 is significantly faster than the unpatched code (with
SZ> higher variance, though) and limit = 2000 is about the same, but
SZ> with a significantly reduced variance. Maybe you'd need to increase
SZ> the number of iterations / forks to get more stable results that
SZ> are in line with expectations - or do I miss something here?

It was just a quick test not in the clean environment, so you should
not draw any conclusions from the error numbers. It's quite expected
that for limit = 2000 the performance is the same as I have 4 CPU
machine and 2000 is greater than 128*4. On the other hand, 200 is less
than 128*4, so this case is also improved (though not so drastically
as less limits).

With best regards,
Tagir Valeev.


SZ> Regards,
SZ> Stefan


SZ> 2016-04-16 15:05 GMT+02:00 Tagir F. Valeev :
>> Hello!
>>
>> Please review and sponsor the following patch:
>> https://bugs.openjdk.java.net/browse/JDK-8154387
>> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/r1/
>>
>> The rationale is to speed-up the parallel processing for unordered
>> streams with low limit value. Such problems occur when you want to
>> perform expensive filtering and select at most x elements which pass
>> the filter (order does not matter). Currently unordered limit
>> operation buffers up to 128 elements for each parallel task before it
>> checks whether limit is reached. This is actually harmful when
>> requested limit is lower: much more elements are requested from the
>> upstream than necessary. Here's simple JMH test which illustrates the
>> problem:
>>
>> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/jmh/
>> It extracts the requested number of probable-primes from the list of
>> 1 BigInteger numbers. The results with 9ea+111:
>>
>> Benchmark(limit)  Mode  Cnt  Score  Error  Units
>> LimitTest.parLimit 2  avgt   30108,971 ±0,643  us/op
>> LimitTest.parLimit20  avgt   30934,176 ±   14,003  us/op
>> LimitTest.parLimit   200  avgt   30   8772,417 ±  190,609  us/op
>> LimitTest.parLimit  2000  avgt   30  41775,463 ± 1800,537  us/op
>> LimitTest.parUnorderedLimit2  avgt   30   2557,798 ±   13,161  us/op
>> LimitTest.parUnorderedLimit   20  avgt   30   2578,283 ±   23,547  us/op
>> LimitTest.parUnorderedLimit  200  avgt   30   4577,318 ±   40,793  us/op
>> LimitTest.parUnorderedLimit 2000  avgt   30  12279,346 ±  523,823  us/op
>> LimitTest.seqLimit 2  avgt   30 34,831 ±0,190  us/op
>> LimitTest.seqLimit20  avgt   30369,729 ±1,427  us/op
>> LimitTest.seqLimit   200  avgt   30   3690,544 ±   13,907  us/op
>> LimitTest.seqLimit  2000  avgt   30  36681,637 ±  156,538  us/op
>>
>> When the limit is 2 or 20, parallel unordered version is slower than
>> parallel ordered! Even for limit = 200 it's still slower than
>> sequential operation.
>>
>> The idea of the patch is to tweak the CHUNK_SIZE using the given limit and
>> parallelism level. I used the following formula:
>>
>> this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
>>  (skip + limit) / ForkJoinPool.getCommonPoolParallelism() + 1) : 
>> CHUNK_SIZE;
>>
>> This does not affect cases when limit is big or not set at all (in
>> skip mode). However it greatly improves cases when limit is small:
>>
>> Benchmark(limit)  Mode  Cnt  Score  Error  Units
>> LimitTest.parLimit 2  avgt   30109,502 ±0,750  us/op
>> LimitTest.parLimit20  avgt   30954,716 ±   39,276  us/op
>> LimitTest.parLimit   200  avgt   30   8706,226 ±  184,330  us/op
>> LimitTest.parLimit  2000  avgt   30  42126,346 ± 3163,444  us/op
>> LimitTest.parUnorderedLimit2  avgt   30 39,303 ±0,177  us/op 
>> !!!
>> LimitTest.parUnorderedLimit   20  avgt   30266,107 ±0,492  us/op 
>> !!!
>> LimitTest.parUnorderedLimit  200  avgt   30   2547,177 ±   58,538  us/op 
>> !!!
>> LimitTest.parUnorderedLimit 2000  avgt   30  12216,402 ±  430,574  us/op
>> LimitTest.seqLimit 2  avgt   30 34,993 ±0,704  us/op
>> LimitTest.seqLimit20  avgt   30369,497 ±1,754  us/op
>> LimitTest.seqLimit   200  avgt   30   3716,059 ±   61,054  us/op
>> LimitTest.seqLimit  2000  avgt   30  36814,356 ±  161,531  us/op
>>
>> Here you can see that unordered cases are significantly improved. Now
>> they are always faster than parallel ordered and faster than
>> sequential for limit >= 20.
>>
>> I did not think up how to test this patch as it does not change
>> visible behavior, only speed. However all the existing tests pass.
>>
>> What do you think?
>>
>> With best regards,
>> Tagir Valeev.
>>

Re: RFR: 8154387 - Parallel unordered Stream.limit() tries to collect 128 elements even if limit is less

[Stefan Zobel]

Hi Tagir,

nice catch. I think this optimization is worthwile.

I'm a bit surprised about the JMH results for limits 200 and 2000.

limit = 200 is significantly faster than the unpatched code (with
higher variance, though) and limit = 2000 is about the same, but
with a significantly reduced variance. Maybe you'd need to increase
the number of iterations / forks to get more stable results that
are in line with expectations - or do I miss something here?


Regards,
Stefan


2016-04-16 15:05 GMT+02:00 Tagir F. Valeev :
> Hello!
>
> Please review and sponsor the following patch:
> https://bugs.openjdk.java.net/browse/JDK-8154387
> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/r1/
>
> The rationale is to speed-up the parallel processing for unordered
> streams with low limit value. Such problems occur when you want to
> perform expensive filtering and select at most x elements which pass
> the filter (order does not matter). Currently unordered limit
> operation buffers up to 128 elements for each parallel task before it
> checks whether limit is reached. This is actually harmful when
> requested limit is lower: much more elements are requested from the
> upstream than necessary. Here's simple JMH test which illustrates the
> problem:
>
> http://cr.openjdk.java.net/~tvaleev/webrev/8154387/jmh/
> It extracts the requested number of probable-primes from the list of
> 1 BigInteger numbers. The results with 9ea+111:
>
> Benchmark(limit)  Mode  Cnt  Score  Error  Units
> LimitTest.parLimit 2  avgt   30108,971 ±0,643  us/op
> LimitTest.parLimit20  avgt   30934,176 ±   14,003  us/op
> LimitTest.parLimit   200  avgt   30   8772,417 ±  190,609  us/op
> LimitTest.parLimit  2000  avgt   30  41775,463 ± 1800,537  us/op
> LimitTest.parUnorderedLimit2  avgt   30   2557,798 ±   13,161  us/op
> LimitTest.parUnorderedLimit   20  avgt   30   2578,283 ±   23,547  us/op
> LimitTest.parUnorderedLimit  200  avgt   30   4577,318 ±   40,793  us/op
> LimitTest.parUnorderedLimit 2000  avgt   30  12279,346 ±  523,823  us/op
> LimitTest.seqLimit 2  avgt   30 34,831 ±0,190  us/op
> LimitTest.seqLimit20  avgt   30369,729 ±1,427  us/op
> LimitTest.seqLimit   200  avgt   30   3690,544 ±   13,907  us/op
> LimitTest.seqLimit  2000  avgt   30  36681,637 ±  156,538  us/op
>
> When the limit is 2 or 20, parallel unordered version is slower than
> parallel ordered! Even for limit = 200 it's still slower than
> sequential operation.
>
> The idea of the patch is to tweak the CHUNK_SIZE using the given limit and
> parallelism level. I used the following formula:
>
> this.chunkSize = limit >= 0 ? (int)Math.min(CHUNK_SIZE,
>  (skip + limit) / ForkJoinPool.getCommonPoolParallelism() + 1) : 
> CHUNK_SIZE;
>
> This does not affect cases when limit is big or not set at all (in
> skip mode). However it greatly improves cases when limit is small:
>
> Benchmark(limit)  Mode  Cnt  Score  Error  Units
> LimitTest.parLimit 2  avgt   30109,502 ±0,750  us/op
> LimitTest.parLimit20  avgt   30954,716 ±   39,276  us/op
> LimitTest.parLimit   200  avgt   30   8706,226 ±  184,330  us/op
> LimitTest.parLimit  2000  avgt   30  42126,346 ± 3163,444  us/op
> LimitTest.parUnorderedLimit2  avgt   30 39,303 ±0,177  us/op 
> !!!
> LimitTest.parUnorderedLimit   20  avgt   30266,107 ±0,492  us/op 
> !!!
> LimitTest.parUnorderedLimit  200  avgt   30   2547,177 ±   58,538  us/op 
> !!!
> LimitTest.parUnorderedLimit 2000  avgt   30  12216,402 ±  430,574  us/op
> LimitTest.seqLimit 2  avgt   30 34,993 ±0,704  us/op
> LimitTest.seqLimit20  avgt   30369,497 ±1,754  us/op
> LimitTest.seqLimit   200  avgt   30   3716,059 ±   61,054  us/op
> LimitTest.seqLimit  2000  avgt   30  36814,356 ±  161,531  us/op
>
> Here you can see that unordered cases are significantly improved. Now
> they are always faster than parallel ordered and faster than
> sequential for limit >= 20.
>
> I did not think up how to test this patch as it does not change
> visible behavior, only speed. However all the existing tests pass.
>
> What do you think?
>
> With best regards,
> Tagir Valeev.
>

RFR [9] 8147553: Remove sun.misc.ManagedLocalsThread from java.management

[Chris Hegarty]

8056152 added a new constructor to java.lang.Thread to constructing
Threads that do not  inherit inheritable-thread-local initial values.
Given there is now a supported API for creating such threads, other 
areas of the JDK should be updated to use it.


This change updates the code in java.management to use the new Thread 
constructor.


http://cr.openjdk.java.net/~chegar/8147553/
https://bugs.openjdk.java.net/browse/JDK-8147553

-Chris.

Re: RFR: JDK-8152690: main thread does not have native thread name

[Yasumasa Suenaga]

2016/04/18 13:41 "David Holmes" :
>
> On 17/04/2016 8:38 PM, David Holmes wrote:
>>
>> On 17/04/2016 1:38 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
 Need to hear from core-libs and/or launcher folk as this touches a
 number of pieces of code.
>>>
>>>
>>> I'm waiting more reviewer(s) .
>>>
>>> BTW, Should I make patches which are based on jdk9/dev repos?
>>> My proposal includes hotspot changes.
>>> So I want to know whether I can push all changes jdk9/dev/{jdk,hotspot}
>>> after reviewing.
>>
>>
>> No, jdk9/hs please.
>
>
> And it will need to go via JPRT so I will sponsor it for you.

Thanks!
I will send changesets to you after reviewing.

Yasumasa

> Thanks,
> David
>
>
>> Thanks,
>> David
>>
>>> I can update my webrev to adapt to jdk9/dev repos if need.
>>>
>>>
>>> Thanks,
>>>
>>> Yasumasa
>>>
>>>
>>> On 2016/04/17 7:20, David Holmes wrote:

 Hi Yasumasa,

 On 16/04/2016 7:29 PM, Yasumasa Suenaga wrote:
>
> Hi David,
>
> I uploaded new webrev:
>
>   - hotspot:
>
> http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/hotspot/
>
>   - jdk:
>  http://cr.openjdk.java.net/~ysuenaga/JDK-8152690/webrev.03/jdk/


 Ah sneaky! :) Using JNI to by-pass access control so we can set up a
 private method to do the name setting, yet avoid any API change that
 would require a CCC request. I think I like it. :)

 Need to hear from core-libs and/or launcher folk as this touches a
 number of pieces of code.

 Thanks,
 David
 -

>
>> it won't work unless you change the semantics of setName so I'm not
>> sure what you were thinking here. To take advantage of an arg taking
>> JVM_SetNativThreadName you would need to call it directly as no Java
>> code will call it . ???
>
>
> I added a flag for setting native thread name to JNI-attached thread.
> This change can set native thread name if main thread changes to
> JNI-attached thread.
>
>
> Thanks,
>
> Yasumasa
>
>
> On 2016/04/16 16:11, David Holmes wrote:
>>
>> On 16/04/2016 3:27 PM, Yasumasa Suenaga wrote:
>>>
>>> Hi David,
>>>
 That change in behaviour may be a problem, it could be considered a
 regression that setName stops setting the native thread main, even
 though we never really intended it to work in the first place. :(
 Such
 a change needs to go through CCC.
>>>
>>>
>>> I understood.
>>> Can I send CCC request?
>>> (I'm jdk 9 commiter, but I'm not employee at Oracle.)
>>
>>
>> Sorry you can't file a CCC request, you would need a sponsor for
that.
>> But at this stage I don't think I agree with the proposed change
>> because of the change in behaviour - there's no way to restore the
>> "broken" behaviour.
>>
>>> I want to continue to discuss about it on JDK-8154331 [1].
>>
>>
>> Okay we can do that.
>>
>>>
 Further, we expect the launcher to use the supported JNI interface
 (as
 other processes would), not the internal JVM interface that exists
 for
 the JDK sources to communicate with the JVM.
>>>
>>>
>>> I think that we do not use JVM interface if we add new method in
>>> LauncherHelper as below:
>>> 
>>> diff -r f02139a1ac84
>>> src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> --- a/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Wed Apr 13 14:19:30 2016 +
>>> +++ b/src/java.base/share/classes/sun/launcher/LauncherHelper.java
>>> Sat Apr 16 11:25:53 2016 +0900
>>> @@ -960,4 +960,8 @@
>>>   else
>>>   return md.toNameAndVersion() + " (" + loc + ")";
>>>   }
>>> +
>>> +static void setNativeThreadName(String name) {
>>> +Thread.currentThread().setName(name);
>>> +}
>>
>>
>> You could also make that call via JNI directly, so not sure the
helper
>> adds much here. But it won't work unless you change the semantics of
>> setName so I'm not sure what you were thinking here. To take
advantage
>> of an arg taking JVM_SetNativThreadName you would need to call it
>> directly as no Java code will call it . ???
>>
>> David
>> -
>>
>>>   }
>>> diff -r f02139a1ac84 src/java.base/share/native/libjli/java.c
>>> --- a/src/java.base/share/native/libjli/java.cWed Apr 13
14:19:30
>>> 2016 +
>>> +++ b/src/java.base/share/native/libjli/java.cSat Apr 16
11:25:53
>>> 2016 +0900
>>> @@ -125,6 +125,7 @@
>>>   static void PrintUsage(JNIEnv* env, jboolean doXUsage);
>>>   static void ShowSettings(JNIEnv* env, char *optString);
>>>   static void ListModules(JNIEnv* env, char *optString);
>>> +static void 

Re: RFR [9] 8153158: Remove sun.misc.ManagedLocalsThread from java.logging

[Claes Redestad]

Looks good, Chris

/Claes

On 04/18/2016 08:01 AM, Chris Hegarty wrote:

8056152 added a new constructor to java.lang.Thread to constructing Threads that
do not  inherit inheritable-thread-local initial values. Given there is now a 
supported
API for creating such threads, other areas of the JDK should be updated to use 
it.

This change updates the code in java.logging to use the new Thread constructor.

--- a/src/java.logging/share/classes/java/util/logging/LogManager.java
+++ b/src/java.logging/share/classes/java/util/logging/LogManager.java
@@ -42,7 +42,6 @@
  import java.util.stream.Stream;
  import jdk.internal.misc.JavaAWTAccess;
  import jdk.internal.misc.SharedSecrets;
-import sun.misc.ManagedLocalsThread;
  import sun.util.logging.internal.LoggingProviderImpl;
  
  /**

@@ -254,9 +253,10 @@
  
  // This private class is used as a shutdown hook.

  // It does a "reset" to close all open handlers.
-private class Cleaner extends ManagedLocalsThread {
+private class Cleaner extends Thread {
  
  private Cleaner() {

+super(null, null, "Logging-Cleaner", 0, false);
  /* Set context class loader to null in order to avoid
   * keeping a strong reference to an application classloader.
   */
diff --git a/src/java.logging/share/classes/module-info.java 
b/src/java.logging/share/classes/module-info.java
--- a/src/java.logging/share/classes/module-info.java
+++ b/src/java.logging/share/classes/module-info.java
@@ -24,8 +24,6 @@
   */
  
  module java.logging {

-// 8153158
-requires jdk.unsupported;
  exports java.util.logging;
  provides jdk.internal.logger.DefaultLoggerFinder with
  sun.util.logging.internal.LoggingProviderImpl;

-Chris.

Re: RFR: 8151056: ASM enable original deprecated methods. (simple fix)

[Remi Forax]

Thumb up !

Rémi

- Mail original -
> De: "Sundararajan Athijegannathan" 
> À: core-libs-dev@openjdk.java.net
> Envoyé: Lundi 18 Avril 2016 06:18:37
> Objet: Re: RFR: 8151056: ASM enable original deprecated methods. (simple fix)
> 
> Looks good
> 
> -Sundar
> 
> On 4/16/2016 2:43 AM, Kumar Srinivasan wrote:
> > Hello,
> >
> > Please review this simple fix [1] to re-instate the original deprecated
> > methods, as a background, there were commented out because
> > of an internal dependency [2], and since has been fixed.
> >
> > Thanks
> > Kumar
> >
> >
> > [1] http://cr.openjdk.java.net/~ksrini/8151056/webrev.00/
> > [2]
> > http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-March/039256.html
> 
> 

RFR [9] 8153158: Remove sun.misc.ManagedLocalsThread from java.logging

[Chris Hegarty]

8056152 added a new constructor to java.lang.Thread to constructing Threads that
do not  inherit inheritable-thread-local initial values. Given there is now a 
supported
API for creating such threads, other areas of the JDK should be updated to use 
it.

This change updates the code in java.logging to use the new Thread constructor.

--- a/src/java.logging/share/classes/java/util/logging/LogManager.java
+++ b/src/java.logging/share/classes/java/util/logging/LogManager.java
@@ -42,7 +42,6 @@
 import java.util.stream.Stream;
 import jdk.internal.misc.JavaAWTAccess;
 import jdk.internal.misc.SharedSecrets;
-import sun.misc.ManagedLocalsThread;
 import sun.util.logging.internal.LoggingProviderImpl;
 
 /**
@@ -254,9 +253,10 @@
 
 // This private class is used as a shutdown hook.
 // It does a "reset" to close all open handlers.
-private class Cleaner extends ManagedLocalsThread {
+private class Cleaner extends Thread {
 
 private Cleaner() {
+super(null, null, "Logging-Cleaner", 0, false);
 /* Set context class loader to null in order to avoid
  * keeping a strong reference to an application classloader.
  */
diff --git a/src/java.logging/share/classes/module-info.java 
b/src/java.logging/share/classes/module-info.java
--- a/src/java.logging/share/classes/module-info.java
+++ b/src/java.logging/share/classes/module-info.java
@@ -24,8 +24,6 @@
  */
 
 module java.logging {
-// 8153158
-requires jdk.unsupported;
 exports java.util.logging;
 provides jdk.internal.logger.DefaultLoggerFinder with
 sun.util.logging.internal.LoggingProviderImpl;

-Chris.