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

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 -

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

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

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

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,

Re: RFR 8141409 Arrays.equals accepting a Comparator

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'

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

> 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. > >

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

> 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

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

.. 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

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

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

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

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

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

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

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

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

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

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

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

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, > >

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

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

Re: RFR 8141409 Arrays.equals accepting a Comparator

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

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

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

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

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

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

> 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

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

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

> 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

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

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>

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

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

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

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

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

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"

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

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

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

> 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

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

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

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

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"

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

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

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

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

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

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

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

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

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

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, >> >>

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

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/ > >

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

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

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

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

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

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,

Re: RFR 8141409 Arrays.equals accepting a Comparator

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

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

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

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

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

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

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

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

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