RFR(m): JEP 269 initial API and skeleton implementation (JDK-8139232)

[Stuart Marks]

Hi all,

Please review these API and implementation changes that comprise the first 
integration of JEP 269. I intend to push this under the "initial API and 
skeleton implementation" subtask, JDK-8139232.


Changes since the previous review:
 - more precise wording regarding static factory methods (thanks Remi)
 - add defensive copy and test for List.of(E...) (thanks Michael Hixson)
 - more null checking and tests
 - various small wording cleanups
 - @since 9

I've left the fixed-arg overloads at ten for all three interfaces. The fixed-arg 
methods provide a useful fast path for initialization and non-desktop, 
non-server cases. The cost is some API clutter; ten elements or pairs is rather 
a lot. This number should be sufficient, though, to handle the vast majority of 
cases without having to switch to varargs. Note that the clutter is only in the 
API definitions, and it doesn't intrude into the point of call (at least for 
List and Set). For the Map case, it's particularly useful to have lots of 
fixed-arg overloads, since the varargs case requires switching APIs and adding 
more boilerplate.


I've also updated the JEP text to reflect the current proposal, mainly the 
removal of the static factory methods from the concrete classes.


JEP:

http://openjdk.java.net/jeps/269

API spec (basically List, Map, and Set):

http://cr.openjdk.java.net/~smarks/reviews/jep269/api.20151123/

Specdiff:


http://cr.openjdk.java.net/~smarks/reviews/jep269/specdiff.20151123/overview-summary.html

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/jep269/webrev.20151123/

Thanks,

s'marks

Re: RFR: jsr166 jdk9 integration wave 2

[Martin Buchholz]

On Mon, Nov 23, 2015 at 8:04 PM, joe darcy  wrote:

> Hello,
>
> There is a high barrier to consistent use of new jtreg keywords. FWIW, I
> would prefer expanding the scope of @key randomness to defining a new @key
> nondeterminstic.
>

It should come as no surprise to anyone that jtreg tests in j.u.concurrent
are usually nondeterministic.  When we have deterministic tests, they
usually end up as tck tests instead.   Do you really think adding @key
randomness to all of those tests will help?

Our approach instead has been to try to make these tests much more robust,
including the use of lots of noisy testing.

Re: RFR: 8140344: add support for 3 digit update release numbers

[David Holmes]

On 24/11/2015 12:24 AM, Rob McKenna wrote:

Thanks David, I'll mark this 9-na. (bcc'ing verona-dev)

Something similar was fixed in 7
(https://bugs.openjdk.java.net/browse/JDK-8009634) - that may be what
you're thinking of.


6 hit this with 6u101 first, so presumably it was addressed there (and 
there was a big bug tail for that).



Aside from that are you ok with the changes?


Sorry I didn't actually review the code.

David


 -Rob

On 18/11/15 09:00, David Holmes wrote:

On 18/11/2015 1:30 AM, Rob McKenna wrote:

I'd expect this to be superseded by 223 completely but I've cc'd
verona-dev in case there are objections. At this point the fix is more
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes
more sense?


I'd vote for a 7 (and 8?) specific fix. Didn't this already get fixed
in 6.

David


 -Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

Re: RFR: jsr166 jdk9 integration wave 2

[joe darcy]

Hello,

There is a high barrier to consistent use of new jtreg keywords. FWIW, I 
would prefer expanding the scope of @key randomness to defining a new 
@key nondeterminstic.


Thanks,

-Joe

On 11/18/2015 12:41 AM, Paul Sandoz wrote:


On 18 Nov 2015, at 03:46, Martin Buchholz > wrote:




On Tue, Nov 17, 2015 at 2:26 AM, Paul Sandoz > wrote:



For the jtregTest updates, if you have any excess energy
remaining, you might want to consider adding the jtreg randomness
tag (i cannot recall its exact name) to the test metadata.


Most of the jtreg tests deliberately include non-determinism from use 
of multiple threads, and there is no @key nondeterminstic tag, and 
adding randomness tags to the tests that additionally invoke a prng 
doesn't really seem useful.  Flakes R Us!




I am sure Joe, CC’ed will have an opinion on this. IIUC correct the 
existing randomness tag actually serves a simialr purpose as your 
proposed nondeterminstic tag.



I support adding @key nondeterministic, and allowing that a tag to 
apply to an entire tree (java.util.concurrent).  Do we have any tests 
dependent on the phase of the moon?


Dunno :-) perhaps every one in a while a machine running tests is 
effected by neutrinos and causes a one off error?


Paul.

Re: RFR: 8085984 Add JDBC Sharding API

[Lance Andersen]

Hi Joe,

Thank you and Ulf for catching the error message typo as we changed the name of 
the method.   I will fix that before I push

Best
Lance
On Nov 23, 2015, at 7:09 PM, huizhe wang  wrote:

> Hi Lance,
> 
> Looks good, except as Ulf pointed out already, in both in Connection.java and 
> XAConnection.java, SQLFeatureNotSupportedException should have been thrown 
> with a message "setShardingKeyIfValid not implemented" rather than "isValid 
> not implemented" (4 occurrences in total).
> 
> Best,
> Joe
> 
> On 11/23/2015 3:01 PM, Lance Andersen wrote:
>> Hi all,
>> 
>> Need a reviewer for 8085984, which adds support for Sharding.  The CCC has 
>> been approved.  The webrev can be found at 
>> http://cr.openjdk.java.net/~lancea/8085984/webrev.00/
>> 
>> 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
>> 
>> 
>> 
> 



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

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[John Rose]

On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev  
wrote:
> 
> Okay, here it is (only tests changed):
> http://cr.openjdk.java.net/~shade/8136500/webrev.06/ 
> 

Thanks; that's a solid test.

Cleanups are good too.

You can count me as a reviewer.

— John

P.S.  I think a promising stringSize for *random* inputs would
switch on numberOfLeadingZeros and then perform at most
one comparison per case.  We'd have to improve our game
optimizing switches, though, not to hurt performance on
the naturally occurring distributions.

Again, Integer.toString for *random* inputs is an interesting
problem that is probably amenable to divide-and-conquer,
or at least to encoding more than one digit at a time.

P.P.S. I filed https://bugs.openjdk.java.net/browse/JDK-8143859 

to track an underlying problem with the JIT, which is that
it should be balancing those test trees for you.

Re: RFR: 8085984 Add JDBC Sharding API

[huizhe wang]

Hi Lance,

Looks good, except as Ulf pointed out already, in both in 
Connection.java and XAConnection.java, SQLFeatureNotSupportedException 
should have been thrown with a message "setShardingKeyIfValid not 
implemented" rather than "isValid not implemented" (4 occurrences in total).


Best,
Joe

On 11/23/2015 3:01 PM, Lance Andersen wrote:

Hi all,

Need a reviewer for 8085984, which adds support for Sharding.  The CCC has been 
approved.  The webrev can be found at 
http://cr.openjdk.java.net/~lancea/8085984/webrev.00/

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: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Vitaly Davidovich]

Ah, the good 'ole switch statement earning more optimization jiras :).

sent from my phone
On Nov 23, 2015 7:36 PM, "John Rose"  wrote:

> On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev 
> wrote:
> >
> > Okay, here it is (only tests changed):
> > http://cr.openjdk.java.net/~shade/8136500/webrev.06/ <
> http://cr.openjdk.java.net/~shade/8136500/webrev.06/>
>
> Thanks; that's a solid test.
>
> Cleanups are good too.
>
> You can count me as a reviewer.
>
> — John
>
> P.S.  I think a promising stringSize for *random* inputs would
> switch on numberOfLeadingZeros and then perform at most
> one comparison per case.  We'd have to improve our game
> optimizing switches, though, not to hurt performance on
> the naturally occurring distributions.
>
> Again, Integer.toString for *random* inputs is an interesting
> problem that is probably amenable to divide-and-conquer,
> or at least to encoding more than one digit at a time.
>
> P.P.S. I filed https://bugs.openjdk.java.net/browse/JDK-8143859 <
> https://bugs.openjdk.java.net/browse/JDK-8143859>
> to track an underlying problem with the JIT, which is that
> it should be balancing those test trees for you.

Re: Reference.reachabilityFence

[mark . reinhold]

2015/11/23 8:38 -0800, paul.san...@oracle.com:
> Please review the addition of Reference.reachabilityFence contributed
> by Aleksey, Doug and myself:
> 
>   
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/
>   
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-hotspot/webrev/

This seems eminently reasonable, but why does it belong in the
java.lang.ref.Reference class?  It has nothing (directly) to do
with reference objects.

java.lang.Runtime, perhaps?

- Mark

Re: Reference.reachabilityFence

[Vitaly Davidovich]

Hi Peter,

Reachability is not a side effect the compiler should preserve.  In other
words, if a method has no other side effects and is eliminated, I'd expect
this to shorten the live range of any of its, say, arguments assuming no
further use of them.  If that doesn't happen, I'd consider that a bug (or
at least missed optimization).

sent from my phone
On Nov 23, 2015 6:09 PM, "Peter Levart"  wrote:

> Hi,
>
> On 11/23/2015 05:50 PM, Vitaly Davidovich wrote:
>
> Hi Paul,
>
> Glad you guys are addressing this.
>
> It looks like C1 and C2 will actually call this method.  Is the longer term
> plan to teach the compilers that this method does not need to be called but
> rather expand the live range of the reference?
>
>
> It seems that (at least sometimes) compiler(s) are able to do just that.
>
> With the following test:
>
>
> http://cr.openjdk.java.net/~plevart/misc/ReachabilityFence/ReachabilityFence2.java
>
> Even a simple empty static method "inlineableNoOp(Object)" acts as a
> reachabilityFence.
>
> When invoked with:
>
> -Dpremature=true
> -XX:+IgnoreUnrecognizedVMOptions
> -showversion
> -server
> -XX:-TieredCompilation
> -Xbatch
> -Xcomp
> -XX:+PrintCompilation
> -XX:+UnlockDiagnosticVMOptions
> -XX:+PrintInlining
>
>
> ...we can see that inlineableNoOp() is in fact inlined, but the test still
> passes (if the call to inlineableNoOp() is removed it fails!)
>
>
>2771 1161bReachabilityFence2::fenced (38 bytes)
> @ 0   java.lang.System::nanoTime (0 bytes)
> (intrinsic)
> @ 9   ReachabilityFence2::fencedTry (30
> bytes)   inline (hot)
>   @ 4
> ReachabilityFence2$MyFinalizeable:: (16 bytes)   inline (hot)
> @ 1   java.lang.Object:: (1 bytes)
> inline (hot)
> @ 9
> java.util.concurrent.atomic.AtomicBoolean:: (5 bytes)   inline (hot)
>   @ 1   java.lang.Object:: (1
> bytes)   inline (hot)
>!  @ 14   ReachabilityFence2::doGc (22 bytes)
> inline (hot)
>   @ 18
> java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)   inline (hot)
>   @ 24   ReachabilityFence2::inlineableNoOp (1
> bytes)   inline (hot)
> @ 25   java.lang.System::nanoTime (0 bytes)
> (intrinsic)
> @ 9   ReachabilityFence2::fencedTry (30
> bytes)   inline (hot)
>   @ 4
> ReachabilityFence2$MyFinalizeable:: (16 bytes)   inline (hot)
> @ 1   java.lang.Object:: (1 bytes)
> inline (hot)
> @ 9
> java.util.concurrent.atomic.AtomicBoolean:: (5 bytes)   inline (hot)
>   @ 1   java.lang.Object:: (1
> bytes)   inline (hot)
>!  @ 14   ReachabilityFence2::doGc (22 bytes)
> inline (hot)
> @ 8   java.lang.System::gc (7 bytes)
> inline (hot)
>   @ 0   java.lang.Runtime::getRuntime (4
> bytes)   inline (hot)
>   @ 3   java.lang.Runtime::gc (0 bytes)
> native method
> @ 14   java.lang.Thread::sleep (0 bytes)
> native method
>   @ 18
> java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)   inline (hot)
>   @ 24   ReachabilityFence2::inlineableNoOp (1
> bytes)   inline (hot)
>
>
> When invoked with:
>
> -Dpremature=true
> -XX:+IgnoreUnrecognizedVMOptions
> -showversion
> -server
> -XX:TieredStopAtLevel=1
> -Xbatch
> -Xcomp
> -XX:+PrintCompilation
> -XX:+UnlockDiagnosticVMOptions
> -XX:+PrintInlining
>
>
> The test passes and prints:
>
> 904  837b  1   ReachabilityFence2::fenced (38 bytes)
>   @ 0   java.lang.System::nanoTime (0 bytes)
> intrinsic
>   @ 9   ReachabilityFence2::fencedTry (30
> bytes)
> @ 4
> ReachabilityFence2$MyFinalizeable:: (16 bytes)
>   @ 1   java.lang.Object:: (1 bytes)
>   @ 9
> java.util.concurrent.atomic.AtomicBoolean:: (5 bytes)
> @ 1   java.lang.Object:: (1
> bytes)
>!@ 14   ReachabilityFence2::doGc (22 bytes)
>   @ 8   java.lang.System::gc (7 bytes)
> @ 0   java.lang.Runtime::getRuntime (4
> bytes)
> @ 3   java.lang.Runtime::gc (0
> bytes)   native method
>   @ 14   java.lang.Thread::sleep (0
> bytes)   native method
> @ 18
> java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)
> @ 24   ReachabilityFence2::inlineableNoOp
> (1 bytes)
>

Re: RFR: 8085984 Add JDBC Sharding API

[Ulf Zibis]

Hi,

Isn't it "i_f_Valid not implemented" instead "i_s_Valid not implemented"  or eventually 
"setShardingKeyIfValid not implemented"?


-Ulf


Am 24.11.2015 um 00:01 schrieb Lance Andersen:

Hi all,

Need a reviewer for 8085984, which adds support for Sharding.  The CCC has been 
approved.  The webrev can be found at 
http://cr.openjdk.java.net/~lancea/8085984/webrev.00/

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: Reference.reachabilityFence

[Peter Levart]

Hi,

On 11/23/2015 05:50 PM, Vitaly Davidovich wrote:

Hi Paul,

Glad you guys are addressing this.

It looks like C1 and C2 will actually call this method.  Is the longer term
plan to teach the compilers that this method does not need to be called but
rather expand the live range of the reference?


It seems that (at least sometimes) compiler(s) are able to do just that.

With the following test:

http://cr.openjdk.java.net/~plevart/misc/ReachabilityFence/ReachabilityFence2.java

Even a simple empty static method "inlineableNoOp(Object)" acts as a 
reachabilityFence.


When invoked with:

-Dpremature=true
-XX:+IgnoreUnrecognizedVMOptions
-showversion
-server
-XX:-TieredCompilation
-Xbatch
-Xcomp
-XX:+PrintCompilation
-XX:+UnlockDiagnosticVMOptions
-XX:+PrintInlining


...we can see that inlineableNoOp() is in fact inlined, but the test 
still passes (if the call to inlineableNoOp() is removed it fails!)



   2771 1161bReachabilityFence2::fenced (38 bytes)
@ 0   java.lang.System::nanoTime (0 
bytes)   (intrinsic)
@ 9   ReachabilityFence2::fencedTry (30 
bytes)   inline (hot)
  @ 4 
ReachabilityFence2$MyFinalizeable:: (16 bytes)   inline (hot)
@ 1   java.lang.Object:: (1 
bytes)   inline (hot)
@ 9 
java.util.concurrent.atomic.AtomicBoolean:: (5 bytes) inline (hot)
  @ 1 java.lang.Object:: (1 
bytes)   inline (hot)
   !  @ 14   ReachabilityFence2::doGc (22 
bytes)   inline (hot)
  @ 18 
java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)   inline (hot)
  @ 24 ReachabilityFence2::inlineableNoOp 
(1 bytes)   inline (hot)
@ 25   java.lang.System::nanoTime (0 
bytes)   (intrinsic)
@ 9   ReachabilityFence2::fencedTry (30 
bytes)   inline (hot)
  @ 4 
ReachabilityFence2$MyFinalizeable:: (16 bytes)   inline (hot)
@ 1   java.lang.Object:: (1 
bytes)   inline (hot)
@ 9 
java.util.concurrent.atomic.AtomicBoolean:: (5 bytes) inline (hot)
  @ 1 java.lang.Object:: (1 
bytes)   inline (hot)
   !  @ 14   ReachabilityFence2::doGc (22 
bytes)   inline (hot)
@ 8   java.lang.System::gc (7 bytes)   
inline (hot)
  @ 0 java.lang.Runtime::getRuntime (4 
bytes)   inline (hot)
  @ 3   java.lang.Runtime::gc (0 
bytes)   native method
@ 14   java.lang.Thread::sleep (0 
bytes)   native method
  @ 18 
java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)   inline (hot)
  @ 24 ReachabilityFence2::inlineableNoOp 
(1 bytes)   inline (hot)



When invoked with:

-Dpremature=true
-XX:+IgnoreUnrecognizedVMOptions
-showversion
-server
-XX:TieredStopAtLevel=1
-Xbatch
-Xcomp
-XX:+PrintCompilation
-XX:+UnlockDiagnosticVMOptions
-XX:+PrintInlining


The test passes and prints:

904  837b  1   ReachabilityFence2::fenced (38 bytes)
  @ 0   java.lang.System::nanoTime (0 
bytes)   intrinsic
  @ 9   ReachabilityFence2::fencedTry (30 
bytes)
@ 4 
ReachabilityFence2$MyFinalizeable:: (16 bytes)

  @ 1 java.lang.Object:: (1 bytes)
  @ 9 
java.util.concurrent.atomic.AtomicBoolean:: (5 bytes)

@ 1 java.lang.Object:: (1 bytes)
   !@ 14   ReachabilityFence2::doGc (22 bytes)
  @ 8   java.lang.System::gc (7 bytes)
@ 0 java.lang.Runtime::getRuntime 
(4 bytes)
@ 3   java.lang.Runtime::gc (0 
bytes)   native method
  @ 14   java.lang.Thread::sleep (0 
bytes)   native method
@ 18 
java.util.concurrent.atomic.AtomicBoolean::get (13 bytes)
@ 24 ReachabilityFence2::inlineableNoOp 
(1 bytes)
  @ 25   java.lang.System::nanoTime (0 
bytes)   intrinsic



This seems to indicate that for establishing a reachability even 
inlineable no-op (empty) method with single argument is enough.


It might not be enough in every situation, but what this means is that 
it will be very difficult to write a positive test for 
Reference.reachabilityFence(Object) when we can't write a negative test 
with a simple inlineable static no-op method. I haven't been able to 
come-up with a form of code that passes the negative test (nonFenced) 
and call the inlineableNoOp in the process.

RFR: 8085984 Add JDBC Sharding API

[Lance Andersen]

Hi all,

Need a reviewer for 8085984, which adds support for Sharding.  The CCC has been 
approved.  The webrev can be found at 
http://cr.openjdk.java.net/~lancea/8085984/webrev.00/

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: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Karen Kinnear]

Frederic,

Looks good.

Many thanks.
Karen

> On Nov 23, 2015, at 12:44 PM, Frederic Parain  
> wrote:
> 
> Karen,
> 
> Thank you for your review, my answers are in-lined below.
> 
> New Webrevs (including some fixes suggested by Paul Sandoz):
> 
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/ 
> 
> http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/ 
> 
> 
> On 20/11/2015 19:44, Karen Kinnear wrote:
>> Frederic,
>> 
>> Code review for web revs you sent out.
>> Code looks good. I am not as familiar with the compiler code.
>> 
>> I realize you need to check in at least a subset of the java.util.concurrent 
>> changes for
>> the test to work, so maybe I should not have asked Doug about his preference 
>> there.
>> Sorry.
>> 
>> I am impressed you found a way to make a reproducible test!
>> 
>> Comments/questions:
>> 1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”
> 
> Fixed
> 
>> 2. IIRC, due to another bug with windows handling of our guard pages, this
>> is disabled for Windows. Would it be worth putting a comment in the
>> bug , 8067946, that once this is fixed, the reserved stack logic on windows
>> will need testing before enabling?
> 
> More than testing, the implementation would have to be completed on
> Windows. I've added a comment to JDK-8067946.
> 
>> 3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
>> this is in interpreter code, you explicitly return the Java sender
>> of the current frame. I was expecting to see that on Solaris_sparc and 
>> Windows
>> as well? I do see the assertion in caller that you do have a java frame.
> 
> It doesn't make sense to check the current frame (no bytecode has been
> executed yet, so risk of partially executed critical section). I did the
> change but not for all platforms. This is now fixed for Solaris_SPARC
> and Windows too.
> 
>> 4. test line 83 “writtent” -> “written”
> 
> Fixed
> 
>> 5. I like the way you set up the preallocated exception and then set the 
>> message - we may
>> try that for default methods in future.
>> 
>> 6. I had a memory that you had found a bug in safe_for_sender - did you 
>> already check that in?
> 
> I've fixed x86 platforms in JDK-8068655.
> I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
> I had hoped it would have been silently accepted :-)
> 
> 
>> 7. I see the change in trace.xml, and I see an include added to 
>> SharedRuntime.cpp,
>> but I didn’t see where it was used - did I just miss it?
> 
> trace.xml changes define a new event.
> This event is created at sharedRuntime.cpp::3006 and it is used
> in the next 3 lines.
Thanks. I must have mistyped when I searched for it.
> 
> Thanks,
> 
> Fred
> 
> -- 
> Frederic Parain - Oracle
> Grenoble Engineering Center - France
> Phone: +33 4 76 18 81 17
> Email: frederic.par...@oracle.com 

Re: RFR 9: 8138696 : java.lang.ref.Cleaner - an easy to use alternative to finalization

[mark . reinhold]

( Finally getting back to this, after too many weeks of travel ... )

2015/10/20 11:28 -0700, roger.ri...@oracle.com:
> Sorry for the silence, JavaOne preparations and the availability of
> folks who wanted to review have stretched things out.
> 
> The Cleaner API was very simple and saw feature creep as the ideas for
> how it might be used were explored.  There are concerns about
> committing to supporting subclassable CleanableReferences in all
> future JDK versions before there had been a chance to see how if they
> would be useful and necessary to address the need to reduce the use of
> finalization within the OpenJDK and beyond.
> 
> ...
> 
> Updated Javadoc:
>http://cr.openjdk.java.net/~rriggs/cleaner-doc/
> 
> Updated Webrev:
> http://cr.openjdk.java.net/~rriggs/webrev-cleaner-8138696/

I'm very happy to see this API return to something like its original
simple form, but I think it can be simplified even further.

We have a very strong need for phantom-ref-based cleaners, so as to
discourage people from relying upon flaky finalization. The arguments in
support of the weak and soft forms have, by contrast, been rather weak
(and soft?).  I don't think it's right to bake methods into a core API
based on just a couple of hypothetical use cases.  I'd much rather see
the Cleaner::{phantom,soft,weak}Cleanable methods reduced to a single
register method,

Cleaner.Cleanable register(Object, Runnable);

which would create the phantom form only.  If strong justification for
the other forms arise then we can generalize this later, either to
distinct register{Soft,Weak} methods or, perhaps, to a method that takes
a type token.

- Mark

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Aleksey Shipilev]

On 11/23/2015 08:58 PM, John Rose wrote:
> On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov  > wrote:
>>
>> Though, it may be better to get yet another pair of eyes.
>>
>> One minor nit: In the tests, in the summary, it is written, "Test
>> Integer.toString method*s*", but only one of the overloads is tested.
> 
> Here's another nit in the tests.
> This is supposed to "wiggle around" critical points, which I agree with.
> But it only wiggles above:
> 
>   39 while (base < Long.MAX_VALUE / 10) {
>   40 for (int c = 1; c < 65536; c++) {
>   41 buildAndTest(base + c);
>   42 }
>   43 base = (base == 0) ? 1 : base * 10;
>   44 }
> 
> I suggest:  for (int c = -1<<15; c <= 1<<15; c++)
> 
> You'll need to guard the call to buildAndTest to avoid negatives.
> You could also start base at, say, 1.

Okay, here it is (only tests changed):
 http://cr.openjdk.java.net/~shade/8136500/webrev.06/

Thanks,
-Aleksey

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Paul Sandoz]

> On 23 Nov 2015, at 16:08, Aleksey Shipilev  
> wrote:
> 
> On 11/23/2015 04:34 PM, Ivan Gerasimov wrote:
>> With this fixed patch:
>> http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch
>> all tests from test/lang pass.
>> 
>> Would you give it another chance?
> 
> Okay, but this is better be the last non-cosmetic change to board this
> departing train:
>  http://cr.openjdk.java.net/~shade/8136500/webrev.05/
> 

+1

Nice cleanup.

Paul.

> This passes java/lang, java/util tests, and performance improvement is
> still very good.
> 
> Thanks,
> -Aleksey
> 

Re: RFR(L): JDK-8046936 : JEP 270: Reserved Stack Areas for Critical Sections

[Frederic Parain]

Karen,

Thank you for your review, my answers are in-lined below.

New Webrevs (including some fixes suggested by Paul Sandoz):

http://cr.openjdk.java.net/~fparain/8046936/webrev.01/hotspot/
http://cr.openjdk.java.net/~fparain/8046936/webrev.01/jdk/

On 20/11/2015 19:44, Karen Kinnear wrote:

Frederic,

Code review for web revs you sent out.
Code looks good. I am not as familiar with the compiler code.

I realize you need to check in at least a subset of the java.util.concurrent 
changes for
the test to work, so maybe I should not have asked Doug about his preference 
there.
Sorry.

I am impressed you found a way to make a reproducible test!

Comments/questions:
1. src/cpu/sparc/vm/interp_masm_sparc.cpp line 1144 “is” -> “if”


Fixed


2. IIRC, due to another bug with windows handling of our guard pages, this
is disabled for Windows. Would it be worth putting a comment in the
bug , 8067946, that once this is fixed, the reserved stack logic on windows
will need testing before enabling?


More than testing, the implementation would have to be completed on
Windows. I've added a comment to JDK-8067946.


3. In get_frame_at_stack_banging_point on Linux, BSD and solaris_x86 if
this is in interpreter code, you explicitly return the Java sender
of the current frame. I was expecting to see that on Solaris_sparc and Windows
as well? I do see the assertion in caller that you do have a java frame.


It doesn't make sense to check the current frame (no bytecode has been
executed yet, so risk of partially executed critical section). I did the
change but not for all platforms. This is now fixed for Solaris_SPARC
and Windows too.


4. test line 83 “writtent” -> “written”


Fixed


5. I like the way you set up the preallocated exception and then set the 
message - we may
try that for default methods in future.

6. I had a memory that you had found a bug in safe_for_sender - did you already 
check that in?


I've fixed x86 platforms in JDK-8068655.
I've piggybacked the SPARC fix to this webrev (frame_sparc.cpp:635),
I had hoped it would have been silently accepted :-)



7. I see the change in trace.xml, and I see an include added to 
SharedRuntime.cpp,
but I didn’t see where it was used - did I just miss it?


trace.xml changes define a new event.
This event is created at sharedRuntime.cpp::3006 and it is used
in the next 3 lines.

Thanks,

Fred

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

Re: Reference.reachabilityFence

[Peter Levart]

On 11/23/2015 06:33 PM, Peter Levart wrote:


http://cr.openjdk.java.net/~plevart/misc/ReachabilityFence/ReachabilityFence.java

This works reliably on Linux and only takes not much more than 200ms 
per test.


I wanted to say, not more than 100ms per negative test. How to choose 
the POSITIVE_TIMEOUT then?


Hint: measure negative test time and double or triple it.

Regards, Peter

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

> On Nov 23, 2015, at 8:42 AM, Alan Bateman  wrote:
> 
> We need to do a bit of clean-up in Images.gmk to make things clearer as this 
> MAIN vs. PROVIDER topic has caused confusion on a few cases. If we can keep 
> the lists separate to the list of modules for the compact profile builds then 
> there is no reason why they can't be combined as JRE_MODULES.

Agree and we should clean that up to remove the confusion.

Mandy

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Xueming Shen]

On 11/23/2015 07:08 AM, Aleksey Shipilev wrote:

On 11/23/2015 04:34 PM, Ivan Gerasimov wrote:

With this fixed patch:
http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch
all tests from test/lang pass.

Would you give it another chance?

Okay, but this is better be the last non-cosmetic change to board this
departing train:
   http://cr.openjdk.java.net/~shade/8136500/webrev.05/

This passes java/lang, java/util tests, and performance improvement is
still very good.

Thanks,
-Aleksey



+1

sherman

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 16:07, Attila Szegedi wrote:

:
Whichever is the stronger criteria for deciding whether to place it in MAIN or 
PROVIDER is fine with me. Intuitively “provider” seems like a weaker category 
(exposes a service provider but doesn’t have its own API), so (without knowing 
the particulars of the use of these *_MODULES variables) it seems to me Mandy’s 
suggestion is correct to reclassify Nashorn into a MAIN module.

We need to do a bit of clean-up in Images.gmk to make things clearer as 
this MAIN vs. PROVIDER topic has caused confusion on a few cases. If we 
can keep the lists separate to the list of modules for the compact 
profile builds then there is no reason why they can't be combined as 
JRE_MODULES.


In this case then jdk.scripting.nashorn.shell is already listed in 
MAIN_MODULES so this will ensure than Nashorn is linked into any 
run-time image that has the the jjs tool/shell. It doesn't matter if 
jdk.scripting.nashorn is listed or not.


-Alan.

(De-)serialization, quo vadis

[Moritz Bechler]

Hi,

I'm not absolutely sure this is the best place to have this discussion
(pointers welcome), but it's the most appropriate I figured out so far.

In the light of the most recent code execution vulnerabilities through
arbitrary object deserialization - and the follow-ups that I can
guarantee you to come - I think that the Java community has to have a
serious discussion about serialization and it's use.

If, by any chance, someone is not familiar with these issues,

may be the best write-up on this I've seen so far. Still, the presented
workarounds are mere short term mitigations because they either break a
lot of stuff (yeah, disable deserialization completely), are very hard
to apply (whitelisting) or are broken by design (blacklisting).

Imho, this whole mess mostly results from differing positions that are
taken on the issue in general:
1. using java deserialization on any input crossing a trust boundary is
a vulnerability - sometimes even extending to: we don't care that our
library code makes others exploitable.
2. well, we only use that on internal traffic we define as "trusted", so
we don't care. (that, I would consider a rather dangerous position, I
also wouldn't want my monitoring system to take over my servers or vice
versa)
3. (de)serialization should be safe

This is a very explosive mixture, so at some point the community has to
reach an agreement on what it should be and how we are going fix this.

I think we will agree that the deserialization primitive is totally
broken from a security point of view at this time, but a the same time
this is used (or let's even say encouraged) by quite a few of core
specifications (think RMI/JRMP, JMS, JPA - although I haven't checked
whether this is actually mandated by the spec, EJB, ...) - so were in a
bit of a pickle here. This reflects very badly on the whole ecosystem.

So, the basic choices more or less are to either fix the primitive some
way or to drop it from all those technologies that use it in an
potentially unsafe way. Both ways will be painful, but I think this is
very necessary or issues like these will turn up every other year.



Moritz

-- 
AgNO3 GmbH & Co. KG, Sitz Tübingen, Amtsgericht Stuttgart HRA 728731
Persönlich haftend:
Metagesellschaft mbH, Sitz Tübingen, Amtsgericht Stuttgart HRB 744820,
Vertreten durch Joachim Keltsch

Reference.reachabilityFence

[Paul Sandoz]

Hi,

Please review the addition of Reference.reachabilityFence contributed by 
Aleksey, Doug and myself:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/
 

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-hotspot/webrev/
 


The implementation approach marks the method Reference.reachabilityFence as not 
inline-able, thereby “keeping alive” an object passed to the method until at 
least after the method call.

The testing approach i have taken is to verify that the method does not get 
inlined either in C1 or C2. The test approach seems fragile (as fragile as the 
accessor-based test i code-cargo-culted from) but passes ok through JPRT.

I could not find a suitable mechanism in WhiteBox. Is there a more reliable 
mechanism to determine what methods are inlined into a compiled method?

There is another testing approach in the VarHandles sandbox:

  http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/433114b32d2d#l2.2 


But i am not confident that the test can be run within a reasonable time and 
reliably on all platforms and VM modes.

Paul.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Mandy Chung]

> On Nov 23, 2015, at 7:40 AM, Alan Bateman  wrote:
> 
> 
> 
> On 23/11/2015 15:27, Attila Szegedi wrote:
>> Folks,
>> 
>> I integrated the changes Mandy suggested; please review these (build 
>> related) changes:
>> jdk9 top level: 
>> 
>> jdk9/jdk: 
>> 
>> 
>> For the sake of completeness, the jdk/nashorn changes are here: 
>>  but they have 
>> already been reviewed by Hannes and Sundar; only the above two (jdk9 and 
>> jdk9/jdk) have been modified compared to the original review request.
>> 
>> Sundar was kind enough to verify that JDK9 builds fine with these changes.
>> 
> The jdk repo looks okay (just had to change your link to find it :-)
> 
> In the top-level repo then you've moved jdk.scripting.nashorn from 
> PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
> PROVIDER_MODULES is because we treat it as a service provider module (it 
> provides an implementation of javax.script.ScriptEngineFactory).

jdk.scripting.nashorn now exports two API packages.  It is no longer a sole 
service provider and so I asked Attila to move it MAIN_MODULE.

Mandy

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:43, Sundararajan Athijegannathan wrote:
But, in addition to providing service, jdk.scripting.nashorn module 
also "exports" nashorn specific APIs in jdk.nashorn.api.* packages.

Sure, it could go in either but we mostly treat it as a service provider.

-Alan.

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

> On Nov 23, 2015, at 4:40 PM, Alan Bateman  wrote:
> 
> 
> 
> On 23/11/2015 15:27, Attila Szegedi wrote:
>> Folks,
>> 
>> I integrated the changes Mandy suggested; please review these (build 
>> related) changes:
>> jdk9 top level: 
>> 
>> jdk9/jdk: 
>> 
>> 
>> For the sake of completeness, the jdk/nashorn changes are here: 
>>  but they have 
>> already been reviewed by Hannes and Sundar; only the above two (jdk9 and 
>> jdk9/jdk) have been modified compared to the original review request.
>> 
>> Sundar was kind enough to verify that JDK9 builds fine with these changes.
>> 
> The jdk repo looks okay (just had to change your link to find it :-)

D’oh. I was doublechecking those links few times and still managed to bungle 
it… thanks for figuring it out.
> 
> In the top-level repo then you've moved jdk.scripting.nashorn from 
> PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
> PROVIDER_MODULES is because we treat it as a service provider module (it 
> provides an implementation of javax.script.ScriptEngineFactory).

Whichever is the stronger criteria for deciding whether to place it in MAIN or 
PROVIDER is fine with me. Intuitively “provider” seems like a weaker category 
(exposes a service provider but doesn’t have its own API), so (without knowing 
the particulars of the use of these *_MODULES variables) it seems to me Mandy’s 
suggestion is correct to reclassify Nashorn into a MAIN module.

Attila.

> 
> -Alan.
> 

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Sundararajan Athijegannathan]

But, in addition to providing service, jdk.scripting.nashorn module also 
"exports" nashorn specific APIs in jdk.nashorn.api.* packages.


-Sundar

On 11/23/2015 9:10 PM, Alan Bateman wrote:



On 23/11/2015 15:27, Attila Szegedi wrote:

Folks,

I integrated the changes Mandy suggested; please review these (build 
related) changes:
jdk9 top level: 

jdk9/jdk: 



For the sake of completeness, the jdk/nashorn changes are here: 
 but they 
have already been reviewed by Hannes and Sundar; only the above two 
(jdk9 and jdk9/jdk) have been modified compared to the original 
review request.


Sundar was kind enough to verify that JDK9 builds fine with these 
changes.



The jdk repo looks okay (just had to change your link to find it :-)

In the top-level repo then you've moved jdk.scripting.nashorn from 
PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
PROVIDER_MODULES is because we treat it as a service provider module 
(it provides an implementation of javax.script.ScriptEngineFactory).


-Alan.

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Aleksey Shipilev]

On 11/23/2015 09:05 PM, Aleksey Shipilev wrote:
> On 11/23/2015 08:58 PM, John Rose wrote:
>> On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov > > wrote:
>>>
>>> Though, it may be better to get yet another pair of eyes.
>>>
>>> One minor nit: In the tests, in the summary, it is written, "Test
>>> Integer.toString method*s*", but only one of the overloads is tested.
>>
>> Here's another nit in the tests.
>> This is supposed to "wiggle around" critical points, which I agree with.
>> But it only wiggles above:
>>
>>   39 while (base < Long.MAX_VALUE / 10) {
>>   40 for (int c = 1; c < 65536; c++) {
>>   41 buildAndTest(base + c);
>>   42 }
>>   43 base = (base == 0) ? 1 : base * 10;
>>   44 }
> 
> Yes, it *does* wiggle around:
> 
>   71 test(sbN.toString(), -c);
>   72 test(sbP.toString(), c);

Ah no, I understand now. Doing too much at the same time. Let me make
tests more testy.

Thanks,
-Aleksey

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Aleksey Shipilev]

On 11/23/2015 08:58 PM, John Rose wrote:
> On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov  > wrote:
>>
>> Though, it may be better to get yet another pair of eyes.
>>
>> One minor nit: In the tests, in the summary, it is written, "Test
>> Integer.toString method*s*", but only one of the overloads is tested.
> 
> Here's another nit in the tests.
> This is supposed to "wiggle around" critical points, which I agree with.
> But it only wiggles above:
> 
>   39 while (base < Long.MAX_VALUE / 10) {
>   40 for (int c = 1; c < 65536; c++) {
>   41 buildAndTest(base + c);
>   42 }
>   43 base = (base == 0) ? 1 : base * 10;
>   44 }

Yes, it *does* wiggle around:

  71 test(sbN.toString(), -c);
  72 test(sbP.toString(), c);

This saves much testing time to build both "c" and "-c" representations.

> You'll need to guard the call to buildAndTest to avoid negatives.

It already does:

  51 private static void buildAndTest(int c) {
  52 if (c < 0) {
  53 throw new IllegalArgumentException("Test bug: cannot
handle negatives, " + c);
  54 }

> You could also start base at, say, 1.

I don't see a reason for this, why? The overlap in the test data is
fine, if that makes test more understandable and therefore reliable.

Thanks,
-Aleksey

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[John Rose]

On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov  wrote:
> 
> Though, it may be better to get yet another pair of eyes.
> 
> One minor nit: In the tests, in the summary, it is written, "Test 
> Integer.toString method*s*", but only one of the overloads is tested.

Here's another nit in the tests.
This is supposed to "wiggle around" critical points, which I agree with.
But it only wiggles above:

  39 while (base < Long.MAX_VALUE / 10) {
  40 for (int c = 1; c < 65536; c++) {
  41 buildAndTest(base + c);
  42 }
  43 base = (base == 0) ? 1 : base * 10;
  44 }

I suggest:  for (int c = -1<<15; c <= 1<<15; c++)

You'll need to guard the call to buildAndTest to avoid negatives.
You could also start base at, say, 1.

— John

Re: Reference.reachabilityFence

[Peter Levart]

Hi Paul,

Good to see this getting in. Cleaner API depends on it ;-)

On 11/23/2015 05:38 PM, Paul Sandoz wrote:

Hi,

Please review the addition of Reference.reachabilityFence contributed by 
Aleksey, Doug and myself:

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/ 

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-hotspot/webrev/
 


The implementation approach marks the method Reference.reachabilityFence as not 
inline-able, thereby “keeping alive” an object passed to the method until at 
least after the method call.

The testing approach i have taken is to verify that the method does not get 
inlined either in C1 or C2. The test approach seems fragile (as fragile as the 
accessor-based test i code-cargo-culted from) but passes ok through JPRT.

I could not find a suitable mechanism in WhiteBox. Is there a more reliable 
mechanism to determine what methods are inlined into a compiled method?

There is another testing approach in the VarHandles sandbox:

   http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/433114b32d2d#l2.2 


But i am not confident that the test can be run within a reasonable time and 
reliably on all platforms and VM modes.

Paul.



The problem of this other approach is that you constantly invoke 
System.gc(), so your time to warm-up the code to get it compiled is big. 
Why don't you allow the loop to run faster for some warm-up time and 
kick GC in only after that. Like:


http://cr.openjdk.java.net/~plevart/misc/ReachabilityFence/ReachabilityFence.java

This works reliably on Linux and only takes not much more than 200ms per 
test.


You are also relying on JIT to do on-stack replacement. It seems to work 
for all modes (besides interpreter-only).


Regards, Peter

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Aleksey Shipilev]

On 11/23/2015 04:34 PM, Ivan Gerasimov wrote:
> With this fixed patch:
> http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch
> all tests from test/lang pass.
> 
> Would you give it another chance?

Okay, but this is better be the last non-cosmetic change to board this
departing train:
  http://cr.openjdk.java.net/~shade/8136500/webrev.05/

This passes java/lang, java/util tests, and performance improvement is
still very good.

Thanks,
-Aleksey

Re: Reference.reachabilityFence

[Vitaly Davidovich]

Hi Paul,

Glad you guys are addressing this.

It looks like C1 and C2 will actually call this method.  Is the longer term
plan to teach the compilers that this method does not need to be called but
rather expand the live range of the reference?

Thanks

On Mon, Nov 23, 2015 at 11:38 AM, Paul Sandoz 
wrote:

> Hi,
>
> Please review the addition of Reference.reachabilityFence contributed by
> Aleksey, Doug and myself:
>
>
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-jdk/webrev/
>
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8133348-reachability-fence-hotspot/webrev/
>
> The implementation approach marks the method Reference.reachabilityFence
> as not inline-able, thereby “keeping alive” an object passed to the method
> until at least after the method call.
>
> The testing approach i have taken is to verify that the method does not
> get inlined either in C1 or C2. The test approach seems fragile (as fragile
> as the accessor-based test i code-cargo-culted from) but passes ok through
> JPRT.
>
> I could not find a suitable mechanism in WhiteBox. Is there a more
> reliable mechanism to determine what methods are inlined into a compiled
> method?
>
> There is another testing approach in the VarHandles sandbox:
>
>   http://hg.openjdk.java.net/jdk9/sandbox/jdk/rev/433114b32d2d#l2.2
>
> But i am not confident that the test can be run within a reasonable time
> and reliably on all platforms and VM modes.
>
> Paul.
>
>

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Alan Bateman]

On 23/11/2015 15:27, Attila Szegedi wrote:

Folks,

I integrated the changes Mandy suggested; please review these (build related) 
changes:
jdk9 top level: 

jdk9/jdk: 


For the sake of completeness, the jdk/nashorn changes are here: 
 but they have already 
been reviewed by Hannes and Sundar; only the above two (jdk9 and jdk9/jdk) have been 
modified compared to the original review request.

Sundar was kind enough to verify that JDK9 builds fine with these changes.


The jdk repo looks okay (just had to change your link to find it :-)

In the top-level repo then you've moved jdk.scripting.nashorn from 
PROVIDER_MODULES to MAIN_MODULES. The reason that we've had it in 
PROVIDER_MODULES is because we treat it as a service provider module (it 
provides an implementation of javax.script.ScriptEngineFactory).


-Alan.

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Alexander Fomin]

Hi Jason

On 20.11.2015 22:15, Jason Mehrens wrote:

Alexander,

I see your point.  It is also out of spec for Handler.setFormatter to actually 
call Formatter.setErrorManager.  What if there are subclasses of formatter that 
exist today with a setErrorManager method?  Also not all handlers have one 
formatter or actually call super.setFormatter since you can't unseal the super 
handler (GAE).


Good point. You are right, modifying Handler.setErrorManager and 
Handler.setFormatter methods by the way to call 
Formatter.setErrorManger() we may introduce  undesirable behavior. For 
example, if  Handler is configured with a custom Formatter with a custom 
ErrorManger specified, we may spoil the ErrorManager in that Formatter.


Probably the better solution would be to specify the default 
ErrorManager for a Formatter in the constructor of specific Handler. But 
seems we can't just call getErrorManager() in ConsoleHandler 
constructor, for example, as far as the subclass isn't fully initialized 
yet.




I still lean toward letting the exceptions fly since there is already an 
expectation that formatters fail.  Is the concern that the handler will fail or 
that the program will fail?
Well, actually my initial thought was to let the exceptions to be thrown 
from Formatter to corresponding logging Handler where it should be 
processed and reported to a registered ErrorManager. But we've given up 
from this idea because:

- would require a CCC any way.
- would probably require a release note since it will alter the existing 
behavior.
- the current implementation of ErrorManager will cause a 
printStackTrace() - but only the first time such an error happens in 
Handler. So that, the errors in Formatter will be swallowed any way if 
some error happened before.
- this approach may cause an NPE if the concrete Handler subclass has 
not installed any ErrorManager and does not override reportError. So, 
there's the issue of potentially breaking existing code.


So, I was looking for a solution that shouldn't change any existing 
behavior, shouldn't break a code, should work in all cases, should allow 
to a Formatter report about its internal errors itself without relay on 
the corresponding Handler where the Exception could be intentionally 
ignored.




I've recently been tinkering with a custom filter that internally uses a 
formatter and have prototyped adding get/setErrorManager method to it and it 
just becomes a mess because to have start thinking about more corner cases and 
adding more to the spec.
What is wrong with get/serErrorManager for Formatter? It should be just 
a way for Formatter to report its internal errors.



I really think that if you are going to add the 3 ErrorManager methods you 
should only add them to the LogManager (JDK-6865510) as a way to unify failure 
reporting in the LogManager itself and to deal with failures inside of error 
manger.  As in to remove that ugly System.err code in Handler.reportError.  The 
LogManager would work kind of like Thread.defaultUncaughtExceptionHandler and 
the Handler.errorManager is kind of like the Thread.uncaughtException handler.
Well. I'm with you on this. I would prefer the fix is a part of more 
wider enhancement that unified Logger's inner errors reporting inclusive 
redesign for ErrorManager and so on. Probably it's not a big deal, but 
it should be some JEP for JDK9. I'm afraid it's too late for 9.



Then to fix this bug we could report it to via 
LogManager.getLogManager().reportError.
Well, it's good solution as a default behavior if a user not specified 
its own ErrorManager. But, I don't think we have to restrict a user with 
what ErrorManager is used in their Formatters. So, set/getErrorManager 
for Formatter, Handler and others make sense, i guess.


Ok, I'm still hope on some compromise with the current solution.
The only way to avoid undesirable behavior changes that I see now is 
some property in logging.properties that would define if the 
ErrorManager in Formatter for a specific Handler must be specified by 
default with something. Something like that:


java.util.logging.ConsoleHandler.formatter.ErrorManager = default

If it's set in logging.properties then we are eligible to call 
"formatter.setErrorManager(errorManager);" somewhere in Handler 
constructor. If not, we have to use Formatter as it has been specified 
in specific handler.
Looks ugly, but Let my play with that. If I found it OK, I'll back with 
new webrev.


Thanks,
Alexander



Jason



From: Alexander Fomin 
Sent: Friday, November 20, 2015 11:27 AM
To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

Hi Jason

On 20.11.2015 17:47, Jason Mehrens wrote:

Alexander,

Why not just cache the last exception in the formatter and use getTail to clear 
it and report it?

  It might be a good solution,

Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() swallows Exceptions

[Jason Mehrens]

Alexander,

Given those constraints, what about simply modify the output string?  As in:
=
.
} catch (Exception ex) {
// Formatting failed: use localized format string.
return format + " " + toString(record, ex);
}

private String toString(LogRecord record, Exception ex) {
StringWriter sw = new StringWriter();
PrintWriter pw = new PrintWriter(sw);
Object[] params = record.getParameters();
if (params != null) {
   String sep = "";
   for (Object o : params)  {
   pw.append(sep);
   if (o != null) {
   pw.append(o.getClass().getName());
   } else {
   pw.append("null");
   }
   pw.println();
   sep = ",";
   }
}
ex.printStackTrace(pw);
return sw.toString();
}


There is wiggle room in the API docs for modifying the output string which 
would avoid a CCC.


Jason


From: Alexander Fomin 
Sent: Monday, November 23, 2015 9:27 AM
To: Jason Mehrens; core-libs-dev@openjdk.java.net; Daniel Fuchs; 
mandy.ch...@oracle.com
Subject: Re: JDK 9 RFR [8137005]: java.util.logging.Formatter#formatMessage() 
swallows Exceptions

Hi Jason

On 20.11.2015 22:15, Jason Mehrens wrote:
> Alexander,
>
> I see your point.  It is also out of spec for Handler.setFormatter to 
> actually call Formatter.setErrorManager.  What if there are subclasses of 
> formatter that exist today with a setErrorManager method?  Also not all 
> handlers have one formatter or actually call super.setFormatter since you 
> can't unseal the super handler (GAE).

Good point. You are right, modifying Handler.setErrorManager and
Handler.setFormatter methods by the way to call
Formatter.setErrorManger() we may introduce  undesirable behavior. For
example, if  Handler is configured with a custom Formatter with a custom
ErrorManger specified, we may spoil the ErrorManager in that Formatter.

Probably the better solution would be to specify the default
ErrorManager for a Formatter in the constructor of specific Handler. But
seems we can't just call getErrorManager() in ConsoleHandler
constructor, for example, as far as the subclass isn't fully initialized
yet.

>
> I still lean toward letting the exceptions fly since there is already an 
> expectation that formatters fail.  Is the concern that the handler will fail 
> or that the program will fail?
Well, actually my initial thought was to let the exceptions to be thrown
from Formatter to corresponding logging Handler where it should be
processed and reported to a registered ErrorManager. But we've given up
from this idea because:
- would require a CCC any way.
- would probably require a release note since it will alter the existing
behavior.
- the current implementation of ErrorManager will cause a
printStackTrace() - but only the first time such an error happens in
Handler. So that, the errors in Formatter will be swallowed any way if
some error happened before.
- this approach may cause an NPE if the concrete Handler subclass has
not installed any ErrorManager and does not override reportError. So,
there's the issue of potentially breaking existing code.

So, I was looking for a solution that shouldn't change any existing
behavior, shouldn't break a code, should work in all cases, should allow
to a Formatter report about its internal errors itself without relay on
the corresponding Handler where the Exception could be intentionally
ignored.

>
> I've recently been tinkering with a custom filter that internally uses a 
> formatter and have prototyped adding get/setErrorManager method to it and it 
> just becomes a mess because to have start thinking about more corner cases 
> and adding more to the spec.
What is wrong with get/serErrorManager for Formatter? It should be just
a way for Formatter to report its internal errors.

> I really think that if you are going to add the 3 ErrorManager methods you 
> should only add them to the LogManager (JDK-6865510) as a way to unify 
> failure reporting in the LogManager itself and to deal with failures inside 
> of error manger.  As in to remove that ugly System.err code in 
> Handler.reportError.  The LogManager would work kind of like 
> Thread.defaultUncaughtExceptionHandler and the Handler.errorManager is kind 
> of like the Thread.uncaughtException handler.
Well. I'm with you on this. I would prefer the fix is a part of more
wider enhancement that unified Logger's inner errors reporting inclusive
redesign for ErrorManager and so on. Probably it's not a big deal, but
it should be some JEP for JDK9. I'm afraid it's too late for 9.

> Then to fix this bug we could report it to via 
> LogManager.getLogManager().reportError.
Well, it's good solution as a default behavior if a user not specified
its own ErrorManager. But, I don't think we have to restrict a user with
what ErrorManager is u

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Ivan Gerasimov]

Great!  Now I like it the best.

Though, it may be better to get yet another pair of eyes.

One minor nit: In the tests, in the summary, it is written, "Test 
Integer.toString method*s*", but only one of the overloads is tested.


Sincerely yours,
Ivan

On 23.11.2015 18:08, Aleksey Shipilev wrote:

On 11/23/2015 04:34 PM, Ivan Gerasimov wrote:

With this fixed patch:
http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch
all tests from test/lang pass.

Would you give it another chance?

Okay, but this is better be the last non-cosmetic change to board this
departing train:
   http://cr.openjdk.java.net/~shade/8136500/webrev.05/

This passes java/lang, java/util tests, and performance improvement is
still very good.

Thanks,
-Aleksey

Re: RFR: jsr166 jdk9 integration wave 2

[Doug Lea]

On 11/23/2015 10:27 AM, Peter Levart wrote:


Sorry for confusion. I now noticed that Throwable.addSuppressed() and
getSuppressed() methods are actually synchronized! I don't know how I missed 
that...



And I don't know how you led me to forget that :-)


So above patch is not needed. Even printing uses getSuppressed() that returns a
snapshot. So the only thing remaining is the behavior that exceptions can change
while they are being observed.


Throwable.clone would allow a better solution, so especially if there
are any other use cases for it, I encourage you to pursue this.

-Doug

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Attila Szegedi]

Folks,

I integrated the changes Mandy suggested; please review these (build related) 
changes:
jdk9 top level: 
 
jdk9/jdk: 
 

For the sake of completeness, the jdk/nashorn changes are here: 
 but they have already 
been reviewed by Hannes and Sundar; only the above two (jdk9 and jdk9/jdk) have 
been modified compared to the original review request.

Sundar was kind enough to verify that JDK9 builds fine with these changes.

Thanks,
  Attila.

> On Nov 20, 2015, at 4:41 PM, Attila Szegedi  wrote:
> 
> Thanks for pointing out these, Mandy; I will make the changes you suggested.
> 
> Attila.
> 
>> On Nov 20, 2015, at 6:10 AM, Mandy Chung  wrote:
>> 
>> jdk.scripting.nashorn is loaded by the extension class loader.  Is 
>> jdk.dynalink expected to be loaded by the ext. class loader?
>> 
>> You need to edit this file to include the new module:
>>  jdk/make/src/classes/build/tools/module/ext.modules
>> 
>> This is an interim file to map modules to class loader and we will fix it 
>> when the changeset propagates to jake.
>> Mandy
>> 
>>> On Nov 19, 2015, at 7:00 PM, Mandy Chung  wrote:
>>> 
>>> I reviewed the top repo change.
>>> 
>>> modules.xml looks fine.
>>> 
>>> jdk.dynalink  should be in MAIN_MODULES since it has exported APIs.  
>>> jdk.scripting.nashorn should be moved too.  They are not sole service 
>>> providers.  Since you are on this file, can you move jdk.scripting.nashorn 
>>> to MAIN_MODULES as well?
>>> 
>>> Mandy
>>> 
 On Nov 19, 2015, at 3:15 PM, Attila Szegedi  wrote:
 
 Please review JDK-8141338 "Move jdk.internal.dynalink package to 
 jdk.dynalink" for . This 
 is basically the implementation step for integrating JEP 276. This 
 changeset will introduce a new public API that has CCC approval (request 
 8075866), and is also the implementation step of JEP 276 which is now 
 targeted for 9 and thus can be integrated.
 
 The changes in this changeset fall into several categories:
 - renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, 
 with ripple effects in Nashorn classes that import from these packages
 - changes to modules.xml and some build files to accommodate a new public 
 module and a dependency of Nashorn on it
 - new tests
 
 I’m sending this webrev to several lists with the following rationales:
 - nashorn-dev as the primary users and expected reviewers (also, the 
 Dynalink module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of 
 newly added test code was contributed by Sundar.
 - jigsaw-dev because of modules.xml changes
 - jdk9-dev for build changes (build file changes were graciously 
 contributed by Erik Joelsson and Sundar)
 - core-libs-dev since that’s the designated JEP 276 discussion list.
 
 Nashorn changes:  
 top-level jdk9 changes: 
 
 
 Thanks,
 Attila.
 
>>> 
>> 
> 

Re: RFR: jsr166 jdk9 integration wave 2

[Peter Levart]

Hi,


On 11/23/2015 03:12 PM, Doug Lea wrote:

On 11/23/2015 04:54 AM, Peter Levart wrote:



In CompletableFuture.uniWhenComplete method, the possible exception 
thrown from
BiConsumer action is added as suppressed exception to the exception 
of the
previous stage. This updated exception is then passed as completion 
result to
next stage. When previous stage is appended with more than one 
asynchronous

continuation:

...then both secondary exceptions are added as suppressed to the same 
primary

exception:

This is not nice for two reasons:

- Throwable::addSuppressed is not thread-safe
- The consumer of the result of one CompletableFuture can see the 
exceptional

result being modified as it observes it.



Thanks. The minimal solution is to lock, at least avoiding conflict
among multiple whenCompletes:

! else if (x != ex)
! x.addSuppressed(ex);

--- 771,781 

! else if (x != ex) {
! synchronized (x) {
! x.addSuppressed(ex);
! }
! }

This is not as good a solution as your proposal to add Throwable.clone(),
but we should do this until something like clone is in place.


Sorry for confusion. I now noticed that Throwable.addSuppressed() and 
getSuppressed() methods are actually synchronized! I don't know how I 
missed that...


So above patch is not needed. Even printing uses getSuppressed() that 
returns a snapshot. So the only thing remaining is the behavior that 
exceptions can change while they are being observed. If this is 
acceptable then this whole report of mine is just a bunch of misinformation.


Regards, Peter



When this stage is complete, the given action is invoked with the 
result (or
null if none) and the exception (or null if none) of this stage as 
arguments.
The returned stage is completed when the action returns. If the 
supplied action
itself encounters an exception, then the returned stage exceptionally 
completes

with this exception unless this stage also completed exceptionally."

Could specification be tweaked a bit? The last statement leaves it 
open to what

actually happens when "this stage also completes exceptionally".


The looseness was intentional. We'd like to improve debuggability of
implementations without strictly promising a particular form in 
interfaces.

This is similar to what's done in ForkJoinTask, where we try to
relay exception causes from other threads, but can't promise anything
beyond a plain exception.

-Doug

Re: RFR: jsr166 jdk9 integration wave 2

[Doug Lea]

On 11/23/2015 04:54 AM, Peter Levart wrote:



In CompletableFuture.uniWhenComplete method, the possible exception thrown from
BiConsumer action is added as suppressed exception to the exception of the
previous stage. This updated exception is then passed as completion result to
next stage. When previous stage is appended with more than one asynchronous
continuation:

...then both secondary exceptions are added as suppressed to the same primary
exception:

This is not nice for two reasons:

- Throwable::addSuppressed is not thread-safe
- The consumer of the result of one CompletableFuture can see the exceptional
result being modified as it observes it.



Thanks. The minimal solution is to lock, at least avoiding conflict
among multiple whenCompletes:

! else if (x != ex)
! x.addSuppressed(ex);

--- 771,781 

! else if (x != ex) {
! synchronized (x) {
! x.addSuppressed(ex);
! }
! }

This is not as good a solution as your proposal to add Throwable.clone(),
but we should do this until something like clone is in place.


When this stage is complete, the given action is invoked with the result (or
null if none) and the exception (or null if none) of this stage as arguments.
The returned stage is completed when the action returns. If the supplied action
itself encounters an exception, then the returned stage exceptionally completes
with this exception unless this stage also completed exceptionally."

Could specification be tweaked a bit? The last statement leaves it open to what
actually happens when "this stage also completes exceptionally".


The looseness was intentional. We'd like to improve debuggability of
implementations without strictly promising a particular form in interfaces.
This is similar to what's done in ForkJoinTask, where we try to
relay exception causes from other threads, but can't promise anything
beyond a plain exception.

-Doug

Re: RFR: 8140344: add support for 3 digit update release numbers

[Rob McKenna]

Thanks David, I'll mark this 9-na. (bcc'ing verona-dev)

Something similar was fixed in 7 
(https://bugs.openjdk.java.net/browse/JDK-8009634) - that may be what 
you're thinking of.


Aside from that are you ok with the changes?

-Rob

On 18/11/15 09:00, David Holmes wrote:

On 18/11/2015 1:30 AM, Rob McKenna wrote:

I'd expect this to be superseded by 223 completely but I've cc'd
verona-dev in case there are objections. At this point the fix is more
for the benefit of 7 & 8. I'd be happy to mark this 9-na if that makes
more sense?


I'd vote for a 7 (and 8?) specific fix. Didn't this already get fixed in 6.

David


 -Rob

On 17/11/15 15:19, Alan Bateman wrote:



On 17/11/2015 14:47, Rob McKenna wrote:

Hi folks,

With 7u hitting 101 shortly we've noticed that our Version parsing
infrastructure balks at the extra digit. With that in mind I'd like to
update the logic surrounding the parsing of version numbers and the
test itself to use regex instead of the less flexible
CharacterSequence method.

http://cr.openjdk.java.net/~robm/8140344/webrev.01/

Would it be better to bring this to verona-dev so that this can be
looked at in the context of the JEP 223 changes?

-Alan

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Ivan Gerasimov]

What sanity checks you had in mind? Because
java/lang/Integer/IntegerDecode.java fails, and it fails *in compiler*.

Oh, how embarrassing!  Overlooked a sign :(
I had tested it only with new lang/Integer/ToString.java, in a hope it 
gives enough coverage for sanity, but I was wrong.



I tried to see what might be wrong, but failed.

The mere fact the debugging is hard for this code tells me that we are
much better off having cleaner code that handles MIN_VALUE separately.
If you still want MIN_VALUE to go away, then file a follow up, and try
to disentangle those weird failures ;)


I don't think the change is too complicated.
It just moves all the calculations into negative area, so we only need 
to be careful with signs :)


With this fixed patch:
http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch
all tests from test/lang pass.

Would you give it another chance?


Also, with this kind of change, AbstractStringBuilders would *still*
have the MIN_VALUE checks, *plus* all the additional handling logic in
stringSize and getChars, that will run for positive values.
Incorporating this into ASB will result in removal of one if-statement 
altogether, so it seems to be a clear win.



Of course, it can be done later as a separate fix.

Sincerely yours,
Ivan


  You can
explore removing that as well, but it makes sense only after Indify
String Concat lands and brings its own stringSize/getChar usages (and
tests!) on the table.



Do you think it's worth it to reorganize the code this way?

No, I think the perfect is the enemy of done here.

Thanks,
-Aleksey

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Aleksey Shipilev]

On 11/23/2015 12:36 PM, Ivan Gerasimov wrote:
> A couple of concerns, though:
> 1)
> Wouldn't it be clearer to have "(byte)('0' + q);" instead of "(byte)(48
> + q); // 48 is ASCII '0'"?
> I see that '0' constant is used in arithmetic in some other places.

Yes, okay:
  http://cr.openjdk.java.net/~shade/8136500/webrev.04/

> 2)
> What still annoys me is the necessity to handle the MIN_VALUE separately.
> It seems to be possible to generalize the stringSize() and getChars() to
> handle this edge case as well, keeping the amount of arithmetic the same.
> 
> Under the link please find a patch, which does that for Integer.
> http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition.patch
> 
> The patch was created on top of the changes from your latest webrev.
> The sanity test passed, though it might be necessary to add some more
> testing.

What sanity checks you had in mind? Because
java/lang/Integer/IntegerDecode.java fails, and it fails *in compiler*.
I tried to see what might be wrong, but failed.

The mere fact the debugging is hard for this code tells me that we are
much better off having cleaner code that handles MIN_VALUE separately.
If you still want MIN_VALUE to go away, then file a follow up, and try
to disentangle those weird failures ;)

Also, with this kind of change, AbstractStringBuilders would *still*
have the MIN_VALUE checks, *plus* all the additional handling logic in
stringSize and getChars, that will run for positive values. You can
explore removing that as well, but it makes sense only after Indify
String Concat lands and brings its own stringSize/getChar usages (and
tests!) on the table.


> Do you think it's worth it to reorganize the code this way?

No, I think the perfect is the enemy of done here.

Thanks,
-Aleksey

Re: Throwable support for cloning: was Re: RFR: jsr166 jdk9 integration wave 2

[Peter Levart]

Even more backwards-compatible. It keeps current behavior for 
Throwable.clone() method if subclasses use it.


Just one static method and implemented interface need to be added:

public class Throwable implements Serializable, Cloneable {

/**
 * Clones given {@code exception} and returns it's clone so that it 
shares all
 * state with original exception (shallow clone) except for the 
possible list of already
 * {@link #addSuppressed(Throwable) added} {@link #getSuppressed() 
suppressed}
 * exceptions. The suppressed exception instances are not cloned, 
just the
 * list containing them. Further {@link #addSuppressed(Throwable) 
additions}

 * to the suppressed exceptions of the returned clone instance
 * don't affect the suppressed exceptions of original exception and 
vice versa.

 *
 * @param exception the exception to clone.
 * @paramthe type of exception
 * @return shallow clone of given exception with suppressed exception
 * list shallow-cloned
 * @since 1.9
 */
@SuppressWarnings("unchecked")
public static  T clone(T exception) {
try {
Throwable clone = (Throwable) exception.clone();
if (clone.suppressedExceptions != null &&
clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
clone.suppressedExceptions = new 
ArrayList<>(clone.suppressedExceptions);

}
return (T) clone;
} catch (CloneNotSupportedException e) {
throw new InternalError(e);
}
}


Regards, Peter

On 11/23/2015 12:07 PM, Peter Levart wrote:

Hi,

Until Throwable.addSuppressed() was added in JDK7 to support 
try-with-resources statement, Throwable has been a more-or-less 
immutable from the outside (except for initCause which is a one-of 
method meant to be called right after construction and before throwing 
and can't be called multiple times).


addSuppressed() is different. It allows a Throwable instance to be 
modified after it has been constructed and thrown. In 
try-with-resources, the caught exception from the body of 
try-with-resources statement is modified with possible exception 
thrown from the AutoCloesable.close(). This all happens in the same 
thread, so there's no problem. The final exception that is thrown from 
the method or handled (printed) contains all suppressed exceptions 
added so-far.


CompletionStage::whenComplete has been designed to act as a cleanup 
action equivalent to AutoCloseable.close() in try-with-resources. If 
cleanup action throws exception, it would be nice if it could be added 
to the exception of the completing stage as a suppressed exception. 
The problem with duplicating this behavior from try-with-resources is 
in the CompletionStage (CompletableFuture) design where it allows 
multiple continuations (cleanup actions for example) to be attached to 
a single completion stage. It would be desirable for those cleanup 
actions to not affect the exceptional result of the  stage they are 
appended to. There's also a problem if those continuations are 
asynchronous as they would execute Throwable::addSuppressed from 
multiple threads.


I suggest adding support for cloning the Throwable instances. It could 
be added in a backwards compatible way. The changes are very simple:


- add Cloneable to the implements clause of Throwable:

public class Throwable implements Serializable, Cloneable {

- add the following two methods to Throwable:

/**
 * Clones this exception so that it shares all state with original 
exception

 * (shallow clone) except for the possible list of already
 * {@link #addSuppressed(Throwable) added} {@link #getSuppressed() 
suppressed}
 * exceptions. The suppressed exception instances are not cloned, 
just the
 * list containing them. Further {@link #addSuppressed(Throwable) 
additions}
 * to the list of suppressed exceptions of the returned clone 
therefore
 * don't affect the original (this) suppressed exception list and 
vice versa.

 *
 * @return a shallow clone of this exception except for the 
suppressed exception

 * list which is shallow-cloned.
 * @throws CloneNotSupportedException never thrown, but declared 
to keep source
 *compatibility with possible 
subclasses
 *that declare that they are 
{@link Cloneable}
 *themselves and call {@code 
super.clone()}.

 * @since 1.9
 */
@Override
protected Object clone() throws CloneNotSupportedException {
Throwable clone = (Throwable) super.clone();
if (clone.suppressedExceptions != null &&
clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
clone.suppressedExceptions = new 
ArrayList<>(clone.suppressedExceptions);

}
return clone;
}

/**
 * Invokes protected {@link #clone()} on the passed-in

Throwable support for cloning: was Re: RFR: jsr166 jdk9 integration wave 2

[Peter Levart]

Hi,

Until Throwable.addSuppressed() was added in JDK7 to support 
try-with-resources statement, Throwable has been a more-or-less 
immutable from the outside (except for initCause which is a one-of 
method meant to be called right after construction and before throwing 
and can't be called multiple times).


addSuppressed() is different. It allows a Throwable instance to be 
modified after it has been constructed and thrown. In 
try-with-resources, the caught exception from the body of 
try-with-resources statement is modified with possible exception thrown 
from the AutoCloesable.close(). This all happens in the same thread, so 
there's no problem. The final exception that is thrown from the method 
or handled (printed) contains all suppressed exceptions added so-far.


CompletionStage::whenComplete has been designed to act as a cleanup 
action equivalent to AutoCloseable.close() in try-with-resources. If 
cleanup action throws exception, it would be nice if it could be added 
to the exception of the completing stage as a suppressed exception. The 
problem with duplicating this behavior from try-with-resources is in the 
CompletionStage (CompletableFuture) design where it allows multiple 
continuations (cleanup actions for example) to be attached to a single 
completion stage. It would be desirable for those cleanup actions to not 
affect the exceptional result of the  stage they are appended to. 
There's also a problem if those continuations are asynchronous as they 
would execute Throwable::addSuppressed from multiple threads.


I suggest adding support for cloning the Throwable instances. It could 
be added in a backwards compatible way. The changes are very simple:


- add Cloneable to the implements clause of Throwable:

public class Throwable implements Serializable, Cloneable {

- add the following two methods to Throwable:

/**
 * Clones this exception so that it shares all state with original 
exception

 * (shallow clone) except for the possible list of already
 * {@link #addSuppressed(Throwable) added} {@link #getSuppressed() 
suppressed}
 * exceptions. The suppressed exception instances are not cloned, 
just the
 * list containing them. Further {@link #addSuppressed(Throwable) 
additions}

 * to the list of suppressed exceptions of the returned clone therefore
 * don't affect the original (this) suppressed exception list and 
vice versa.

 *
 * @return a shallow clone of this exception except for the 
suppressed exception

 * list which is shallow-cloned.
 * @throws CloneNotSupportedException never thrown, but declared to 
keep source
 *compatibility with possible 
subclasses
 *that declare that they are 
{@link Cloneable}
 *themselves and call {@code 
super.clone()}.

 * @since 1.9
 */
@Override
protected Object clone() throws CloneNotSupportedException {
Throwable clone = (Throwable) super.clone();
if (clone.suppressedExceptions != null &&
clone.suppressedExceptions != SUPPRESSED_SENTINEL) {
clone.suppressedExceptions = new 
ArrayList<>(clone.suppressedExceptions);

}
return clone;
}

/**
 * Invokes protected {@link #clone()} on the passed-in {@code 
exception}

 * and returns the result.
 *
 * @param exception the exception to clone.
 * @paramthe type of exception
 * @return the result of {@link #clone()} invoked on the passed-in 
{@code exception}

 * @since 1.9
 */
@SuppressWarnings("unchecked")
public static  T clone(T exception) {
try {
return (T) exception.clone();
} catch (CloneNotSupportedException e) {
throw new InternalError(e);
}
}


I think that this addition would enable CompletableFuture to mimic the 
logic of to try-with-resources statement and might prove useful in other 
similar designs.



Regards, Peter


On 11/23/2015 10:54 AM, Peter Levart wrote:


On 11/16/2015 10:39 PM, Martin Buchholz wrote:

Smaller than wave 1, but still large.  Nothing like a looming deadline to
spur work ...

Oracle folks will need to help with API review.

https://bugs.openjdk.java.net/issues/?jql=(subcomponent%20%3D%20java.util.concurrent)%20AND%20status%20%3D%20%22In%20Progress%22%20ORDER%20BY%20updatedDate%20DESC
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

The primary focus is making jtreg tests more robust and faster.
It also contains the changes to j.u.c.atomic that Aleksey is waiting for.


Hi,

In CompletableFuture.uniWhenComplete method, the possible exception 
thrown from BiConsumer action is added as suppressed exception to the 
exception of the previous stage. This updated exception is then passed 
as completion result to next stage. When previous stage is appended 
with more than one asynchronous continuatio

Re: RFR [9] 8140687: Move @Contended to the jdk.internal.vm.annotation package

[Chris Hegarty]

Given John's comments, and the limitation of -XX:-RestrictContended,
adding an additional command line flag, -XaddExports, in 9 to access
@CS seems reasonable. I will proceed with this change as is.

  http://cr.openjdk.java.net/~chegar/8140687/00/

-Chris.


On 13/11/15 00:08, John Rose wrote:

On Nov 12, 2015, at 5:48 AM, Vitaly Davidovich  wrote:





There's a misunderstanding here.  Sadly, it relates to security policy.

The shadowy figures with the liberal usage of sharp-edged features
are not dumb users, nor are they smart users like you whom we
unaccountably view as "dumb and misinformed", but black hat
attackers (or security researchers of any stripe), who take sharp
stuff like that and use it backwards and upside down, in ways neither
dumb nor smart users would ever dream of, to coax the JVM into
disallowed states.

The overflow Paul refers to would not be a heap OOM but (hypothetically)
size indicators in the implementation of the JVM.  Adding @C to the public
mix gives a determined attacker 3 more bits of dynamic range to maximum
instance sizes.

I personally don't think there is any specific way to weaponize that, but I do 
know,
from bitter experience, that some such expansions produce bugs.  (Think 16-bit
overflow:  It has happened, it hurts when it happens.)  Adding @C to the public
mix means we have to race the black hats to find any bug tail due to the extra
degrees of freedom in instance layout.  It's not a race I want to run or need 
to run,
so we are not making @C public.

As we do value types we are having to open up object layout to many more
degrees of freedom.  This will be carefully reviewed and stress-tested.  At that
point it will be very safe to add other layout hacks like @C.  In fact, it is 
likely
that we will be able to factor field-semantic hacks like @C (and WeakReference
and Optional and various other interesting variable types) into value type
wrappers of the base type.

For now, sorry.  Please use the -XX and -Xbcp thingies as workarounds.

— John

Re: RFR: jsr166 jdk9 integration wave 2

[Peter Levart]

On 11/16/2015 10:39 PM, Martin Buchholz wrote:

Smaller than wave 1, but still large.  Nothing like a looming deadline to
spur work ...

Oracle folks will need to help with API review.

https://bugs.openjdk.java.net/issues/?jql=(subcomponent%20%3D%20java.util.concurrent)%20AND%20status%20%3D%20%22In%20Progress%22%20ORDER%20BY%20updatedDate%20DESC
http://cr.openjdk.java.net/~martin/webrevs/openjdk9/jsr166-jdk9-integration/

The primary focus is making jtreg tests more robust and faster.
It also contains the changes to j.u.c.atomic that Aleksey is waiting for.


Hi,

In CompletableFuture.uniWhenComplete method, the possible exception 
thrown from BiConsumer action is added as suppressed exception to the 
exception of the previous stage. This updated exception is then passed 
as completion result to next stage. When previous stage is appended with 
more than one asynchronous continuation:


CompletableFuture cf0 = new CompletableFuture<>();

CompletableFuture cf1 = cf0.whenCompleteAsync((v, x) -> {
throw new RuntimeException("Secondary 1");
});

CompletableFuture cf2 = cf0.whenCompleteAsync((v, x) -> {
throw new RuntimeException("Secondary 2");
});

cf0.completeExceptionally(new RuntimeException("Primary"));

try {
cf1.get();
} catch (Exception e) {
System.err.println("\ncf1 exception:\n");
e.printStackTrace();
}

try {
cf2.get();
} catch (Exception e) {
System.err.println("\ncf2 exception:\n");
e.printStackTrace();
}


...then both secondary exceptions are added as suppressed to the same 
primary exception:



cf1 exception:

java.util.concurrent.ExecutionException: java.lang.RuntimeException: Primary
at 
java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:386)
at 
java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1948)

at CFTest.main(CFTest.java:22)
Caused by: java.lang.RuntimeException: Primary
at CFTest.main(CFTest.java:19)
Suppressed: java.lang.RuntimeException: Secondary 2
at CFTest.lambda$main$1(CFTest.java:16)
at 
java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:795)
at 
java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:771)
at 
java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:478)
at 
java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:281)
at 
java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1149)
at 
java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1985)
at 
java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1933)
at 
java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157) 


Suppressed: java.lang.RuntimeException: Secondary 1
at CFTest.lambda$main$0(CFTest.java:12)
at 
java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:795) 

at 
java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:771) 

at 
java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:478) 

at 
java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:281)
at 
java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1149)
at 
java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1985)
at 
java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1933)
at 
java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:157)


cf2 exception:

java.util.concurrent.ExecutionException: java.lang.RuntimeException: Primary
at 
java.util.concurrent.CompletableFuture.reportGet(CompletableFuture.java:386)
at 
java.util.concurrent.CompletableFuture.get(CompletableFuture.java:1948)

at CFTest.main(CFTest.java:29)
Caused by: java.lang.RuntimeException: Primary
at CFTest.main(CFTest.java:19)
Suppressed: java.lang.RuntimeException: Secondary 2
at CFTest.lambda$main$1(CFTest.java:16)
at 
java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:795)
at 
java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:771)
at 
java.util.concurrent.CompletableFuture$Completion.exec(CompletableFuture.java:478)
at 
java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:281)
at 
java.util.concurrent.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:1149)
at 
java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1985)
at

Re: RFR (S) 8136500: Integer/Long getChars and stringSize should be more idiomatic

[Ivan Gerasimov]

Hi Aleksey!

I think the code looks much better now!

A couple of concerns, though:
1)
Wouldn't it be clearer to have "(byte)('0' + q);" instead of "(byte)(48 
+ q); // 48 is ASCII '0'"?

I see that '0' constant is used in arithmetic in some other places.

2)
What still annoys me is the necessity to handle the MIN_VALUE separately.
It seems to be possible to generalize the stringSize() and getChars() to 
handle this edge case as well, keeping the amount of arithmetic the same.


Under the link please find a patch, which does that for Integer.
http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition.patch

The patch was created on top of the changes from your latest webrev.
The sanity test passed, though it might be necessary to add some more 
testing.


Do you think it's worth it to reorganize the code this way?

Sincerely yours,
Ivan


On 22.11.2015 22:59, Aleksey Shipilev wrote:

On 11/22/2015 01:55 AM, Ivan Gerasimov wrote:

Second, I think, the code of stringSize() might be better inlined, if it
was used only once.

No-no! That's a helpful little fella. Not-yet-integrated Indify String
Concat uses Integer/Long.stringSize.

I didn't really propose to manually inline the stringSize() into the
calling code.
What I was trying to say was that if we replace the code "int size = (i
< 0) ? stringSize(-i) + 1 : stringSize(i)", which calls stringSize()
twice, with something, which calls it only once, it might be easier for
compiler to inline it.

Sorry, I didn't make it clear :)

Ah, that "only once" confused me. Yes, that makes sense. In fact, I
think we should just soak the negative check into the stringSize, given
that all the usages are doing the checks externally. This exposes the
MIN_VALUE checks back, but it seems a fair trade for cleaner code; the
performance is still on par with previous revisions. And, this also
caters for AbstractStringBuilders, as you and Fabian wanted.

See:
  http://cr.openjdk.java.net/~shade/8136500/webrev.03/

Thanks,
-Aleksey

Re: Review request for JDK-8141338: Move jdk.internal.dynalink package to jdk.dynalink

[Hannes Wallnoefer]

+1 for the Nashorn/Dynalink changes.

The top level changes look good to me but I'd prefer to leave reviews to 
those who are more familiar with the build/modules infrastructure.


Hannes

Am 2015-11-20 um 00:15 schrieb Attila Szegedi:

Please review JDK-8141338 "Move jdk.internal.dynalink package to jdk.dynalink" for 
. This is basically the implementation 
step for integrating JEP 276. This changeset will introduce a new public API that has CCC 
approval (request 8075866), and is also the implementation step of JEP 276 which is now 
targeted for 9 and thus can be integrated.

The changes in this changeset fall into several categories:
- renaming of jdk.internal.dynalink.* package to jdk.dynalink.* package, with 
ripple effects in Nashorn classes that import from these packages
- changes to modules.xml and some build files to accommodate a new public 
module and a dependency of Nashorn on it
- new tests

  I’m sending this webrev to several lists with the following rationales:
- nashorn-dev as the primary users and expected reviewers (also, the Dynalink 
module code lives in jdk9/nashorn/src/jdk.dynalink). A lot of newly added test 
code was contributed by Sundar.
- jigsaw-dev because of modules.xml changes
- jdk9-dev for build changes (build file changes were graciously contributed by 
Erik Joelsson and Sundar)
- core-libs-dev since that’s the designated JEP 276 discussion list.

Nashorn changes: 
top-level jdk9 changes: 


Thanks,
   Attila.

Re: RFR JDK-8143330: No appropriate CCC request for two new protected methods added in JDK9b93 by JDK-8141132

[Tobias Hartmann]

Hi,

looks good to me.

Best,
Tobias

On 20.11.2015 19:41, Xueming Shen wrote:
> 
> I missed the override version in StringBuffer.java, thanks
> Tobias for catching it.
> 
> http://cr.openjdk.java.net/~sherman/8143553
> https://bugs.openjdk.java.net/browse/JDK-8143553
> 
> I did re-generate the api doc, but still missed it :-(
> 
> Thanks,
> Sherman
> 
> 
> On 11/19/2015 12:50 PM, Xueming Shen wrote:
>> Thanks Joe, Alan! The synopsis has been updated accordingly.
>>
>> On 11/19/2015 12:43 PM, Alan Bateman wrote:
>>>
>>>
>>> On 19/11/2015 20:39, joe darcy wrote:
 Looks good Sherman; thanks,

>>> Looks okay to me too. We should fix the synopsis on the JIRA issue or at 
>>> least put a better message in the change-set comment.
>>>
>>> -Alan.
>>
> 

Re: JDK 9 RFR of JDK-8143583: Several tests don't work with latest jtreg due to non-existing files in @build

[Alan Bateman]

On 23/11/2015 07:10, Amy Lu wrote:
Below tests failed with latest nightly jtreg due to non-existing files 
in @build


com/sun/management/HotSpotDiagnosticMXBean/DumpHeap.java
sun/tools/jmap/BasicJMapTest.java
com/sun/jdi/DoubleAgentTest.java
com/sun/jdi/SuspendNoFlagTest.java

Please review this patch to fix the typo in @build

bug: https://bugs.openjdk.java.net/browse/JDK-8143583
webrev: http://cr.openjdk.java.net/~amlu/8143583/webrev.00/

Looks good.

-Alan