Re: Testing hotspot's Atomic::cmpxchg(jbyte*) ?

[Aleksey Shipilev]

On 08/19/2016 09:31 AM, Aleksey Shipilev wrote:
> Now it might be cleaner to ditch Java version from Unsafe, and make
> native entry points like Unsafe_CompareAnd{Exchange,Swap}{Byte,Short}
> which would call relevant Atomic::cmpxchg-s.

...or not. There is no Atomic::cmpxchg(jshort), argh.

Thanks,
-Aleksey

Re: Testing hotspot's Atomic::cmpxchg(jbyte*) ?

[Aleksey Shipilev]

On 08/19/2016 08:15 AM, David Holmes wrote:
> I need to test the jbyte version of Atomic::cmpxchg in the VM, but so
> far it doesn't seem to be the case that any library code will actually
> invoke it. At the top Java level there is no AtomicByte class. Unsafe
> defines compareAndSwapByte variants but implements them using
> compareAndSwapInt and isolating the byte of interest. VarHandles seems
> to have direct *SwapByte operations but I can't figure out what they map
> to - there are VM intrinsics but again I can't the actual implementation
> to see if it uses Atomic::cmpxchg(jbyte*).

VarHandles byte CAS/CAE map to Unsafe. Therefore they will use the
Java-ish compareAndExchangeByte from Unsafe, without touching
Atomic::cmpxchg(jbyte).

In retrospect, we did that before Atomic::cmpxchg(jbyte) was available.
Now it might be cleaner to ditch Java version from Unsafe, and make
native entry points like Unsafe_CompareAnd{Exchange,Swap}{Byte,Short}
which would call relevant Atomic::cmpxchg-s.

Then, regular jdk/test/java/lang/invoke/varhandles and
hotspot/tests/compiler/unsafe tests would exercise that path, as they
run in interpreter too.

Thanks,
-Aleksey

Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[nadeesh tv]

Hi  Ivan,

ZoneOffset.ofTotalSeconds(Integer.MIN_VALUE) have also the same issue. It' not 
throwing the expected DTE.

May be you can correct this also as part of this.

Please update the copyright year also.

Rest looks good.

--
Thanks and Regards,
Nadeesh TV



On 8/18/2016 10:23 PM, Ivan Gerasimov wrote:

Hello everybody!

The factory methods of ZoneOffset class demonstrate a minor 
inconsistency.
Normally, invalid values of arguments are rejected (e.g. when minutes 
> 59), but the value of Integer.MIN_VALUE is allowed to be passed in.


This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE 
well.


Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366
WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/

With kind regards,
Ivan



--
Thanks and Regards,
Nadeesh TV

Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[nadeesh tv]

Hi ,
Sorry , my bad.  I misread 'plusmn'.

--
Thanks and Regards,
Nadeesh TV


On 8/19/2016 11:10 AM, nadeesh tv wrote:

Hi Stephen/Ivan,

Is not the the statement

"Zone offset minutes and seconds must be negative because hours is 
negative"


and the following doc definition contradicts ?

 358  * @param minutes  the time-zone offset in minutes, from 0 to 
±59
 359  * @param seconds  the time-zone offset in seconds, from 0 to 
±59





--
Thanks and Regards,
Nadeesh TV

Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[nadeesh tv]

Hi Stephen/Ivan,

Is not the the statement

"Zone offset minutes and seconds must be negative because hours is negative"

and the following doc definition contradicts ?

 358  * @param minutes  the time-zone offset in minutes, from 0 to 
±59
 359  * @param seconds  the time-zone offset in seconds, from 0 to 
±59


--
Thanks and Regards,
Nadeesh TV



On 8/18/2016 11:46 PM, Stephen Colebourne wrote:

Looks good
Stephen

On 18 August 2016 at 17:53, Ivan Gerasimov  wrote:

Hello everybody!

The factory methods of ZoneOffset class demonstrate a minor inconsistency.
Normally, invalid values of arguments are rejected (e.g. when minutes > 59),
but the value of Integer.MIN_VALUE is allowed to be passed in.

This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366
WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/

With kind regards,
Ivan

Testing hotspot's Atomic::cmpxchg(jbyte*) ?

[David Holmes]

No I didn't send this to the wrong mailing list :)

I need to test the jbyte version of Atomic::cmpxchg in the VM, but so 
far it doesn't seem to be the case that any library code will actually 
invoke it. At the top Java level there is no AtomicByte class. Unsafe 
defines compareAndSwapByte variants but implements them using 
compareAndSwapInt and isolating the byte of interest. VarHandles seems 
to have direct *SwapByte operations but I can't figure out what they map 
to - there are VM intrinsics but again I can't the actual implementation 
to see if it uses Atomic::cmpxchg(jbyte*).


So can someone (Aleksey?, Paul? :) ) tell me if this code does get used, 
and if not what I need to change to make it get used. And then what 
tests exist to exercise this?


Thanks,
David

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

I’ve updated the webrev to incorporate most of Paul’s suggestions.

http://cr.openjdk.java.net/~sdrach/8164389/webrev.01/index.html 



> On Aug 18, 2016, at 5:01 PM, Paul Sandoz  wrote:
> 
> 
>> On 18 Aug 2016, at 16:36, Steve Drach  wrote:
>> 
>> Hi,
>> 
>> Please review this rather simple fix to JarFileSystem.
>> 
>> issue: https://bugs.openjdk.java.net/browse/JDK-8164389 
>> 
>> webrev: http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html 
>> 
>> 
> 
> JarFileSystem
> —
> 
> 162 private void walk(IndexNode inode, Consumer process) {
> 163 if (inode == null) return;
> 164 if (inode.isDir())
> 165 walk(inode.child, process);
> 166 else
> 167 process.accept(inode);
> 168 walk(inode.sibling, process);
> 169 }
> 
> I would recommend using braces, even for single statement blocks.
> 
> 
> JFSTester
> —
> 
> You might wanna create a temporary jar file if possible, just in case the 
> test somehow fails to clean things up.
> 
> 
>  80 public void TestWalk() throws IOException {
> 
> Lower case for first letter.
> 
> Paul.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

>>> You might wanna create a temporary jar file if possible, just in case the 
>>> test somehow fails to clean things up.
>> 
>> Unsure how to do that, or perhaps I don’t understand your request.
> 
> I believe it’s possible to create a temporary directory, see 
> Files.createTempDirectory, and use that to place the test.jar, although it 
> might be more awkward to clean up.
> 
> Actually maybe it’s ok with regards to test execution if you can confirm the 
> “user.dir” is set up correctly with jtreg, i cannot recall.

It’s usually JTwork/scratch.  A lot of tests use user.dir.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Paul Sandoz]

> On 18 Aug 2016, at 17:15, Steve Drach  wrote:
> 
>> You might wanna create a temporary jar file if possible, just in case the 
>> test somehow fails to clean things up.
> 
> Unsure how to do that, or perhaps I don’t understand your request.

I believe it’s possible to create a temporary directory, see 
Files.createTempDirectory, and use that to place the test.jar, although it 
might be more awkward to clean up.

Actually maybe it’s ok with regards to test execution if you can confirm the 
“user.dir” is set up correctly with jtreg, i cannot recall.

Paul.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

> You might wanna create a temporary jar file if possible, just in case the 
> test somehow fails to clean things up.

Unsure how to do that, or perhaps I don’t understand your request.

Re: RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Paul Sandoz]

> On 18 Aug 2016, at 16:36, Steve Drach  wrote:
> 
> Hi,
> 
> Please review this rather simple fix to JarFileSystem.
> 
> issue: https://bugs.openjdk.java.net/browse/JDK-8164389 
> 
> webrev: http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html 
> 
> 

JarFileSystem
—

 162 private void walk(IndexNode inode, Consumer process) {
 163 if (inode == null) return;
 164 if (inode.isDir())
 165 walk(inode.child, process);
 166 else
 167 process.accept(inode);
 168 walk(inode.sibling, process);
 169 }

I would recommend using braces, even for single statement blocks.


JFSTester
—

You might wanna create a temporary jar file if possible, just in case the test 
somehow fails to clean things up.


  80 public void TestWalk() throws IOException {

Lower case for first letter.

Paul.

RFR: 8164389: jdk.nio.zipfs.JarFileSystem does not completely traverse the versioned entries in a multi-release jar file

[Steve Drach]

Hi,

Please review this rather simple fix to JarFileSystem.

issue: https://bugs.openjdk.java.net/browse/JDK-8164389 

webrev: http://cr.openjdk.java.net/~sdrach/8164389/webrev/index.html 


Thanks,
Steve

Re: RFR: JDK-8161230 - Create java.util.stream.Stream from Iterator / Enumeration

[Paul Sandoz]

> On 17 Aug 2016, at 02:39, Patrick Reinhart  wrote:
> 
> Hi Paul,
> 
> Thanks for the input. I integrated that into the version here:
> 
> http://cr.openjdk.java.net/~reinhapa/reviews/8161230/ClassLoader_StreamMethods.03
> 

Onward to the implementation!

Paul.

Re: Exceptional behavior of parallel stream

[Paul Sandoz]

> On 18 Aug 2016, at 03:54, Doug Lea  wrote:
> 
> On 08/17/2016 09:01 AM, Tagir F. Valeev wrote:
>> Hello!
>> 
>> I found no information in Stream API documentation on how it behaves
>> in case if exception occurs during the stream processing. While it's
>> quite evident for sequential stream (stream processing is terminated
>> and exception is propagated to the caller as is), the behavior for
>> parallel streams differs from one might expect. Consider the following
>> test:
> 
> In your example, you can witness the delayed termination of other threads
> upon exception in another because you add side-effecting operations
> (here, just printing). Avoiding all this would force sequential processing.
> 
> But if the supplied functions follow the documented properties,
> it should not matter that parallel processing of some elements continues
> when another hits an exception. Which is why nothing is said about it.
> Similar effects occur in findAny and other methods. I don't see any benefit
> in trying to specify exactly what happens in these cases.
> 

Short circuiting terminal operations such as findAny and findFirst will not 
return until the computation has “quiesced”, so once a result has been found 
the result will not be returned until all executing f/j tasks have completed. 
We consciously decided to do that so as the common pool would not contain 
straggling tasks beyond evaluation of the pipeline.

When an behavioural operation throws an exception the stream can return before 
all related f/j tasks have finished. I thought i had a good grasp of the 
CountedCompleter exception propagation behaviour vs. normal completion 
propagation behaviour, but i am not so sure now, since i cannot bias things to 
make the non-quiescent behaviour very obvious. I think it’s because the main 
non-worker thread tries to help out executing tasks before waiting.

Here is another reproducer:
try {
int s = 1024 * 16;
IntStream.range(0, s).parallel()
.peek(e -> {
if (e < s / 2) throw new RuntimeException();
BigInteger.probablePrime(256, ThreadLocalRandom.current());
})
.forEach(e -> System.out.println(Thread.currentThread().getId() + " 
" + e));
} catch (RuntimeException e) {
System.out.println("FAILED: " + ForkJoinPool.commonPool().isQuiescent());
}
Paul.

Re: [jdk9] RFR: 8163517: Various cleanup in java.io code

[Ivan Gerasimov]

On 18.08.2016 21:27, Claes Redestad wrote:
Rather than breaking into Reflection we should likely add an 
internally public API with a more well-defined contract, ensuring that 
it addresses arrays, primitives etc, and then Reflection, VerifyAccess 
and ObjectStream could consolidate their use to such a method.


Yes, totally agree with this.

Re: [jdk9] RFR: 8163517: Various cleanup in java.io code

[Claes Redestad]

On 2016-08-18 20:23, Ivan Gerasimov wrote:

Hi Claes!

Thank you for looking into this.


On 18.08.2016 15:25, Claes Redestad wrote:

On 2016-08-16 19:20, Ivan Gerasimov wrote:

ObjectStreamClass:

I wonder if the entire getPackageName is replaceable with
Class.getPackageName()? If so, ditch the method completely and use
Class.getPackageName everywhere. This may complicate backporting to 
JDK

8 though.


Unfortunately, Class.getPackageName() cannot handle arrays, so I'll 
leave it as is. 


There are a number of utility methods around that could be reused,
e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both 
arrays and
primitive types and could replace both packageEquals and 
getPackageName in
ObjectStreamClass (which already use j.i.r.Reflection for other 
things). Making
isSameClassPackage in Reflection public should be non-controversial, 
I hope.




Yes, thanks for the pointer!
That's right, there are several similar methods in different packages.
I see that the third option is 
sun.invoke.util.VerifyAccess.isSamePackage / getPackageName, but it 
also has its nuances.


I guess that keeping ObjectStreamClass.getPackageName() separated 
would be the simplest approach: it minimizes the chances of regression 
and doesn't make it harder to do future modifications to 
Reflection.isSameClassPackage, if it is ever needed.


If there are no objections, I'd like to push the latest variant of the 
fix, which is at

http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/


Go ahead, I'm just musing about the opportunity to consolidate further, 
which is better kept as a follow-up anyhow.


Rather than breaking into Reflection we should likely add an internally 
public API with a more well-defined contract, ensuring that it addresses 
arrays, primitives etc, and then Reflection, VerifyAccess and 
ObjectStream could consolidate their use to such a method.


Thanks!

/Claes

Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[Ivan Gerasimov]

Thank you Stephen for review!


On 18.08.2016 21:16, Stephen Colebourne wrote:

Looks good
Stephen

On 18 August 2016 at 17:53, Ivan Gerasimov  wrote:

Hello everybody!

The factory methods of ZoneOffset class demonstrate a minor inconsistency.
Normally, invalid values of arguments are rejected (e.g. when minutes > 59),
but the value of Integer.MIN_VALUE is allowed to be passed in.

This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366
WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/

With kind regards,
Ivan

Re: [jdk9] RFR: 8163517: Various cleanup in java.io code

[Ivan Gerasimov]

Hi Claes!

Thank you for looking into this.


On 18.08.2016 15:25, Claes Redestad wrote:

On 2016-08-16 19:20, Ivan Gerasimov wrote:

ObjectStreamClass:

I wonder if the entire getPackageName is replaceable with
Class.getPackageName()? If so, ditch the method completely and use
Class.getPackageName everywhere. This may complicate backporting to JDK
8 though.


Unfortunately, Class.getPackageName() cannot handle arrays, so I'll 
leave it as is. 


There are a number of utility methods around that could be reused,
e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both 
arrays and
primitive types and could replace both packageEquals and 
getPackageName in
ObjectStreamClass (which already use j.i.r.Reflection for other 
things). Making
isSameClassPackage in Reflection public should be non-controversial, I 
hope.




Yes, thanks for the pointer!
That's right, there are several similar methods in different packages.
I see that the third option is 
sun.invoke.util.VerifyAccess.isSamePackage / getPackageName, but it also 
has its nuances.


I guess that keeping ObjectStreamClass.getPackageName() separated would 
be the simplest approach: it minimizes the chances of regression and 
doesn't make it harder to do future modifications to 
Reflection.isSameClassPackage, if it is ever needed.


If there are no objections, I'd like to push the latest variant of the 
fix, which is at

http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/

With kind regards,
Ivan

Re: RFR 8138661, Address DataSource javadoc typo

[Naoto Sato]

Looks good, Lance.

Naoto

On 8/18/16 11:08 AM, Lance Andersen wrote:

Hi all,

This is a trivial fix of a typo in DataSource

-

ljanders-mac:jdk ljanders$ hg diff
diff -r 6f5da13861db src/java.sql/share/classes/javax/sql/DataSource.java
--- a/src/java.sql/share/classes/javax/sql/DataSource.java  Thu Aug 18 
16:27:15 2016 +0300
+++ b/src/java.sql/share/classes/javax/sql/DataSource.java  Thu Aug 18 
14:05:10 2016 -0400
@@ -64,7 +64,7 @@
  * 
  * A driver that is accessed via a {@code DataSource} object does not
  * register itself with the {@code DriverManager}.  Rather, a
- * {@code DataSource} object is retrieved though a lookup operation
+ * {@code DataSource} object is retrieved through a lookup operation
  * and then used to create a {@code Connection} object.  With a basic
  * implementation, the connection obtained through a {@code DataSource}
  * object is identical to a connection obtained through the
ljanders-mac:jdk ljanders$

-

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 

Re: [jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[Stephen Colebourne]

Looks good
Stephen

On 18 August 2016 at 17:53, Ivan Gerasimov  wrote:
> Hello everybody!
>
> The factory methods of ZoneOffset class demonstrate a minor inconsistency.
> Normally, invalid values of arguments are rejected (e.g. when minutes > 59),
> but the value of Integer.MIN_VALUE is allowed to be passed in.
>
> This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well.
>
> Would you please help review the fix?
>
> BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366
> WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/
>
> With kind regards,
> Ivan
>

RFR 8138661, Address DataSource javadoc typo

[Lance Andersen]

Hi all,

This is a trivial fix of a typo in DataSource

-

ljanders-mac:jdk ljanders$ hg diff
diff -r 6f5da13861db src/java.sql/share/classes/javax/sql/DataSource.java
--- a/src/java.sql/share/classes/javax/sql/DataSource.java  Thu Aug 18 
16:27:15 2016 +0300
+++ b/src/java.sql/share/classes/javax/sql/DataSource.java  Thu Aug 18 
14:05:10 2016 -0400
@@ -64,7 +64,7 @@
  * 
  * A driver that is accessed via a {@code DataSource} object does not
  * register itself with the {@code DriverManager}.  Rather, a
- * {@code DataSource} object is retrieved though a lookup operation
+ * {@code DataSource} object is retrieved through a lookup operation
  * and then used to create a {@code Connection} object.  With a basic
  * implementation, the connection obtained through a {@code DataSource}
  * object is identical to a connection obtained through the
ljanders-mac:jdk ljanders$

-

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 

[jdk9] RFR: 8164366: ZoneOffset.ofHoursMinutesSeconds() does not reject invalid input

[Ivan Gerasimov]

Hello everybody!

The factory methods of ZoneOffset class demonstrate a minor inconsistency.
Normally, invalid values of arguments are rejected (e.g. when minutes > 
59), but the value of Integer.MIN_VALUE is allowed to be passed in.


This is due to using Math.abs(), which cannot handle Integer.MIN_VALUE well.

Would you please help review the fix?

BUGURL: https://bugs.openjdk.java.net/browse/JDK-8164366
WEBREV: http://cr.openjdk.java.net/~igerasim/8164366/00/webrev/

With kind regards,
Ivan

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Claes Redestad]

On 2016-08-18 18:11, Aleksey Shipilev wrote:

On 08/18/2016 05:32 PM, Claes Redestad wrote:

http://cr.openjdk.java.net/~redestad/8164044/webrev.03/

Doing this twice now?

+dedupSet.add(methodTypes[i]);


Oops. Fixed and updated in-place.



Otherwise thumbs up.


Thanks!

/Claes

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Aleksey Shipilev]

On 08/18/2016 05:32 PM, Claes Redestad wrote:
> http://cr.openjdk.java.net/~redestad/8164044/webrev.03/

Doing this twice now?

+dedupSet.add(methodTypes[i]);

Otherwise thumbs up.

-Aleksey

Re: RFR: JDK-8164326: jrtfsviewer.js and jrtls.js does not work

[Sundararajan Athijegannathan]

+1


On 8/18/2016 8:48 PM, Yasumasa Suenaga wrote:
> Hi all,
>
> This review request is related to [1].
>
> jrtfsviewer.js and jrtls.js are not contained JDK/JRE binaries.
> However, these tool might be used by JDK developers. So I think we
> should fix this issue.
>
> I uploaded a webrev. Could you review it?
>
>   http://cr.openjdk.java.net/~ysuenaga/JDK-8164326/webrev.00/
>
>
> Thanks,
>
> Yasumasa
>
>
> [1]
> http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042995.html

Re: RFR: JDK-8164326: jrtfsviewer.js and jrtls.js does not work

[Xueming Shen]

+1

On 8/18/16 8:18 AM, Yasumasa Suenaga wrote:

Hi all,

This review request is related to [1].

jrtfsviewer.js and jrtls.js are not contained JDK/JRE binaries.
However, these tool might be used by JDK developers. So I think we 
should fix this issue.


I uploaded a webrev. Could you review it?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164326/webrev.00/


Thanks,

Yasumasa


[1] 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042995.html

RFR: JDK-8164326: jrtfsviewer.js and jrtls.js does not work

[Yasumasa Suenaga]

Hi all,

This review request is related to [1].

jrtfsviewer.js and jrtls.js are not contained JDK/JRE binaries.
However, these tool might be used by JDK developers. So I think we should fix 
this issue.

I uploaded a webrev. Could you review it?

  http://cr.openjdk.java.net/~ysuenaga/JDK-8164326/webrev.00/


Thanks,

Yasumasa


[1] http://mail.openjdk.java.net/pipermail/core-libs-dev/2016-August/042995.html

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Claes Redestad]

On 2016-08-18 16:06, Aleksey Shipilev wrote:

On 08/18/2016 12:33 AM, Claes Redestad wrote:

Hi,

please review this change which adds pre-generation of simple
DelegatingMethodHandles corresponding to the DirectMethodHandles we
already pre-generate during linking.

webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/

*) This block:

   if (!dedupSet.contains(methodTypes[i])) {
  // do stuff
  dedupSet.add(methodTypes[i]);
   }

Is replaceable by:

   if (dedupSet.add(methodTypes[i])) {
  // do stuff
   }


Sure!



*) Can save instantiation here, by moving ptypes into else branch:

  333 Class[] ptypes = new Class[parameters.length()];
  334 if (ptypes.length == 0) {
  335 return MethodType.methodType(rtype);
  336 } else {
  337 for (int i = 0; i < ptypes.length; i++) {
  338 ptypes[i] = simpleType(parameters.charAt(i));
  339 }
  340 return MethodType.methodType(rtype, ptypes);
  341 }
  342 }


Efficiency of code only called at link time isn't too important,
but it also looks cleaner: fixed.



*) Many whitespaces from this switch:

195 switch (which) {
  196 case LF_INVVIRTUAL:linkerName = "linkToVirtual";   kind
= DIRECT_INVOKE_VIRTUAL; break;
  197 case LF_INVSTATIC: linkerName = "linkToStatic";kind
= DIRECT_INVOKE_STATIC;  break;
  198 case LF_INVSTATIC_INIT:linkerName = "linkToStatic";kind
= DIRECT_INVOKE_STATIC_INIT; break;
  199 case LF_INVSPECIAL:linkerName = "linkToSpecial";   kind
= DIRECT_INVOKE_SPECIAL; break;
  200 case LF_INVINTERFACE:  linkerName = "linkToInterface"; kind
= DIRECT_INVOKE_INTERFACE;   break;
  201 case LF_NEWINVSPECIAL: linkerName = "linkToSpecial";   kind
= DIRECT_NEW_INVOKE_SPECIAL; break;
  202 default:  throw new InternalError("which="+which);

...ran away to this one:

  154 private static Kind whichKind(int whichCache) {
  155 switch(whichCache) {
  156 case MethodTypeForm.LF_REBIND:return
BOUND_REINVOKER;
  157 case MethodTypeForm.LF_DELEGATE:  return DELEGATE;
  158 default:  return REINVOKER;
  159 }
  160 }


I assume you want less whitespace?

http://cr.openjdk.java.net/~redestad/8164044/webrev.03/

Thanks!

/Claes



Thanks,
-Aleksey

Re: Error at jrt*.js

[Sundararajan Athijegannathan]

Yes, those scripts are meant for developers and not part of binary JDK
bits. Please do file a bug and send webrev for review.

-Sundar


On 8/18/2016 7:51 PM, Yasumasa Suenaga wrote:
> Hi all,
>
> I tried to run jrtfsviewer.js and jrtls.js . But they did not work as
> below:
> 
> $ jjs -fx jrtfsviewer.js
> Exception in Application start method
> Exception in thread "main" java.lang.RuntimeException: Exception in
> Application start method
> at
> com.sun.javafx.application.LauncherImpl.launchApplication1(javafx.graphics@9-ea/LauncherImpl.java:897)
> at
> com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(javafx.graphics@9-ea/LauncherImpl.java:188)
> at java.lang.Thread.run(java.base@9-ea/Thread.java:843)
> Caused by: java.lang.NullPointerException
> at
> java.util.Objects.requireNonNull(java.base@9-ea/Objects.java:221)
> at
> jdk.internal.jrtfs.JrtFileSystemProvider.newFileSystem(java.base@9-ea/JrtFileSystemProvider.java:104)
> at
> java.nio.file.FileSystems.newFileSystem(java.base@9-ea/FileSystems.java:342)
> at
> jdk.nashorn.internal.scripts.Script$Recompilation$22$3116$jrtfsviewer$cu1$restOf.getJrtFileSystem(jdk.scripting.nashorn.scripts/jrtfsviewer.js:103)
> at
> jdk.nashorn.internal.scripts.Script$Recompilation$17$3748A$jrtfsviewer.start(jdk.scripting.nashorn.scripts/jrtfsviewer.js:109)
> at
> jdk.nashorn.internal.scripts.Script$Recompilation$6$839A$\=fx\!bootstrap$cu1$restOf.start(jdk.scripting.nashorn.scripts/fx:bootstrap.js:57)
> at
> jdk.nashorn.javaadapters.javafx_application_Application.start(Unknown
> Source)
> at
> com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$9(javafx.graphics@9-ea/LauncherImpl.java:843)
> at
> com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(javafx.graphics@9-ea/PlatformImpl.java:452)
> at
> com.sun.javafx.application.PlatformImpl.lambda$runLater$10(javafx.graphics@9-ea/PlatformImpl.java:421)
> at
> java.security.AccessController.doPrivileged(java.base@9-ea/Native Method)
> at
> com.sun.javafx.application.PlatformImpl.lambda$runLater$11(javafx.graphics@9-ea/PlatformImpl.java:420)
> at
> com.sun.glass.ui.InvokeLaterDispatcher$Future.run(javafx.graphics@9-ea/InvokeLaterDispatcher.java:96)
> at
> com.sun.glass.ui.win.WinApplication._runLoop(javafx.graphics@9-ea/Native
> Method)
> at
> com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(javafx.graphics@9-ea/WinApplication.java:189)
> ... 1 more
> 
> $ jjs jrtls.js
> jrtls.js:37:1 Expected an operand but found *
>  */
>  ^
> 
>
> These scripts are not in JDK. I guess they are used for checking
> JDK/JRE modules by JDK developers, and they are no longer used.
> So I'm not sure whether they should be fixed.
>
> I think they can be fixed as below:
> 
> diff -r b60dcba6b4f9
> src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js
> --- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js   
> Tue Aug 16 09:57:50 2016 +0200
> +++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js   
> Wed Aug 18 20:41:25 2016 +0900
> @@ -53,6 +53,7 @@
>  var Files = Java.type("java.nio.file.Files");
>  var System = Java.type("java.lang.System");
>  var URI = Java.type("java.net.URI");
> +var Collections = Java.type("java.util.Collections");
>  
>  // JavaFX classes used
>  var StackPane = Java.type("javafx.scene.layout.StackPane");
> @@ -100,7 +101,7 @@
>  print("did you miss specifying jrt-fs.jar with -cp
> option?");
>  usage();
>  }
> -return FileSystems.newFileSystem(uri, null, cls.classLoader);
> +return FileSystems.newFileSystem(uri, Collections.emptyMap(),
> cls.classLoader);
>  }
>  }
>  
> diff -r b60dcba6b4f9
> src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js
> --- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.jsTue
> Aug 16 09:57:50 2016 +0200
> +++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.jsWed
> Aug 18 20:41:25 2016 +0900
> @@ -34,7 +34,6 @@
>   * but also compiled and delivered as part of the jrtfs.jar to
> support access
>   * to the jimage file provided by the shipped JDK by tools running on
> JDK 8.
>   */
> - */
>  
>  // classes used
>  var Files = Java.type("java.nio.file.Files");
> 
>
> If this fix should be merged, I'll file it to JBS and upload webrev.
> What do you think about it?
>
>
> Thanks,
>
> Yasumasa
>

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Claes Redestad]

Michael, Vladimir,

thanks for reviewing and suggesting improvements.

Getting rid of LF.debugName seems like a rather straight-forward and 
worthwhile cleanup at this point.


/Claes

On 2016-08-18 16:13, Vladimir Ivanov wrote:

Good work! Reviewed.

Best regards,
Vladimir Ivanov

PS: I wish LF.debugString completely went away, but let's keep it for 
future cleanup.


On 8/18/16 12:33 AM, Claes Redestad wrote:

Hi,

please review this change which adds pre-generation of simple
DelegatingMethodHandles corresponding to the DirectMethodHandles we
already pre-generate during linking.

webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8164044

This also includes some cleanup suggested by Vladimir during internal
review, such as:

- adding an enum to control this behavior, which allows removing the
special version of LF.compileToBytecode introduced by JDK-8163369
- refactored resolution of pregenerated code to the
InvokerBytecodeGenerator
- removing reliance on the LF.debugName (which we have some loose ideas
to remove from LF and tuck away in a map we only create and initialize
when actually debugging).

This patch removes 11 out of the 39 classes generated on first use of
the StringConcatFactory in a simple test, amounting to a ~10ms speedup
on my machine.

Thanks!

/Claes

Error at jrt*.js

[Yasumasa Suenaga]

Hi all,

I tried to run jrtfsviewer.js and jrtls.js . But they did not work as below:

$ jjs -fx jrtfsviewer.js
Exception in Application start method
Exception in thread "main" java.lang.RuntimeException: Exception in Application 
start method
at 
com.sun.javafx.application.LauncherImpl.launchApplication1(javafx.graphics@9-ea/LauncherImpl.java:897)
at 
com.sun.javafx.application.LauncherImpl.lambda$launchApplication$2(javafx.graphics@9-ea/LauncherImpl.java:188)
at java.lang.Thread.run(java.base@9-ea/Thread.java:843)
Caused by: java.lang.NullPointerException
at java.util.Objects.requireNonNull(java.base@9-ea/Objects.java:221)
at 
jdk.internal.jrtfs.JrtFileSystemProvider.newFileSystem(java.base@9-ea/JrtFileSystemProvider.java:104)
at 
java.nio.file.FileSystems.newFileSystem(java.base@9-ea/FileSystems.java:342)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$22$3116$jrtfsviewer$cu1$restOf.getJrtFileSystem(jdk.scripting.nashorn.scripts/jrtfsviewer.js:103)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$17$3748A$jrtfsviewer.start(jdk.scripting.nashorn.scripts/jrtfsviewer.js:109)
at 
jdk.nashorn.internal.scripts.Script$Recompilation$6$839A$\=fx\!bootstrap$cu1$restOf.start(jdk.scripting.nashorn.scripts/fx:bootstrap.js:57)
at 
jdk.nashorn.javaadapters.javafx_application_Application.start(Unknown Source)
at 
com.sun.javafx.application.LauncherImpl.lambda$launchApplication1$9(javafx.graphics@9-ea/LauncherImpl.java:843)
at 
com.sun.javafx.application.PlatformImpl.lambda$runAndWait$12(javafx.graphics@9-ea/PlatformImpl.java:452)
at 
com.sun.javafx.application.PlatformImpl.lambda$runLater$10(javafx.graphics@9-ea/PlatformImpl.java:421)
at java.security.AccessController.doPrivileged(java.base@9-ea/Native 
Method)
at 
com.sun.javafx.application.PlatformImpl.lambda$runLater$11(javafx.graphics@9-ea/PlatformImpl.java:420)
at 
com.sun.glass.ui.InvokeLaterDispatcher$Future.run(javafx.graphics@9-ea/InvokeLaterDispatcher.java:96)
at 
com.sun.glass.ui.win.WinApplication._runLoop(javafx.graphics@9-ea/Native Method)
at 
com.sun.glass.ui.win.WinApplication.lambda$runLoop$3(javafx.graphics@9-ea/WinApplication.java:189)
... 1 more

$ jjs jrtls.js
jrtls.js:37:1 Expected an operand but found *
 */
 ^


These scripts are not in JDK. I guess they are used for checking JDK/JRE 
modules by JDK developers, and they are no longer used.
So I'm not sure whether they should be fixed.

I think they can be fixed as below:

diff -r b60dcba6b4f9 
src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js
--- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js Tue Aug 
16 09:57:50 2016 +0200
+++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtfsviewer.js Wed Aug 
18 20:41:25 2016 +0900
@@ -53,6 +53,7 @@
 var Files = Java.type("java.nio.file.Files");
 var System = Java.type("java.lang.System");
 var URI = Java.type("java.net.URI");
+var Collections = Java.type("java.util.Collections");
 
 // JavaFX classes used

 var StackPane = Java.type("javafx.scene.layout.StackPane");
@@ -100,7 +101,7 @@
 print("did you miss specifying jrt-fs.jar with -cp option?");
 usage();
 }
-return FileSystems.newFileSystem(uri, null, cls.classLoader);
+return FileSystems.newFileSystem(uri, Collections.emptyMap(), 
cls.classLoader);
 }
 }
 
diff -r b60dcba6b4f9 src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js

--- a/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js   Tue Aug 16 
09:57:50 2016 +0200
+++ b/src/java.base/share/classes/jdk/internal/jrtfs/jrtls.js   Wed Aug 18 
20:41:25 2016 +0900
@@ -34,7 +34,6 @@
  * but also compiled and delivered as part of the jrtfs.jar to support access
  * to the jimage file provided by the shipped JDK by tools running on JDK 8.
  */
- */
 
 // classes used

 var Files = Java.type("java.nio.file.Files");


If this fix should be merged, I'll file it to JBS and upload webrev.
What do you think about it?


Thanks,

Yasumasa

Re: [jdk9] RFR: 8163517: Various cleanup in java.io code

[Aleksey Shipilev]

On 08/16/2016 08:20 PM, Ivan Gerasimov wrote:
> Here is the updated webrev:
> http://cr.openjdk.java.net/~igerasim/8163517/01/webrev/

Looks good to me.

-Aleksey

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Vladimir Ivanov]

Good work! Reviewed.

Best regards,
Vladimir Ivanov

PS: I wish LF.debugString completely went away, but let's keep it for 
future cleanup.


On 8/18/16 12:33 AM, Claes Redestad wrote:

Hi,

please review this change which adds pre-generation of simple
DelegatingMethodHandles corresponding to the DirectMethodHandles we
already pre-generate during linking.

webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/
bug: https://bugs.openjdk.java.net/browse/JDK-8164044

This also includes some cleanup suggested by Vladimir during internal
review, such as:

- adding an enum to control this behavior, which allows removing the
special version of LF.compileToBytecode introduced by JDK-8163369
- refactored resolution of pregenerated code to the
InvokerBytecodeGenerator
- removing reliance on the LF.debugName (which we have some loose ideas
to remove from LF and tuck away in a map we only create and initialize
when actually debugging).

This patch removes 11 out of the 39 classes generated on first use of
the StringConcatFactory in a simple test, amounting to a ~10ms speedup
on my machine.

Thanks!

/Claes

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Aleksey Shipilev]

On 08/18/2016 12:33 AM, Claes Redestad wrote:
> Hi,
> 
> please review this change which adds pre-generation of simple
> DelegatingMethodHandles corresponding to the DirectMethodHandles we
> already pre-generate during linking.
> 
> webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/

*) This block:

  if (!dedupSet.contains(methodTypes[i])) {
 // do stuff
 dedupSet.add(methodTypes[i]);
  }

Is replaceable by:

  if (dedupSet.add(methodTypes[i])) {
 // do stuff
  }

*) Can save instantiation here, by moving ptypes into else branch:

 333 Class[] ptypes = new Class[parameters.length()];
 334 if (ptypes.length == 0) {
 335 return MethodType.methodType(rtype);
 336 } else {
 337 for (int i = 0; i < ptypes.length; i++) {
 338 ptypes[i] = simpleType(parameters.charAt(i));
 339 }
 340 return MethodType.methodType(rtype, ptypes);
 341 }
 342 }

*) Many whitespaces from this switch:

195 switch (which) {
 196 case LF_INVVIRTUAL:linkerName = "linkToVirtual";   kind
= DIRECT_INVOKE_VIRTUAL; break;
 197 case LF_INVSTATIC: linkerName = "linkToStatic";kind
= DIRECT_INVOKE_STATIC;  break;
 198 case LF_INVSTATIC_INIT:linkerName = "linkToStatic";kind
= DIRECT_INVOKE_STATIC_INIT; break;
 199 case LF_INVSPECIAL:linkerName = "linkToSpecial";   kind
= DIRECT_INVOKE_SPECIAL; break;
 200 case LF_INVINTERFACE:  linkerName = "linkToInterface"; kind
= DIRECT_INVOKE_INTERFACE;   break;
 201 case LF_NEWINVSPECIAL: linkerName = "linkToSpecial";   kind
= DIRECT_NEW_INVOKE_SPECIAL; break;
 202 default:  throw new InternalError("which="+which);

...ran away to this one:

 154 private static Kind whichKind(int whichCache) {
 155 switch(whichCache) {
 156 case MethodTypeForm.LF_REBIND:return
BOUND_REINVOKER;
 157 case MethodTypeForm.LF_DELEGATE:  return DELEGATE;
 158 default:  return REINVOKER;
 159 }
 160 }

Thanks,
-Aleksey

Re: RFR: 8164044: Generate corresponding simple DelegatingMethodHandles when generating a DirectMethodHandle

[Michael Haupt]

Hi Claes,

thumbs up.

Best,

Michael

> Am 17.08.2016 um 23:33 schrieb Claes Redestad :
> 
> Hi,
> 
> please review this change which adds pre-generation of simple 
> DelegatingMethodHandles corresponding to the DirectMethodHandles we already 
> pre-generate during linking.
> 
> webrev: http://cr.openjdk.java.net/~redestad/8164044/webrev.02/
> bug: https://bugs.openjdk.java.net/browse/JDK-8164044
> 
> This also includes some cleanup suggested by Vladimir during internal review, 
> such as:
> 
> - adding an enum to control this behavior, which allows removing the special 
> version of LF.compileToBytecode introduced by JDK-8163369
> - refactored resolution of pregenerated code to the InvokerBytecodeGenerator
> - removing reliance on the LF.debugName (which we have some loose ideas to 
> remove from LF and tuck away in a map we only create and initialize when 
> actually debugging).
> 
> This patch removes 11 out of the 39 classes generated on first use of the 
> StringConcatFactory in a simple test, amounting to a ~10ms speedup on my 
> machine.
> 
> Thanks!
> 
> /Claes

-- 

 
Dr. Michael Haupt | Principal Member of Technical Staff
Phone: +49 331 200 7277 | Fax: +49 331 200 7561
Oracle Java Platform Group | LangTools Team | Nashorn
Oracle Deutschland B.V. & Co. KG | Schiffbauergasse 14 | 14467 Potsdam, Germany

ORACLE Deutschland B.V. & Co. KG | Hauptverwaltung: Riesstraße 25, D-80992 
München
Registergericht: Amtsgericht München, HRA 95603

Komplementärin: ORACLE Deutschland Verwaltung B.V. | Hertogswetering 163/167, 
3543 AS Utrecht, Niederlande
Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697
Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
  Oracle is committed to developing 
practices and products that help protect the environment

Re: [jdk9] RFR: 8163517: Various cleanup in java.io code

[Claes Redestad]

On 2016-08-16 19:20, Ivan Gerasimov wrote:

ObjectStreamClass:

I wonder if the entire getPackageName is replaceable with
Class.getPackageName()? If so, ditch the method completely and use
Class.getPackageName everywhere. This may complicate backporting to JDK
8 though.


Unfortunately, Class.getPackageName() cannot handle arrays, so I'll 
leave it as is. 


There are a number of utility methods around that could be reused,
e.g., jdk.internal.reflect.Reflection.isSameClassPackage handles both 
arrays and

primitive types and could replace both packageEquals and getPackageName in
ObjectStreamClass (which already use j.i.r.Reflection for other things). 
Making

isSameClassPackage in Reflection public should be non-controversial, I hope.

Thanks!

/Claes

Re: Exceptional behavior of parallel stream

[Tagir F. Valeev]

Hello, Doug!

Thank you for your response.

DL> In your example, you can witness the delayed termination of other threads
DL> upon exception in another because you add side-effecting operations
DL> (here, just printing). Avoiding all this would force sequential processing.

DL> But if the supplied functions follow the documented properties,
DL> it should not matter that parallel processing of some elements continues
DL> when another hits an exception. Which is why nothing is said about it.
DL> Similar effects occur in findAny and other methods. I don't see any benefit
DL> in trying to specify exactly what happens in these cases.

Note that functions supplied to peek, forEach and forEachOrdered
methods, according to the documentation are allowed to produce
side-effect (otherwise these operations would be completely useless).
Thus my example does not violate the documented way of using streams.
While you may argue that the "peek" is intended for debugging use,
it's still possible to show the same effect using forEach. Thus I
don't agree that using Streams in documented way you cannot hit this
problem. Of course using forEach is usually discouraged, but it's
still legal API method.

With best regards,
Tagir Valeev.

DL> -Doug

>>
>> String[] data = IntStream.range(0, 100).mapToObj(String::valueOf)
>>  .toArray(String[]::new);
>> data[20] = "oops";
>> try {
>> int sum = Arrays.stream(data).parallel().mapToInt(Integer::valueOf)
>> .peek(System.out::println).sum();
>> System.out.println("Sum is "+sum);
>> } catch (NumberFormatException e) {
>> System.out.println("Non-number appeared!");
>> }
>>
>> This parses integers stored in string array and sums them outputting
>> every number to stdout once it processed. As data set contains
>> non-number, the stream obviously fails with NumberFormatException. The
>> typical output looks like this:
>>
>> 62
>> 63
>> 12
>> 31
>> 87
>> ...
>> 28
>> 92
>> 29
>> 8
>> Non-number appeared!
>> 9
>> 30
>>
>> So as you can see, the stream is not actually terminated when
>> exception is thrown and caught: even after that some parallel tasks
>> continue running, and you see more numbers printed after catch block
>> is executed.
>>
>> I consider such behavior as confusing and unexpected. Given the fact
>> that stream may produce side-effects (e.g. if terminal op is forEach)
>> this might lead to unforeseen consequences in user programs as
>> left-over parallel stream tasks may continue mutate shared state after
>> main stream thread exceptionally returns the control to the caller.
>>
>> So I suggest that such behavior should be either fixed or explicitly
>> documented. What do you think?
>>
>> With best regards,
>> Tagir Valeev.
>>
>>

Re: Exceptional behavior of parallel stream

[Doug Lea]

On 08/17/2016 09:01 AM, Tagir F. Valeev wrote:

Hello!

I found no information in Stream API documentation on how it behaves
in case if exception occurs during the stream processing. While it's
quite evident for sequential stream (stream processing is terminated
and exception is propagated to the caller as is), the behavior for
parallel streams differs from one might expect. Consider the following
test:


In your example, you can witness the delayed termination of other threads
upon exception in another because you add side-effecting operations
(here, just printing). Avoiding all this would force sequential processing.

But if the supplied functions follow the documented properties,
it should not matter that parallel processing of some elements continues
when another hits an exception. Which is why nothing is said about it.
Similar effects occur in findAny and other methods. I don't see any benefit
in trying to specify exactly what happens in these cases.

-Doug



String[] data = IntStream.range(0, 100).mapToObj(String::valueOf)
 .toArray(String[]::new);
data[20] = "oops";
try {
int sum = Arrays.stream(data).parallel().mapToInt(Integer::valueOf)
.peek(System.out::println).sum();
System.out.println("Sum is "+sum);
} catch (NumberFormatException e) {
System.out.println("Non-number appeared!");
}

This parses integers stored in string array and sums them outputting
every number to stdout once it processed. As data set contains
non-number, the stream obviously fails with NumberFormatException. The
typical output looks like this:

62
63
12
31
87
...
28
92
29
8
Non-number appeared!
9
30

So as you can see, the stream is not actually terminated when
exception is thrown and caught: even after that some parallel tasks
continue running, and you see more numbers printed after catch block
is executed.

I consider such behavior as confusing and unexpected. Given the fact
that stream may produce side-effects (e.g. if terminal op is forEach)
this might lead to unforeseen consequences in user programs as
left-over parallel stream tasks may continue mutate shared state after
main stream thread exceptionally returns the control to the caller.

So I suggest that such behavior should be either fixed or explicitly
documented. What do you think?

With best regards,
Tagir Valeev.

Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Aleks Efimov]

Joe, Christoph,

Thanks for your reviews!

With Best Regards,

Aleksej


On 18/08/16 08:47, Langer, Christoph wrote:

+1, nice fix.


-Original Message-
From: core-libs-dev [mailto:core-libs-dev-boun...@openjdk.java.net] On Behalf
Of Joe Wang
Sent: Mittwoch, 17. August 2016 18:30
To: Aleks Efimov 
Cc: core-libs-dev 
Subject: Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static
final Exceptions

Looks good, Aleksej. Thanks for coming up with a well-thought solution
to get rid of the static RE field.

Best,
Joe

On 8/17/16, 7:04 AM, Aleks Efimov wrote:

Hi Joe, Aleksey,
Thank you for reviewing the initial fix.

I followed the Joe's suggestion (thanks for that) and removed static
'abort' field completely, the functionality was replaced by throwing
the exception of newly added type - AbortException. The new webrev
with removed 'abort' can be found here:
http://cr.openjdk.java.net/~aefimov/8146961/9/01

The Tomcat reproducer attached to the bug report fails without the fix
and passes with the new version of the fix.
The JPRT and JCK testing again shows no related jaxp tests failures.

Best Regards,
Aleksej


On 15/08/16 21:09, Joe Wang wrote:

Hi Aleksej,

I suggest we get rid of the static abort. If RuntimeException
happens, check where it happens. The first use case looks suspicious
as it just returns if it's an instance of RE. For the 2nd case in DOM
error report, let's throw a RuntimeException with the specified error
message if error happens, and there's no handler or the handler
failed to handle it. (would be better than an empty RE)

Best,
Joe

On 8/15/16, 10:38 AM, Aleks Efimov wrote:

Hi,

Please, help to review the fix for memory leak [1] in
com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused
by usage of static final exceptions.
This problem was already fixed in Apache Xerces project [2] and I
would like to backport it to JDK9 Xerces implementation.
Webrev with the changes:
http://cr.openjdk.java.net/~aefimov/8146961/9/00

The Tomcat reproducer attached to the bug report fails without the
fix and passes with the fix.
The JPRT and JCK testing shows no jaxp tests failures with the
proposed fix.

With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8146961
[2] https://issues.apache.org/jira/browse/XERCESJ-1667

Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Aleks Efimov]

Hi Peter,

Thank you for review and suggestion to add super constructor call: will 
add it before pushing the changes.


Best Regards,

Aleksej


On 18/08/16 11:08, Peter Levart wrote:

Hi Aleks,

Looks OK, but if AbortException is never inspected for a stack trace, 
then it could be constructed without it. This is perhaps unnecessary 
if it is not on the hot path, but it is easy to just call the 
appropriate super constructor:


public class AbortException extends RuntimeException {

private static final long serialVersionUID = 
2608302175475740417L;


/**
 * Constructor AbortException
 */
public AbortException() { super(null, null, false, false); }
}


Regards, Peter

On 08/17/2016 04:04 PM, Aleks Efimov wrote:

Hi Joe, Aleksey,
Thank you for reviewing the initial fix.

I followed the Joe's suggestion (thanks for that) and removed static 
'abort' field completely, the functionality was replaced by throwing 
the exception of newly added type - AbortException. The new webrev 
with removed 'abort' can be found here:

http://cr.openjdk.java.net/~aefimov/8146961/9/01

The Tomcat reproducer attached to the bug report fails without the 
fix and passes with the new version of the fix.

The JPRT and JCK testing again shows no related jaxp tests failures.

Best Regards,
Aleksej


On 15/08/16 21:09, Joe Wang wrote:

Hi Aleksej,

I suggest we get rid of the static abort. If RuntimeException 
happens, check where it happens. The first use case looks suspicious 
as it just returns if it's an instance of RE. For the 2nd case in 
DOM error report, let's throw a RuntimeException with the specified 
error message if error happens, and there's no handler or the 
handler failed to handle it. (would be better than an empty RE)


Best,
Joe

On 8/15/16, 10:38 AM, Aleks Efimov wrote:

Hi,

Please, help to review the fix for memory leak [1] in 
com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused 
by usage of static final exceptions.
This problem was already fixed in Apache Xerces project [2] and I 
would like to backport it to JDK9 Xerces implementation.

Webrev with the changes:
http://cr.openjdk.java.net/~aefimov/8146961/9/00

The Tomcat reproducer attached to the bug report fails without the 
fix and passes with the fix.
The JPRT and JCK testing shows no jaxp tests failures with the 
proposed fix.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8146961
[2] https://issues.apache.org/jira/browse/XERCESJ-1667

Re: RFR (JAXP): 8146961: Fix PermGen memory leaks caused by static final Exceptions

[Peter Levart]

Hi Aleks,

Looks OK, but if AbortException is never inspected for a stack trace, 
then it could be constructed without it. This is perhaps unnecessary if 
it is not on the hot path, but it is easy to just call the appropriate 
super constructor:


public class AbortException extends RuntimeException {

private static final long serialVersionUID = 2608302175475740417L;

/**
 * Constructor AbortException
 */
public AbortException() { super(null, null, false, false); }
}


Regards, Peter

On 08/17/2016 04:04 PM, Aleks Efimov wrote:

Hi Joe, Aleksey,
Thank you for reviewing the initial fix.

I followed the Joe's suggestion (thanks for that) and removed static 
'abort' field completely, the functionality was replaced by throwing 
the exception of newly added type - AbortException. The new webrev 
with removed 'abort' can be found here:

http://cr.openjdk.java.net/~aefimov/8146961/9/01

The Tomcat reproducer attached to the bug report fails without the fix 
and passes with the new version of the fix.

The JPRT and JCK testing again shows no related jaxp tests failures.

Best Regards,
Aleksej


On 15/08/16 21:09, Joe Wang wrote:

Hi Aleksej,

I suggest we get rid of the static abort. If RuntimeException 
happens, check where it happens. The first use case looks suspicious 
as it just returns if it's an instance of RE. For the 2nd case in DOM 
error report, let's throw a RuntimeException with the specified error 
message if error happens, and there's no handler or the handler 
failed to handle it. (would be better than an empty RE)


Best,
Joe

On 8/15/16, 10:38 AM, Aleks Efimov wrote:

Hi,

Please, help to review the fix for memory leak [1] in 
com.sun.org.apache.xerces.internal.dom.DOMNormalizer that is caused 
by usage of static final exceptions.
This problem was already fixed in Apache Xerces project [2] and I 
would like to backport it to JDK9 Xerces implementation.

Webrev with the changes:
http://cr.openjdk.java.net/~aefimov/8146961/9/00

The Tomcat reproducer attached to the bug report fails without the 
fix and passes with the fix.
The JPRT and JCK testing shows no jaxp tests failures with the 
proposed fix.


With Best Regards,
Aleksej

[1] https://bugs.openjdk.java.net/browse/JDK-8146961
[2] https://issues.apache.org/jira/browse/XERCESJ-1667