Re: RFR:JDK-8133079:java.time LocalDate and LocalTime ofInstant() factory methods

[Stephen Colebourne]

Looks good to me.
Need to ensure that JDK-8030864 also gets tackled, as it is the
inverse operation of these new methods.
thanks
Stephen


On 12 November 2015 at 06:40, nadeesh tv  wrote:
> Hi all,
>
> Please review a fix for
>
> Bug Id - https://bugs.openjdk.java.net/browse/JDK-8133079
>
> Enhancement -  Enhance LocalDate and LocalTime by adding .ofInstant(Instant,
> ZoneId)
>
> webrev - http://cr.openjdk.java.net/~ntv/8133079/webrev.01/
>
> --
> Thanks and Regards,
> Nadeesh TV
>

Re: RFR:JDK-8133079:java.time LocalDate and LocalTime ofInstant() factory methods

[nadeesh tv]

Hi Stephen,
Yes, I will send out a separate RFR for  JDK-8030864
Regards,
Nadeesh
On 11/12/2015 7:11 PM, Stephen Colebourne wrote:

Looks good to me.
Need to ensure that JDK-8030864 also gets tackled, as it is the
inverse operation of these new methods.
thanks
Stephen


On 12 November 2015 at 06:40, nadeesh tv  wrote:

Hi all,

Please review a fix for

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

Enhancement -  Enhance LocalDate and LocalTime by adding .ofInstant(Instant,
ZoneId)

webrev - http://cr.openjdk.java.net/~ntv/8133079/webrev.01/

--
Thanks and Regards,
Nadeesh TV



--
Thanks and Regards,
Nadeesh TV

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Claes Redestad]

On 2015-11-12 01:10, John Rose wrote:

For better maintainability, I think the zero and identity forms should be 
created together, not in separate twin code blocks.

I think this is a safer, saner way to inject laziness here:

- private static void createIdentityForms() {
+ // Create LF_zero, LF_identity, etc., for the given type.
+ private static void createIdentityForms(BasicType type) {

It means that groups of LFs get lazily created; if that is tolerable for the 
present purpose, it's easier to reason about.

— John


How about this:

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

I'm seeing no increase in number of LambdaForms or NamedFunctions 
created by merging it all back into one method, while getting rid of 
MN_* arrays.


I fixed the obvious mistake in sun.invoke.util.WrapperTest w.r.t 
byte/char, and since these tests fail against JDK-8141678 I wouldn't 
mind keeping it around.


/Claes

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

Hi Roger,

I want to keep this review focused and consistent with the existing methods 
that were previously reviewed.


> On 12 Nov 2015, at 17:42, Roger Riggs  wrote:
> 
> Hi Paul,
> 
> - A minor point but in the argument names, its a bit inconsistent between 
> using 'b' and 'a2'.
> I would have use 'a', and 'b' to be more consistent.  (and yes the current 
> methods show the same inconsistency)
> 

Perhaps, but i don’t want to do that right now.


> - The @return(s) should have an 'otherwise {@code false};  otherwise maybe it 
> is allowed to return true.
> 

The current approach is consistent with the existing equals methods.


> - In the implementation, I generally prefer the form of 
> Objects.requireNonNull(o, "name") so the exception
> identifies the offending argument.
> 
> - I thought the coding style has spaces around operators (like ==).
> 

Same as in other equals methods.


> - The non-range checked version of equals should have the same behavior as:
>   equals(a, 0, a.length, a2, 0, a2.length).
> 
> The range checked version should include the sentence:
> 
> "Also, two array references are  considered equal if both are {@code null}.”
> 

Whats the sub-range of a null array reference?

See the existing sub-range equals methods. This was discussed in the original 
review and i don’t want to reopen that decision right now.


> - The implementation will need to check for nulls before the rangeChecks.
> 

The access to the length will induce an NPE. See other methods in arrays (such 
as binarySearch)

Paul.

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

[Paul Sandoz]

> On 12 Nov 2015, at 14:48, Vitaly Davidovich  wrote:
> 
> Hi Paul,
> 
> There is a very valid concern, since @Contended changes object layout and 
> increases object size, liberal use might tickle an overflow in HotSpot code. 
> Hence why it has remained internal so far.
> 
> What overflow? OOM of the heap? How is that a "very valid" concern? Why would 
> it be used liberally? Why are you allowing users to allocate any memory 
> whatsoever? Why not put a cap on the size of objects non-JDK code can 
> allocate? :) I mean seriously, with all due respect, the occasional "our 
> users are dumb and misinformed" sentiment is very off-putting.
> 

Where did i say that? You are incorrectly reading between the lines. The 
concern is actually smart and well informed users that may dliberately exploit 
a bug/weakness in the VM.

Paul.

> If you think there are, currently, *implementation* issues with how 
> @Contended is handled in the VM which cannot be addresses, I'll buy that.  
> But can we please stop with the dumbing down? :)
> 

Re: RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Paul Sandoz]

> On 12 Nov 2015, at 15:50, Roger Riggs  wrote:
> 
> Hi Paul,
> 
> 
> On 11/12/2015 9:45 AM, Paul Sandoz wrote:
>> One more thing i forgot to mention, if there is ever a chance that 
>> ProcessBuilder is changed from final to non-final we should List> ProcessBuilder>.
> I don't see that happening.
> 
> If so would the change from List to List ProcessBuilder>
> should be source and binary compatible. And therefore could be made at the 
> same time (and not now).
> 

Oh yes, i forgot it is a static method.

Paul.

Re: [RFR]: 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

[Miroslav Kos]

.. anyone?

On 10/11/15 15:31, Miroslav Kos wrote:

Ping ... Would somebody find time for this one?

Thanks
M.


On 26/10/15 13:59, Miroslav Kos wrote:

Hi everybody,

I'd like to ask you for a review for
8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

It is about changes in pluggability layer - there was a proprietary 
service loading implementation which was not fully compatible with to 
use java.util.ServiceLoader facility. There are similar changes in 
all JAX-* APIs specs - JAX-B,  JAX-WS and SAAJ.


JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/
specdiff: http://cr.openjdk.java.net/~mkos/8131334/specdiff.09/

Testing - there are tests in standalone project to ensure there that 
the old behavior is still supported.


Thanks
Miran

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

[Doug Lea]

On 11/12/2015 08:23 AM, Paul Sandoz wrote:

Hi,

I don’t think anyone disagrees on the usefulness of @Contended.

Beyond the JDK and 166 are there any more usages out there in the wild?



My take after browsing through some of these external libraries is that
most people still use non-portable (but usually effective on common
platforms) manual padding hacks. I imagine that they do this so that
the code does not break on pre-jdk8. Given how this is turning out,
they will have no motivation to change what they do until jdk10 or so.
Sigh.

-Doug

Re: Code Review for JEP 259: Stack-Walking API

[David M. Lloyd]

One other kind of stack metadata that is missing (yet is present with 
external tools such as jstack etc.) is ancillary data about stack frames 
indicating whether the frame holds or is waiting on a lock.  I'm not 
sure whether it's practical to add all this stuff though, 
performance-wise, unless maybe it is made optional with an option flag.


On 11/12/2015 09:29 AM, Jason Mehrens wrote:

Mandy,

Forgive my ignorance on this subject and impacts but, would it be possible to 
include more metadata methods in the StackWalker.StackFrame interface such as 
getParameterTypes/parameterArray and getReturnType/returnType?  Ideally, I 
really would like these methods for StackTraceElement since knowing the exact 
parameter types allows the caller to lookup the method to examine all the 
method metadata which is useful for logging not only in output but for use in 
stacktrace reduction algorithms.

Thanks,

Jason


From: core-libs-dev  on behalf of Mandy Chung 

Sent: Monday, November 9, 2015 8:32 PM
To: core-libs-dev; hotspot-runtime-...@openjdk.java.net
Subject: Code Review for JEP 259: Stack-Walking API

javadoc:

http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html

webrev:
   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/

Overview of the implementation:
When stack walking begins, the StackWalker calls into the VM to anchor a native 
frame (callStackWalk) that will fetch the first batch of stack frames.  VM will 
then invoke the doStackWalk method so that the consumer starts getting 
StackFrame object for each frame.  If all frames in the current batch are 
traversed, it will ask the VM to fetch the next batch.  The library side is 
doing the filtering of reflection frames.   For this patch, the VM filters of 
the hidden frames and also filter out Throwable::init related frames for stack 
trace.

Ultimately we will move to these built-in logic out from the VM to the library 
but I’d like to separate them as future works.

This patch also includes the change for Throwable to use StackWalker but it’s 
disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The VM 
backtrace is well tuned for performance.  So we separate the switch of 
Throwable to use StackWalker as a follow-on work:
JDK-8141239 Throwable should use StackWalker API to capture the backtrace

MemberName initialization is one source of overhead and we propose to keep the 
VM flag -XX:-MemberNameInStackFrame for the time being for the performance work 
to continue for JDK-8141239.

Thanks
Mandy



--
- DML

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

Hi,

On 2015-11-12 19:10, Paul Sandoz wrote:

Hi Peter,

This stuff always gives me a headache :-)


Yes. :-)



IIUC it’s all idempotent stuff, and the final field in NamedFunction should 
take care of certain things.


That's been my understanding, as well.



Claes, was it intentional that you call function.resolve() after the array 
store? You might need to reverse that and place a Unsafe.storeFence between 
them if it is required that the published and visible function be resolved.


This however was unintentional, and I think you're right that this would 
ensure visibility:


function.resolve();
UNSAFE.storeFence();
FUNCTIONS[idx] = function;

http://cr.openjdk.java.net/~redestad/8142334/webrev.04/

/Claes



Paul.


>On 12 Nov 2015, at 17:26, Peter Levart  wrote:
>
>Hi Claes,
>
>I have one concern...
>
>645 private static NamedFunction getConstantFunction(int idx) {
>646 NamedFunction function = FUNCTIONS[idx];
>647 if (function != null) {
>648 return function;
>649 }
>650 return setCachedFunction(idx, makeConstantFunction(idx));
>651 }
>652
>653 private static synchronized NamedFunction setCachedFunction(int idx, 
final NamedFunction function) {
>654 // Simulate a CAS, to avoid racy duplication of results.
>655 NamedFunction prev = FUNCTIONS[idx];
>656 if (prev != null) {
>657 return prev;
>658 }
>659 FUNCTIONS[idx] = function;
>660 function.resolve();
>661 return function;
>662 }
>
>
>Above is a classical double-checked locking idiom, but it is not using 
volatile variable to publish the NamedFunction instance. I wonder if this is safe. 
Even if the FUNCTIONS[idx] slot was a volatile variable,  you would publish new 
instance before resolving it. Is it OK to publish unresolved NamedFunction(s)? 
There is a NamedFunction.resolvedHandle() instance method that makes sure 
NamedFunction is resolved before returning a MethodHandle, but there are also 
usages of dereferencing NamedFunction.resolvedHandle field directly in code. Are 
you sure that such unresolved or almost resolved instance of NamedFunction is 
never used in such places where NamedFunction.resolvedHandle field is dereferenced 
directly?
>
>In original code those NamedFunctions were resolved in static initializer so 
they were published properly.
>
>Regards, Peter

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

On 2015-11-12 19:53, Peter Levart wrote:


Do you think this would work correctly w.r.t visibility:

FUNCTIONS[idx] = function;
function.resolve();
UNSAFE.storeFence();


This does not do anything useful.

What happens if you simply omit function.resolve() call. If lazy 
resolving works and tests pass, then perhaps it's all ok.


Peter


Actually, I simply tried backing out the DirectMethodHandle changes 
entirely, and measured how much we'd lose by leaving that particular 
case alone: almost nothing, as it turns out (same number of LFs are 
generated etc). I think the safest option is to leave DirectMethodHandle 
alone:


http://cr.openjdk.java.net/~redestad/8142334/webrev.05/

/Claes

RFR (JAXP) : 8142900: Xerces Update: Xerces XPath

[huizhe wang]

Hi,

The purpose of the update was to fix the JCK issue 8069052[1], which 
demanded the acceptance of a full character list in reZ003v.xml as XML 
Schema defined words. However, since b74, the JDK has been updated to 
support Unicode 7.0.0[2], in particular, U+2308..U+230B were changed 
from Sm to either Ps or Pe in 7.0.0.  As a result, I had to create a 
separate issue 8142900 [3] so that we can continue fixing the issue that 
the XML Schema specification defines \w as 
[#x-#x10]-[\p{P}\p{Z}\p{C}] (all characters except the set of 
"punctuation", "separator" and "other" characters).


Once this is fixed, we'll discuss the JCK issue that now contains two 
extra characters.


[1] https://bugs.openjdk.java.net/browse/JDK-8069052
[2] https://bugs.openjdk.java.net/browse/JDK-8032446
[3] https://bugs.openjdk.java.net/browse/JDK-8142900

Webrev:
http://cr.openjdk.java.net/~joehw/jdk9/8142900/webrev/

Thanks,
Joe

Re: 8142493: Utility methods to check indexes and ranges doesn't specify behavior when function produces null

[Lance @ Oracle]

Looks ok Paul


Best
Lance


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

> On Nov 12, 2015, at 6:19 AM, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8142493-checkIndex-function-return-null/webrev/
>  
> 
> 
> This catches some edge cases for the exception mapping function passed to the 
> Objects::checkIndex* methods, specifically if the function returns null or 
> throws an exception.
> 
> Paul.

8142493: Utility methods to check indexes and ranges doesn't specify behavior when function produces null

[Paul Sandoz]

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8142493-checkIndex-function-return-null/webrev/
 


This catches some edge cases for the exception mapping function passed to the 
Objects::checkIndex* methods, specifically if the function returns null or 
throws an exception.

Paul.

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Paul Sandoz]

Hi,

I have made a retreat on this just to add the two missing equals methods:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/
 


I propose to revisit the overall null-handling semantics, especially for 
Comparable[] accepting methods, at a later date.

Paul.

> On 4 Nov 2015, at 15:32, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/
>  
> 
>  https://bugs.openjdk.java.net/browse/JDK-8141409 
> 
> 
> This builds on (the already reviewed):
> 
>  https://bugs.openjdk.java.net/browse/JDK-8033148 
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8033148-Arrays-lexico-compare/webrev/
>  
> 
> 
> and adds yet more methods to Arrays, only two this time though, to fill out 
> the missing gaps related to array equality with a comparator.
> 
> In addition i added an Objects.compare method, which simplifies the 
> implementations of some object-bearing methods.
> 
> Both additions simplify the specification some object-bearing methods.
> 
> Paul.

Re: [RFR]: 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

[Lance @ Oracle]

Hi,

Overall it looks ok,

I would however suggest adding a couple of tests to the jdk that run with/out a 
security manager as a sanity check.

Best
Lance 


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

> On Nov 12, 2015, at 3:55 AM, Miroslav Kos  wrote:
> 
> .. anyone?
> 
>> On 10/11/15 15:31, Miroslav Kos wrote:
>> Ping ... Would somebody find time for this one?
>> 
>> Thanks
>> M.
>> 
>> 
>>> On 26/10/15 13:59, Miroslav Kos wrote:
>>> Hi everybody,
>>> 
>>> I'd like to ask you for a review for
>>> 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader
>>> 
>>> It is about changes in pluggability layer - there was a proprietary service 
>>> loading implementation which was not fully compatible with to use 
>>> java.util.ServiceLoader facility. There are similar changes in all JAX-* 
>>> APIs specs - JAX-B,  JAX-WS and SAAJ.
>>> 
>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
>>> webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/
>>> specdiff: http://cr.openjdk.java.net/~mkos/8131334/specdiff.09/
>>> 
>>> Testing - there are tests in standalone project to ensure there that the 
>>> old behavior is still supported.
>>> 
>>> Thanks
>>> Miran
>>> 
>> 
> 

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

[Fabian Lange]

Hi,

Without some good argument against it, I think the only issue
> is where (what package) to place it. Carrying it out from there seems
> identical to your webrev diffs, just changing import statements
> and the like.
>

Doug, as you mention java.util.concurrent. I think that should be the place
for a concurrency support annotation. I wouldn't necessarily agree that
this is an esotheric annotation. I could argue that writing efficient
multithreaded programs is more common than any UI or even sound related
functionality in the JDK.
@Contended is important for making the JDK a tool for writing fast code. I
was so happy to see it come, and would be sad to see it go.

Fabian

Re: RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Paul Sandoz]

> On 11 Nov 2015, at 19:11, Roger Riggs  wrote:
> 
> Hi Paul,
> 
> Updated Webrev to use List instead of ProcessHandle... varargs.
> 
>   http://cr.openjdk.java.net/~rriggs/webrev-pipeline-8132394/
> 

+1

Paul.

Re: [RFR]: 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

[Lance Andersen]

Not sure exactly what you want to set, but you can set system properties or run 
tests from a script or from testNG

For running a script, see http://openjdk.java.net/jtreg/vmoptions.html

Best
Lance
On Nov 12, 2015, at 8:28 AM, Miroslav Kos  wrote:

> I have a set of tests in standalone project (using bash scripts), but the 
> problem is that to test the correct behavior, configuring of jdk is necessary 
> (property file) - have no idea if something like this is possible with jtreg 
> - are there any similat tests already?
> 
> Thanks
> Miran
> 
> On 12/11/15 14:01, Lance @ Oracle wrote:
>> Hi,
>> 
>> Overall it looks ok,
>> 
>> I would however suggest adding a couple of tests to the jdk that run 
>> with/out a security manager as a sanity check.
>> 
>> Best
>> Lance 
>> 
>> 
>> Lance Andersen| Principal Member of Technical Staff | +1.781.442.2037
>> Oracle Java Engineering 
>> 1 Network Drive
>> Burlington, MA 01803
>> lance.ander...@oracle.com
>> Sent from my iPad
>> 
>> On Nov 12, 2015, at 3:55 AM, Miroslav Kos  wrote:
>> 
>>> .. anyone?
>>> 
>>> On 10/11/15 15:31, Miroslav Kos wrote:
 Ping ... Would somebody find time for this one?
 
 Thanks
 M.
 
 
 On 26/10/15 13:59, Miroslav Kos wrote:
> Hi everybody,
> 
> I'd like to ask you for a review for
> 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader
> 
> It is about changes in pluggability layer - there was a proprietary 
> service loading implementation which was not fully compatible with to use 
> java.util.ServiceLoader facility. There are similar changes in all JAX-* 
> APIs specs - JAX-B,  JAX-WS and SAAJ.
> 
> JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
> webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/
> specdiff: http://cr.openjdk.java.net/~mkos/8131334/specdiff.09/
> 
> Testing - there are tests in standalone project to ensure there that the 
> old behavior is still supported.
> 
> Thanks
> Miran
> 
 
>>> 
> 



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: 8142334: Improve lazy initialization of java.lang.invoke

[Paul Sandoz]

> On 11 Nov 2015, at 15:32, Claes Redestad  wrote:
> 
> Paul,
> 
> On 2015-11-10 11:55, Paul Sandoz wrote:
>> DirectMethodHandle
>> —
>>  682 private static @Stable NamedFunction[] FUNCTIONS = new 
>> NamedFunction[NF_LIMIT];
>> 
>> Invokers
>> —
>>  442 private static @Stable NamedFunction[] FUNCTIONS = new 
>> NamedFunction[NF_LIMIT];
>> 
>> MethodHandleImpl
>> —
>> 1627 private static @Stable NamedFunction[] FUNCTIONS = new 
>> NamedFunction[NF_LIMIT];
>> 
>> 
>> To be complete you could add “final”, thus it makes it clear that @Stable 
>> refers specifically to the array element.
>> 
>> Paul.
> 
> Thanks for having a look and catching this:
> 
> http://cr.openjdk.java.net/~redestad/8142334/webrev.03
> 
> - added final keyword to FUNCTIONS and HANDLES
> - added @Stable to ARRAYS, FILL_ARRAYS, and FILL_ARRAY_TO_RIGHT
> 

MethodHandleImpl.java
—

1413 private static final @Stable MethodHandle[] FILL_ARRAYS = new 
MethodHandle[FILL_ARRAYS_COUNT + 1];
1414
1415 private static MethodHandle getFillArray(int count) {
1416 assert (count > 0 && count <= FILL_ARRAYS_COUNT);

Why FILL_ARRAYS_COUNT + 1 rather than FILL_ARRAYS_COUNT?

Based on the previous code I would have expected the bounds to be:

  0 < count < FILL_ARRAYS_COUNT

Paul.

Re: RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Roger Riggs]

Hi Paul,


On 11/12/2015 9:45 AM, Paul Sandoz wrote:
One more thing i forgot to mention, if there is ever a chance that 
ProcessBuilder is changed from final to non-final we should Listextends ProcessBuilder>.

I don't see that happening.

If so would the change from List to ListProcessBuilder>
should be source and binary compatible. And therefore could be made at 
the same time (and not now).


Roger

Re: [RFR]: 8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

[Miroslav Kos]

I have a set of tests in standalone project (using bash scripts), but 
the problem is that to test the correct behavior, configuring of jdk is 
necessary (property file) - have no idea if something like this is 
possible with jtreg - are there any similat tests already?


Thanks
Miran

On 12/11/15 14:01, Lance @ Oracle wrote:

Hi,

Overall it looks ok,

I would however suggest adding a couple of tests to the jdk that run 
with/out a security manager as a sanity check.


Best
Lance


Lance 
Andersen| Principal Member of Technical Staff | +1.781.442.2037 


Oracle Java Engineering
1 Network Drive 
Burlington, MA 01803 
lance.ander...@oracle.com 
Sent from my iPad

On Nov 12, 2015, at 3:55 AM, Miroslav Kos > wrote:



.. anyone?

On 10/11/15 15:31, Miroslav Kos wrote:

Ping ... Would somebody find time for this one?

Thanks
M.


On 26/10/15 13:59, Miroslav Kos wrote:

Hi everybody,

I'd like to ask you for a review for
8131334: SAAJ Plugability Layer: using java.util.ServiceLoader

It is about changes in pluggability layer - there was a proprietary 
service loading implementation which was not fully compatible with 
to use java.util.ServiceLoader facility. There are similar changes 
in all JAX-* APIs specs - JAX-B,  JAX-WS and SAAJ.


JBS: https://bugs.openjdk.java.net/browse/JDK-8131334
webrev: http://cr.openjdk.java.net/~mkos/8131334/jaxws.01/ 

specdiff: http://cr.openjdk.java.net/~mkos/8131334/specdiff.09/ 



Testing - there are tests in standalone project to ensure there 
that the old behavior is still supported.


Thanks
Miran

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

[Vitaly Davidovich]

Hi Paul,


> There is a very valid concern, since @Contended changes object layout and
> increases object size, liberal use might tickle an overflow in HotSpot
> code. Hence why it has remained internal so far.


What overflow? OOM of the heap? How is that a "very valid" concern? Why
would it be used liberally? Why are you allowing users to allocate any
memory whatsoever? Why not put a cap on the size of objects non-JDK code
can allocate? :) I mean seriously, with all due respect, the occasional
"our users are dumb and misinformed" sentiment is very off-putting.

If you think there are, currently, *implementation* issues with how
@Contended is handled in the VM which cannot be addresses, I'll buy that.
But can we please stop with the dumbing down? :)

On Thu, Nov 12, 2015 at 8:23 AM, Paul Sandoz  wrote:

> Hi,
>
> I don’t think anyone disagrees on the usefulness of @Contended.
>
> Beyond the JDK and 166 are there any more usages out there in the wild?
>
> I am not aware of any, grepcode does not report any usages outside of the
> JDK, but we may have missed them. The current set of proposed critical
> internal APIs have been derived from analyses of various collections of
> code. This may in part be because it was only introduced in Java 8.
>
> Note that as currently proposed @Contended is not going away, it would
> just be made inaccessible by default for anything not integrated into the
> java.base module. A command line option is required to crack open the safe
> [1].
>
> There is a very valid concern, since @Contended changes object layout and
> increases object size, liberal use might tickle an overflow in HotSpot
> code. Hence why it has remained internal so far.
>
> One approach to mitigate such concern is to provide a -XX flag. There
> happens to be one already available (that Aleksey pointed me to) :-)
> RestrictContended, whose default value is true, such that by default
> @Contended is ignored for anything other that stuff on the boot and
> extension class loader [2]. So any current use outside of the JDK needs to
> opt in with a command line argument.
>
>
> My inclination would be continue with the conservative position for now
> and provide an @sun.misc.Contended that aliases to the JDK internal, the
> former of which requires -XX:-RestrictContended to function outside of
> privileged code, and the latter of which will not function outside of
> privileged code.
>
> I anticipate with Project Panama and Valhalla we are gonna seriously
> hammer on the object layout mechanisms in HotSpot, and that may root out
> any lurking issues, and thus we may be more comfortable supporting this in
> a future public API (there is still the attractive nuisance argument, but
> that one is much harder to solve...)
>
> Paul.
>
> [1] See the “Breaking encapsulation” section of
> http://openjdk.java.net/jeps/261
>
> [2] Note that the extension mechanism has been removed in JDK 9, see the
> "Removed: The extension mechanism” section of
> http://openjdk.java.net/jeps/220
>
> > On 12 Nov 2015, at 01:17, Doug Lea  wrote:
> >
> > On 11/11/2015 09:07 AM, Chris Hegarty wrote:
> >
> >> So I think the questions we need to answer are as follows:
> >>  a) Is it desirable to have @Contended as part of Java SE?
> >> i) If so, is this doable for JDK 9?
> >>  b) If ‘No’ to a or i, do we consider @Contended “critical”,
> >>   as per JEP 260 ?
> >>
> >
> > @Contended is something that only a very small number of developers
> > directly use (sparingly), but the products of their efforts are used
> > by many other developers.
> >
> > The "very small number" is still greater than the number of
> > java.util.concurrent maintainers. As the use of concurrency expands,
> > java.util.concurrent itself can't (and shouldn't) supply all possible
> > specialized components needed in applications, so external libraries
> > are needed to fill in the gaps. I'm sure that every developer of such
> > concurrent components would vote to be able to use @Contended.
> >
> > I don't know of any argument not to expose it, beyond the usual
> > concerns about supporting nichy esoteric stuff in jdk. Under the
> > Unsafe-phase-out plans, public support for nichy esoteric stuff
> > that has no other functional alternative will be more common.
> >
> > Without some good argument against it, I think the only issue
> > is where (what package) to place it. Carrying it out from there seems
> > identical to your webrev diffs, just changing import statements
> > and the like.
> >
> >
> > -Doug
> >
> >
> >
> >
> >
>
>

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

[Vitaly Davidovich]

You didn't explicitly say that, but it came across like that to me;
apologies if I misread.

So your concern, then, is that the implementation may not be 100% correct,
safe, and robust? If it were more readily available to non-JDK users,
you're likely to see more use of it and thus more "coverage" of the
feature.  The disable flag can continue to exist in case something
catastrophic is discovered, and it can be disabled by default so people
have to opt-in (for now).  I don't quite get how it spending more time in
internal packaging (and thus seeing limited application) will help in
shaking out bugs/exploits.

On Thu, Nov 12, 2015 at 9:00 AM, Paul Sandoz  wrote:

>
> > On 12 Nov 2015, at 14:48, Vitaly Davidovich  wrote:
> >
> > Hi Paul,
> >
> > There is a very valid concern, since @Contended changes object layout
> and increases object size, liberal use might tickle an overflow in HotSpot
> code. Hence why it has remained internal so far.
> >
> > What overflow? OOM of the heap? How is that a "very valid" concern? Why
> would it be used liberally? Why are you allowing users to allocate any
> memory whatsoever? Why not put a cap on the size of objects non-JDK code
> can allocate? :) I mean seriously, with all due respect, the occasional
> "our users are dumb and misinformed" sentiment is very off-putting.
> >
>
> Where did i say that? You are incorrectly reading between the lines. The
> concern is actually smart and well informed users that may dliberately
> exploit a bug/weakness in the VM.
>
> Paul.
>
> > If you think there are, currently, *implementation* issues with how
> @Contended is handled in the VM which cannot be addresses, I'll buy that.
> But can we please stop with the dumbing down? :)
> >
>

Re: RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Roger Riggs]

Hi Paul,

Thanks.

One followup comment comparing varargs to List.

If there is any concern about concurrency, the callee still has to make 
a copy of the incoming
List/array before using/check it to get a consistent view.  Only the 
caller can rely on the immutability
of the argument (if it created it); not the callee.  Now if it were a 
concrete final type...

We still have to watch out for that.

Roger


On 11/12/2015 9:03 AM, Paul Sandoz wrote:

On 11 Nov 2015, at 19:11, Roger Riggs  wrote:

Hi Paul,

Updated Webrev to use List instead of ProcessHandle... varargs.

   http://cr.openjdk.java.net/~rriggs/webrev-pipeline-8132394/


+1

Paul.

Re: RFR 9: 8132394 : (process) ProcessBuilder support for a pipeline of processes

[Paul Sandoz]

> On 12 Nov 2015, at 15:24, Roger Riggs  wrote:
> 
> Hi Paul,
> 
> Thanks.
> 

One more thing i forgot to mention, if there is ever a chance that 
ProcessBuilder is changed from final to non-final we should List.


> One followup comment comparing varargs to List.
> 

> If there is any concern about concurrency, the callee still has to make a 
> copy of the incoming
> List/array before using/check it to get a consistent view.  Only the caller 
> can rely on the immutability
> of the argument (if it created it); not the callee.  Now if it were a 
> concrete final type...
> We still have to watch out for that.
> 

Yes.

Paul.

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[Claes Redestad]

On 2015-11-12 18:45, John Rose wrote:


How about this:

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



Yes, that's fine.


I think we have to add a storeFence or two to avoid publishing 
NamedFunctions whose resolvedHandle may otherwise be observed as only 
partially visible:


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

/Claes

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

[Vitaly Davidovich]

Thanks John, I understand your position and rationale (Paul actually
clarified this along the same lines to me earlier today).  I have to admit
that this would have to be quite an exotic exploit, but point taken.

sent from my phone
On Nov 12, 2015 7:08 PM, "John Rose"  wrote:

> On Nov 12, 2015, at 5:48 AM, Vitaly Davidovich  wrote:
>
>
> There is a very valid concern, since @Contended changes object layout and
> increases object size, liberal use might tickle an overflow in HotSpot
> code. Hence why it has remained internal so far.
>
>
>
> What overflow? OOM of the heap? How is that a "very valid" concern? Why
> would it be used liberally? Why are you allowing users to allocate any
> memory whatsoever? Why not put a cap on the size of objects non-JDK code
> can allocate? :) I mean seriously, with all due respect, the occasional
> "our users are dumb and misinformed" sentiment is very off-putting.
>
>
> 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: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

On 2015-11-12 14:47, Paul Sandoz wrote:

On 11 Nov 2015, at 15:32, Claes Redestad  wrote:

Paul,

On 2015-11-10 11:55, Paul Sandoz wrote:

DirectMethodHandle
—
  682 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];

Invokers
—
  442 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];

MethodHandleImpl
—
1627 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];


To be complete you could add “final”, thus it makes it clear that @Stable 
refers specifically to the array element.

Paul.

Thanks for having a look and catching this:

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

- added final keyword to FUNCTIONS and HANDLES
- added @Stable to ARRAYS, FILL_ARRAYS, and FILL_ARRAY_TO_RIGHT


MethodHandleImpl.java
—

1413 private static final @Stable MethodHandle[] FILL_ARRAYS = new 
MethodHandle[FILL_ARRAYS_COUNT + 1];
1414
1415 private static MethodHandle getFillArray(int count) {
1416 assert (count > 0 && count <= FILL_ARRAYS_COUNT);

Why FILL_ARRAYS_COUNT + 1 rather than FILL_ARRAYS_COUNT?

Based on the previous code I would have expected the bounds to be:

   0 < count < FILL_ARRAYS_COUNT

Paul.


Yes. Updated http://cr.openjdk.java.net/~redestad/8142334/webrev.03 
in-place.


/Claes

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Paul Sandoz]

Hi Peter,

This stuff always gives me a headache :-)

IIUC it’s all idempotent stuff, and the final field in NamedFunction should 
take care of certain things.

Claes, was it intentional that you call function.resolve() after the array 
store? You might need to reverse that and place a Unsafe.storeFence between 
them if it is required that the published and visible function be resolved.

Paul.

> On 12 Nov 2015, at 17:26, Peter Levart  wrote:
> 
> Hi Claes,
> 
> I have one concern...
> 
> 645 private static NamedFunction getConstantFunction(int idx) {
> 646 NamedFunction function = FUNCTIONS[idx];
> 647 if (function != null) {
> 648 return function;
> 649 }
> 650 return setCachedFunction(idx, makeConstantFunction(idx));
> 651 }
> 652
> 653 private static synchronized NamedFunction setCachedFunction(int idx, 
> final NamedFunction function) {
> 654 // Simulate a CAS, to avoid racy duplication of results.
> 655 NamedFunction prev = FUNCTIONS[idx];
> 656 if (prev != null) {
> 657 return prev;
> 658 }
> 659 FUNCTIONS[idx] = function;
> 660 function.resolve();
> 661 return function;
> 662 }
> 
> 
> Above is a classical double-checked locking idiom, but it is not using 
> volatile variable to publish the NamedFunction instance. I wonder if this is 
> safe. Even if the FUNCTIONS[idx] slot was a volatile variable,  you would 
> publish new instance before resolving it. Is it OK to publish unresolved 
> NamedFunction(s)? There is a NamedFunction.resolvedHandle() instance method 
> that makes sure NamedFunction is resolved before returning a MethodHandle, 
> but there are also usages of dereferencing NamedFunction.resolvedHandle field 
> directly in code. Are you sure that such unresolved or almost resolved 
> instance of NamedFunction is never used in such places where 
> NamedFunction.resolvedHandle field is dereferenced directly?
> 
> In original code those NamedFunctions were resolved in static initializer so 
> they were published properly.
> 
> Regards, Peter
> 
> On 11/12/2015 04:55 PM, Claes Redestad wrote:
>> 
>> On 2015-11-12 14:47, Paul Sandoz wrote:
 On 11 Nov 2015, at 15:32, Claes Redestad  wrote:
 
 Paul,
 
 On 2015-11-10 11:55, Paul Sandoz wrote:
> DirectMethodHandle
> —
>  682 private static @Stable NamedFunction[] FUNCTIONS = new 
> NamedFunction[NF_LIMIT];
> 
> Invokers
> —
>  442 private static @Stable NamedFunction[] FUNCTIONS = new 
> NamedFunction[NF_LIMIT];
> 
> MethodHandleImpl
> —
> 1627 private static @Stable NamedFunction[] FUNCTIONS = new 
> NamedFunction[NF_LIMIT];
> 
> 
> To be complete you could add “final”, thus it makes it clear that @Stable 
> refers specifically to the array element.
> 
> Paul.
 Thanks for having a look and catching this:
 
 http://cr.openjdk.java.net/~redestad/8142334/webrev.03
 
 - added final keyword to FUNCTIONS and HANDLES
 - added @Stable to ARRAYS, FILL_ARRAYS, and FILL_ARRAY_TO_RIGHT
 
>>> MethodHandleImpl.java
>>> —
>>> 
>>> 1413 private static final @Stable MethodHandle[] FILL_ARRAYS = new 
>>> MethodHandle[FILL_ARRAYS_COUNT + 1];
>>> 1414
>>> 1415 private static MethodHandle getFillArray(int count) {
>>> 1416 assert (count > 0 && count <= FILL_ARRAYS_COUNT);
>>> 
>>> Why FILL_ARRAYS_COUNT + 1 rather than FILL_ARRAYS_COUNT?
>>> 
>>> Based on the previous code I would have expected the bounds to be:
>>> 
>>>   0 < count < FILL_ARRAYS_COUNT
>>> 
>>> Paul.
>> 
>> Yes. Updated http://cr.openjdk.java.net/~redestad/8142334/webrev.03 in-place.
>> 
>> /Claes
> 
> 

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

Hi,

On 2015-11-12 17:26, Peter Levart wrote:

Hi Claes,

I have one concern...

 645 private static NamedFunction getConstantFunction(int idx) {
 646 NamedFunction function = FUNCTIONS[idx];
 647 if (function != null) {
 648 return function;
 649 }
 650 return setCachedFunction(idx, makeConstantFunction(idx));
 651 }
 652
 653 private static synchronized NamedFunction 
setCachedFunction(int idx, final NamedFunction function) {

 654 // Simulate a CAS, to avoid racy duplication of results.
 655 NamedFunction prev = FUNCTIONS[idx];
 656 if (prev != null) {
 657 return prev;
 658 }
 659 FUNCTIONS[idx] = function;
 660 function.resolve();
 661 return function;
 662 }


Above is a classical double-checked locking idiom, but it is not using 
volatile variable to publish the NamedFunction instance. I wonder if 
this is safe. Even if the FUNCTIONS[idx] slot was a volatile 
variable,  you would publish new instance before resolving it. Is it 
OK to publish unresolved NamedFunction(s)? There is a 
NamedFunction.resolvedHandle() instance method that makes sure 
NamedFunction is resolved before returning a MethodHandle, but there 
are also usages of dereferencing NamedFunction.resolvedHandle field 
directly in code. Are you sure that such unresolved or almost resolved 
instance of NamedFunction is never used in such places where 
NamedFunction.resolvedHandle field is dereferenced directly?


In original code those NamedFunctions were resolved in static 
initializer so they were published properly.


I think we're relying on the fact that relevant field, member, is final 
and thus will be published correctly.


resolvedHandle will be set to null in the constructor we're using anyhow:

NamedFunction(Method method) {
this(new MemberName(method));
}
...
NamedFunction(MemberName member) {
this.member = member;
this.resolvedHandle = null;
}

I inquired myself about the rationale for using of DCL without volatile 
elsewhere in the java.lang.invoke code and was pointed to this earlier 
discussion that shed light on the assumptions and caveats: 
http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-May/013902.html


So, all in all, I think the partially unsafe publication is actually 
safe for our purposes here.


Thanks!

/Claes

Re: RFR: 8142487: Cleanup sun.invoke.util.Wrapper zeroes to be both reliable and lazy

[John Rose]

On Nov 12, 2015, at 6:13 AM, Claes Redestad  wrote:
> 
> 
> On 2015-11-12 01:10, John Rose wrote:
>> For better maintainability, I think the zero and identity forms should be 
>> created together, not in separate twin code blocks.
>> 
>> I think this is a safer, saner way to inject laziness here:
>> 
>> - private static void createIdentityForms() {
>> + // Create LF_zero, LF_identity, etc., for the given type.
>> + private static void createIdentityForms(BasicType type) {
>> 
>> It means that groups of LFs get lazily created; if that is tolerable for the 
>> present purpose, it's easier to reason about.
>> 
>> — John
> 
> How about this:
> 
> http://cr.openjdk.java.net/~redestad/8142487/webrev.02 
> 

Yes, that's fine.

> I'm seeing no increase in number of LambdaForms or NamedFunctions created by 
> merging it all back into one method, while getting rid of MN_* arrays.

Good deal.

> I fixed the obvious mistake in sun.invoke.util.WrapperTest w.r.t byte/char, 
> and since these tests fail against JDK-8141678 I wouldn't mind keeping it 
> around.

OK, if it found a bug it has earned its place.

— John

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Peter Levart]

Hi Claes,

It might be ok as you say, but then I don't understand the need to
pre-resolve() NamedFunctions in original code. Do you?

Regards, Peter
On Nov 12, 2015 6:44 PM, "Claes Redestad"  wrote:

> Hi,
>
> On 2015-11-12 17:26, Peter Levart wrote:
>
>> Hi Claes,
>>
>> I have one concern...
>>
>>  645 private static NamedFunction getConstantFunction(int idx) {
>>  646 NamedFunction function = FUNCTIONS[idx];
>>  647 if (function != null) {
>>  648 return function;
>>  649 }
>>  650 return setCachedFunction(idx, makeConstantFunction(idx));
>>  651 }
>>  652
>>  653 private static synchronized NamedFunction setCachedFunction(int
>> idx, final NamedFunction function) {
>>  654 // Simulate a CAS, to avoid racy duplication of results.
>>  655 NamedFunction prev = FUNCTIONS[idx];
>>  656 if (prev != null) {
>>  657 return prev;
>>  658 }
>>  659 FUNCTIONS[idx] = function;
>>  660 function.resolve();
>>  661 return function;
>>  662 }
>>
>>
>> Above is a classical double-checked locking idiom, but it is not using
>> volatile variable to publish the NamedFunction instance. I wonder if this
>> is safe. Even if the FUNCTIONS[idx] slot was a volatile variable,  you
>> would publish new instance before resolving it. Is it OK to publish
>> unresolved NamedFunction(s)? There is a NamedFunction.resolvedHandle()
>> instance method that makes sure NamedFunction is resolved before returning
>> a MethodHandle, but there are also usages of dereferencing
>> NamedFunction.resolvedHandle field directly in code. Are you sure that such
>> unresolved or almost resolved instance of NamedFunction is never used in
>> such places where NamedFunction.resolvedHandle field is dereferenced
>> directly?
>>
>> In original code those NamedFunctions were resolved in static initializer
>> so they were published properly.
>>
>
> I think we're relying on the fact that relevant field, member, is final
> and thus will be published correctly.
>
> resolvedHandle will be set to null in the constructor we're using anyhow:
>
> NamedFunction(Method method) {
> this(new MemberName(method));
> }
> ...
> NamedFunction(MemberName member) {
> this.member = member;
> this.resolvedHandle = null;
> }
>
> I inquired myself about the rationale for using of DCL without volatile
> elsewhere in the java.lang.invoke code and was pointed to this earlier
> discussion that shed light on the assumptions and caveats:
> http://mail.openjdk.java.net/pipermail/hotspot-dev/2014-May/013902.html
>
> So, all in all, I think the partially unsafe publication is actually safe
> for our purposes here.
>
> Thanks!
>
> /Claes
>

Re: 8142493: Utility methods to check indexes and ranges doesn't specify behavior when function produces null

[Mandy Chung]

Looks good.
Mandy

> On Nov 12, 2015, at 3:19 AM, Paul Sandoz  wrote:
> 
> Hi,
> 
> Please review:
> 
>  
> http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8142493-checkIndex-function-return-null/webrev/
>  
> 
> 
> This catches some edge cases for the exception mapping function passed to the 
> Objects::checkIndex* methods, specifically if the function returns null or 
> throws an exception.
> 
> Paul.

Re: Code Review for JEP 259: Stack-Walking API

[Mandy Chung]

You might want to get back Method (or something equivalent non-executable 
version?) this requires a lot more consideration.  StackWalker opens up this 
possibility for the future.   I’d like to keep the scope for this JEP to do 
stack walking and provide the replacement for 
sun.reflect.Reflection.getCallerClass in JDK 9.

You can file RFE for future consideration.

Mandy

> On Nov 12, 2015, at 7:29 AM, Jason Mehrens  wrote:
> 
> Mandy,
> 
> Forgive my ignorance on this subject and impacts but, would it be possible to 
> include more metadata methods in the StackWalker.StackFrame interface such as 
> getParameterTypes/parameterArray and getReturnType/returnType?  Ideally, I 
> really would like these methods for StackTraceElement since knowing the exact 
> parameter types allows the caller to lookup the method to examine all the 
> method metadata which is useful for logging not only in output but for use in 
> stacktrace reduction algorithms.
> 
> Thanks,
> 
> Jason
> 
> 
> From: core-libs-dev  on behalf of 
> Mandy Chung 
> Sent: Monday, November 9, 2015 8:32 PM
> To: core-libs-dev; hotspot-runtime-...@openjdk.java.net
> Subject: Code Review for JEP 259: Stack-Walking API
> 
> javadoc:
>   
> http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> webrev:
>  http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> Overview of the implementation:
> When stack walking begins, the StackWalker calls into the VM to anchor a 
> native frame (callStackWalk) that will fetch the first batch of stack frames. 
>  VM will then invoke the doStackWalk method so that the consumer starts 
> getting StackFrame object for each frame.  If all frames in the current batch 
> are traversed, it will ask the VM to fetch the next batch.  The library side 
> is doing the filtering of reflection frames.   For this patch, the VM filters 
> of the hidden frames and also filter out Throwable::init related frames for 
> stack trace.
> 
> Ultimately we will move to these built-in logic out from the VM to the 
> library but I’d like to separate them as future works.
> 
> This patch also includes the change for Throwable to use StackWalker but it’s 
> disabled by default.  To enable it, set -Dstackwalk.newThrowable=true.  The 
> VM backtrace is well tuned for performance.  So we separate the switch of 
> Throwable to use StackWalker as a follow-on work:
>   JDK-8141239 Throwable should use StackWalker API to capture the backtrace
> 
> MemberName initialization is one source of overhead and we propose to keep 
> the VM flag -XX:-MemberNameInStackFrame for the time being for the 
> performance work to continue for JDK-8141239.
> 
> Thanks
> Mandy

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

[Chris Hegarty]

To me, it sounds like @Contended, in its current form, is not quite ready
for prime-time ( inclusion in Java SE 9 ). There is some concern about its
implementation, and I’m not sure how the loader restriction, and the control
through a -XX flag, would translate into SE spec. There is certainly some
additional work, beyond my primary goal ( preparation for JEP 260 ) that
is required.

It seems a reasonable compromise ( until someone can spend some 
quality time on it ) to support the sun.misc.Contended annotation as
an alias, or similar, along with the internal Contended annotation (
required by java.base ). The changes to support such are relatively
straight forward, and can easily be refactored in the future.

If there is agreement, I will add @Contended to JEP 260.

-Chris.

> On 12 Nov 2015, at 14:15, Vitaly Davidovich  wrote:
> 
> You didn't explicitly say that, but it came across like that to me;
> apologies if I misread.
> 
> So your concern, then, is that the implementation may not be 100% correct,
> safe, and robust? If it were more readily available to non-JDK users,
> you're likely to see more use of it and thus more "coverage" of the
> feature.  The disable flag can continue to exist in case something
> catastrophic is discovered, and it can be disabled by default so people
> have to opt-in (for now).  I don't quite get how it spending more time in
> internal packaging (and thus seeing limited application) will help in
> shaking out bugs/exploits.
> 
> On Thu, Nov 12, 2015 at 9:00 AM, Paul Sandoz  wrote:
> 
>> 
>>> On 12 Nov 2015, at 14:48, Vitaly Davidovich  wrote:
>>> 
>>> Hi Paul,
>>> 
>>> There is a very valid concern, since @Contended changes object layout
>> and increases object size, liberal use might tickle an overflow in HotSpot
>> code. Hence why it has remained internal so far.
>>> 
>>> What overflow? OOM of the heap? How is that a "very valid" concern? Why
>> would it be used liberally? Why are you allowing users to allocate any
>> memory whatsoever? Why not put a cap on the size of objects non-JDK code
>> can allocate? :) I mean seriously, with all due respect, the occasional
>> "our users are dumb and misinformed" sentiment is very off-putting.
>>> 
>> 
>> Where did i say that? You are incorrectly reading between the lines. The
>> concern is actually smart and well informed users that may dliberately
>> exploit a bug/weakness in the VM.
>> 
>> Paul.
>> 
>>> If you think there are, currently, *implementation* issues with how
>> @Contended is handled in the VM which cannot be addresses, I'll buy that.
>> But can we please stop with the dumbing down? :)
>>> 
>> 

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Peter Levart]

Hi Claes,

I have one concern...

 645 private static NamedFunction getConstantFunction(int idx) {
 646 NamedFunction function = FUNCTIONS[idx];
 647 if (function != null) {
 648 return function;
 649 }
 650 return setCachedFunction(idx, makeConstantFunction(idx));
 651 }
 652
 653 private static synchronized NamedFunction 
setCachedFunction(int idx, final NamedFunction function) {

 654 // Simulate a CAS, to avoid racy duplication of results.
 655 NamedFunction prev = FUNCTIONS[idx];
 656 if (prev != null) {
 657 return prev;
 658 }
 659 FUNCTIONS[idx] = function;
 660 function.resolve();
 661 return function;
 662 }


Above is a classical double-checked locking idiom, but it is not using 
volatile variable to publish the NamedFunction instance. I wonder if 
this is safe. Even if the FUNCTIONS[idx] slot was a volatile variable,  
you would publish new instance before resolving it. Is it OK to publish 
unresolved NamedFunction(s)? There is a NamedFunction.resolvedHandle() 
instance method that makes sure NamedFunction is resolved before 
returning a MethodHandle, but there are also usages of dereferencing 
NamedFunction.resolvedHandle field directly in code. Are you sure that 
such unresolved or almost resolved instance of NamedFunction is never 
used in such places where NamedFunction.resolvedHandle field is 
dereferenced directly?


In original code those NamedFunctions were resolved in static 
initializer so they were published properly.


Regards, Peter

On 11/12/2015 04:55 PM, Claes Redestad wrote:


On 2015-11-12 14:47, Paul Sandoz wrote:
On 11 Nov 2015, at 15:32, Claes Redestad  
wrote:


Paul,

On 2015-11-10 11:55, Paul Sandoz wrote:

DirectMethodHandle
—
  682 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];


Invokers
—
  442 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];


MethodHandleImpl
—
1627 private static @Stable NamedFunction[] FUNCTIONS = new 
NamedFunction[NF_LIMIT];



To be complete you could add “final”, thus it makes it clear that 
@Stable refers specifically to the array element.


Paul.

Thanks for having a look and catching this:

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

- added final keyword to FUNCTIONS and HANDLES
- added @Stable to ARRAYS, FILL_ARRAYS, and FILL_ARRAY_TO_RIGHT


MethodHandleImpl.java
—

1413 private static final @Stable MethodHandle[] FILL_ARRAYS = 
new MethodHandle[FILL_ARRAYS_COUNT + 1];

1414
1415 private static MethodHandle getFillArray(int count) {
1416 assert (count > 0 && count <= FILL_ARRAYS_COUNT);

Why FILL_ARRAYS_COUNT + 1 rather than FILL_ARRAYS_COUNT?

Based on the previous code I would have expected the bounds to be:

   0 < count < FILL_ARRAYS_COUNT

Paul.


Yes. Updated http://cr.openjdk.java.net/~redestad/8142334/webrev.03 
in-place.


/Claes

Re: RFR 8141409 Arrays.equals accepting a Comparator

[Roger Riggs]

Hi Paul,

- A minor point but in the argument names, its a bit inconsistent 
between using 'b' and 'a2'.
I would have use 'a', and 'b' to be more consistent.  (and yes the 
current methods show the same inconsistency)


- The @return(s) should have an 'otherwise {@code false};  otherwise 
maybe it is allowed to return true.


- In the implementation, I generally prefer the form of 
Objects.requireNonNull(o, "name") so the exception

identifies the offending argument.

- I thought the coding style has spaces around operators (like ==).

- The non-range checked version of equals should have the same behavior as:
   equals(a, 0, a.length, a2, 0, a2.length).

The range checked version should include the sentence:

"Also, two array references are  considered equal if both are {@code null}."

- The implementation will need to check for nulls before the rangeChecks.

Regards, Roger





On 11/12/2015 5:36 AM, Paul Sandoz wrote:

Hi,

I have made a retreat on this just to add the two missing equals methods:

   
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/ 


I propose to revisit the overall null-handling semantics, especially for 
Comparable[] accepting methods, at a later date.

Paul.


On 4 Nov 2015, at 15:32, Paul Sandoz  wrote:

Hi,

Please review:

  
http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8141409-Arrays-equals-comparator/webrev/ 

  https://bugs.openjdk.java.net/browse/JDK-8141409 


This builds on (the already reviewed):

  https://bugs.openjdk.java.net/browse/JDK-8033148 

  http://cr.openjdk.java.net/~psandoz/jdk9/JDK-8033148-Arrays-lexico-compare/webrev/ 


and adds yet more methods to Arrays, only two this time though, to fill out the 
missing gaps related to array equality with a comparator.

In addition i added an Objects.compare method, which simplifies the 
implementations of some object-bearing methods.

Both additions simplify the specification some object-bearing methods.

Paul.

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Peter Levart]

On 11/12/2015 07:50 PM, Claes Redestad wrote:

Paul,

On 2015-11-12 19:34, Claes Redestad wrote:


Claes, was it intentional that you call function.resolve() after the 
array store? You might need to reverse that and place a 
Unsafe.storeFence between them if it is required that the published 
and visible function be resolved.


This however was unintentional, and I think you're right that this 
would ensure visibility:


function.resolve();
UNSAFE.storeFence();
FUNCTIONS[idx] = function;

http://cr.openjdk.java.net/~redestad/8142334/webrev.04/



turns out the original order might've been necessary at least in 
DirectMethodHandle.java to break a circular dependency:


at 
java.lang.invoke.DirectMethodHandle.setCachedFunction(java.base@9.0/DirectMethodHandle.java:659)
at 
java.lang.invoke.DirectMethodHandle.getConstantFunction(java.base@9.0/DirectMethodHandle.java:650)
at 
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(java.base@9.0/DirectMethodHandle.java:232)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:188)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:177)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:84)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:104)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:109)
at 
java.lang.invoke.LambdaForm$NamedFunction.resolve(java.base@9.0/LambdaForm.java:1078)
at 
java.lang.invoke.DirectMethodHandle.setCachedFunction(java.base@9.0/DirectMethodHandle.java:659)
at 
java.lang.invoke.DirectMethodHandle.getConstantFunction(java.base@9.0/DirectMethodHandle.java:650)
at 
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(java.base@9.0/DirectMethodHandle.java:232)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:188)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:177)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:84)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:104)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:109)
at 
java.lang.invoke.LambdaForm$NamedFunction.resolve(java.base@9.0/LambdaForm.java:1078)


Do you think this would work correctly w.r.t visibility:

FUNCTIONS[idx] = function;
function.resolve();
UNSAFE.storeFence();


This does not do anything useful.

What happens if you simply omit function.resolve() call. If lazy 
resolving works and tests pass, then perhaps it's all ok.


Peter



?

/Claes

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

[Paul Sandoz]

Hi,

I don’t think anyone disagrees on the usefulness of @Contended.

Beyond the JDK and 166 are there any more usages out there in the wild?

I am not aware of any, grepcode does not report any usages outside of the JDK, 
but we may have missed them. The current set of proposed critical internal APIs 
have been derived from analyses of various collections of code. This may in 
part be because it was only introduced in Java 8.

Note that as currently proposed @Contended is not going away, it would just be 
made inaccessible by default for anything not integrated into the java.base 
module. A command line option is required to crack open the safe [1].

There is a very valid concern, since @Contended changes object layout and 
increases object size, liberal use might tickle an overflow in HotSpot code. 
Hence why it has remained internal so far.

One approach to mitigate such concern is to provide a -XX flag. There happens 
to be one already available (that Aleksey pointed me to) :-) RestrictContended, 
whose default value is true, such that by default @Contended is ignored for 
anything other that stuff on the boot and extension class loader [2]. So any 
current use outside of the JDK needs to opt in with a command line argument.


My inclination would be continue with the conservative position for now and 
provide an @sun.misc.Contended that aliases to the JDK internal, the former of 
which requires -XX:-RestrictContended to function outside of privileged code, 
and the latter of which will not function outside of privileged code.

I anticipate with Project Panama and Valhalla we are gonna seriously hammer on 
the object layout mechanisms in HotSpot, and that may root out any lurking 
issues, and thus we may be more comfortable supporting this in a future public 
API (there is still the attractive nuisance argument, but that one is much 
harder to solve...)

Paul.

[1] See the “Breaking encapsulation” section of http://openjdk.java.net/jeps/261

[2] Note that the extension mechanism has been removed in JDK 9, see the 
"Removed: The extension mechanism” section of http://openjdk.java.net/jeps/220

> On 12 Nov 2015, at 01:17, Doug Lea  wrote:
> 
> On 11/11/2015 09:07 AM, Chris Hegarty wrote:
> 
>> So I think the questions we need to answer are as follows:
>>  a) Is it desirable to have @Contended as part of Java SE?
>> i) If so, is this doable for JDK 9?
>>  b) If ‘No’ to a or i, do we consider @Contended “critical”,
>>   as per JEP 260 ?
>> 
> 
> @Contended is something that only a very small number of developers
> directly use (sparingly), but the products of their efforts are used
> by many other developers.
> 
> The "very small number" is still greater than the number of
> java.util.concurrent maintainers. As the use of concurrency expands,
> java.util.concurrent itself can't (and shouldn't) supply all possible
> specialized components needed in applications, so external libraries
> are needed to fill in the gaps. I'm sure that every developer of such
> concurrent components would vote to be able to use @Contended.
> 
> I don't know of any argument not to expose it, beyond the usual
> concerns about supporting nichy esoteric stuff in jdk. Under the
> Unsafe-phase-out plans, public support for nichy esoteric stuff
> that has no other functional alternative will be more common.
> 
> Without some good argument against it, I think the only issue
> is where (what package) to place it. Carrying it out from there seems
> identical to your webrev diffs, just changing import statements
> and the like.
> 
> 
> -Doug
> 
> 
> 
> 
> 

Re: RFR: 8142334: Improve lazy initialization of java.lang.invoke

[Claes Redestad]

Paul,

On 2015-11-12 19:34, Claes Redestad wrote:


Claes, was it intentional that you call function.resolve() after the 
array store? You might need to reverse that and place a 
Unsafe.storeFence between them if it is required that the published 
and visible function be resolved.


This however was unintentional, and I think you're right that this 
would ensure visibility:


function.resolve();
UNSAFE.storeFence();
FUNCTIONS[idx] = function;

http://cr.openjdk.java.net/~redestad/8142334/webrev.04/



turns out the original order might've been necessary at least in 
DirectMethodHandle.java to break a circular dependency:


at 
java.lang.invoke.DirectMethodHandle.setCachedFunction(java.base@9.0/DirectMethodHandle.java:659)
at 
java.lang.invoke.DirectMethodHandle.getConstantFunction(java.base@9.0/DirectMethodHandle.java:650)
at 
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(java.base@9.0/DirectMethodHandle.java:232)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:188)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:177)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:84)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:104)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:109)
at 
java.lang.invoke.LambdaForm$NamedFunction.resolve(java.base@9.0/LambdaForm.java:1078)
at 
java.lang.invoke.DirectMethodHandle.setCachedFunction(java.base@9.0/DirectMethodHandle.java:659)
at 
java.lang.invoke.DirectMethodHandle.getConstantFunction(java.base@9.0/DirectMethodHandle.java:650)
at 
java.lang.invoke.DirectMethodHandle.makePreparedLambdaForm(java.base@9.0/DirectMethodHandle.java:232)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:188)
at 
java.lang.invoke.DirectMethodHandle.preparedLambdaForm(java.base@9.0/DirectMethodHandle.java:177)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:84)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:104)
at 
java.lang.invoke.DirectMethodHandle.make(java.base@9.0/DirectMethodHandle.java:109)
at 
java.lang.invoke.LambdaForm$NamedFunction.resolve(java.base@9.0/LambdaForm.java:1078)


Do you think this would work correctly w.r.t visibility:

FUNCTIONS[idx] = function;
function.resolve();
UNSAFE.storeFence();

?

/Claes

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

[John Rose]

On Nov 12, 2015, at 5:48 AM, Vitaly Davidovich  wrote:
> 
>> There is a very valid concern, since @Contended changes object layout and
>> increases object size, liberal use might tickle an overflow in HotSpot
>> code. Hence why it has remained internal so far.
> 
> 
> What overflow? OOM of the heap? How is that a "very valid" concern? Why
> would it be used liberally? Why are you allowing users to allocate any
> memory whatsoever? Why not put a cap on the size of objects non-JDK code
> can allocate? :) I mean seriously, with all due respect, the occasional
> "our users are dumb and misinformed" sentiment is very off-putting.


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