Re: RFR: 8222144: Lazily create JRT code source URLs for LoadedModules

On 08/04/2019 22:42, Claes Redestad wrote: Hi, currently we eagerly convert code source URIs to URLs to avoid recursive bootstrap issues with overrideable URL handlers. The JRT scheme handler is not overrideable, however, and also commonly used for all system modules. Creating these URLs lazily

Re: [PATCH] Some improvements for java.lang.Class

Hello! As an author of the IDE inspection in question I should mention that there's slight difference between for(int i=0; i= 0. So totally mechanical replacement should work fine, but may produce some amount of unnecessary Math.max and deciding whether to keep or remove them requires thinking.

Re: RFR: [8u] 8205432: Replace the placeholder Japanese era name

On 08/04/2019 09:25, Deepak Kejriwal wrote: > Hi Andrew, > > Thanks for working on this. Please find below few minor comments:- > > 1>. For "src/share/classes/java/util/JapaneseImperialCalendar.java" and > "src/share/classes/sun/util/calendar/Era.java" please changes the date time > format to b

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

The patch looks good (the return statement is missing).  Thanks for fixing it. Mandy P.S. Sorry for the belated review. Good to see this pushed.  I'm OOO. On 4/8/19 9:23 PM, Aleksey Shipilev wrote: On 4/8/19 3:08 PM, David Holmes wrote: +1 Thanks, I am going to push this under triviality rul

Re: [PATCH] Some improvements for java.lang.Class

Hi Sergei, I have created issue [1] to track your enhancement. Please cc me in your next mails I will be helping you to do the paperwork. Talking about that did you sign the OCA, [2]? Thanks, Vicente [1] https://bugs.openjdk.java.net/browse/JDK-8222151 [2] https://www.oracle.com/technetwork/

Re: RFR: 8222029: Optimize Math.floorMod

Should any additional cases be added to test/jdk/java/lang/Math/DivModTests.java to cover the new implementation? Thanks, -Joe On 4/5/2019 10:21 AM, Claes Redestad wrote: On 2019-04-05 17:41, Andrew Haley wrote: On 4/5/19 2:44 PM, Claes Redestad wrote: Testing: tier1-2, all Math tests run

Re: RFR: JDK-8221749: Error messages

On 4/8/2019 5:41 PM, Alexander Matveev wrote: Hi Andy, Looks good. Also, many messages (including added in this review) in MainResources.properties do have "." at the end and some don't. I think we should cleanup it and add "." if needed. Yes - some messages are not full sentences, but most

RFR: 8222144: Lazily create JRT code source URLs for LoadedModules

Hi, currently we eagerly convert code source URIs to URLs to avoid recursive bootstrap issues with overrideable URL handlers. The JRT scheme handler is not overrideable, however, and also commonly used for all system modules. Creating these URLs lazily is a reasonable startup optimization since m

Re: RFR: JDK-8221749: Error messages

Hi Andy, Looks good. Also, many messages (including added in this review) in MainResources.properties do have "." at the end and some don't. I think we should cleanup it and add "." if needed. Thanks, Alexander On 4/8/2019 4:41 AM, Andy Herrick wrote: Please review the jpackage fix for bug

Re: RFR: 8221836: Avoid recalculating String.hash when zero

I agree that this is a good change, and you can use me as a reviewer. I disagree with Aleksey; it's a new technique but not complex to document or understand. The two state components are independent in their action; there is no race between their state changes. Meanwhile, there are two reasons

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 2019-04-08 17:10, Andrew Dinn wrote: On 08/04/2019 15:55, Aleksey Shipilev wrote: Why introduce this complexity to begin with? That's my argument. The complication of this code gives us almost nothing in return, why make the change? Without the compelling reason to do it, all this is jus

Re: RFR: TEST_BUG: 8222027 - java/util/logging/LogManager/TestLoggerNames.java generates intermittent ClassCastException

Thanks Steve, I just pushed it: http://hg.openjdk.java.net/jdk/jdk/rev/e5713cefcf41 best regards, -- daniel On 08/04/2019 15:13, Steve Groeger wrote: Hi Daniel, Thanks, have created a new webrev [1] Once it is merged I will then work on getting it back-ported to jdk11u. Thanks for all your

Re: RFR: 8221836: Avoid recalculating String.hash when zero

Hi Ivan, not sure that would be an optimization, since you'd trade a conditional write for an unconditional one. The computation itself for the empty string has trivial cost. /Claes On 2019-04-08 18:12, Ivan Gerasimov wrote: Hi Claes! Would it make sense to preset hashIsZero = true in the emp

Re: RFR: 8221836: Avoid recalculating String.hash when zero

Hi Claes! Would it make sense to preset hashIsZero = true in the empty string constructor? The current code avoids calculating the hashCode for an empty string, and the new code doesn't seem to do that because hashIsZero = false by default for each newly constructed copy of the empty string.

Re: RFR: 8221836: Avoid recalculating String.hash when zero

Right, this and possibly reducing latency when running with String deduplication enabled might be the more tangible benefits. Removing a cause for spurious performance degradations is nice, but mainly theoretical. There's likely a pre-existing negative interaction between string dedup and String a

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 08/04/2019 15:55, Aleksey Shipilev wrote: > Why introduce this complexity to begin with? That's my argument. The > complication of this code gives > us almost nothing in return, why make the change? Without the compelling > reason to do it, all this > is just a runaway "hold my beer" micro-opt

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 2:44 PM, Peter Levart wrote: > If you're afraid that a future maintainer of that code would not realize > that, then a simple comment > put into String.hashCode method and java_lang_String::set_hash C++ metohd > that would say something > like the following: > > // only a single field

Re: RFR: TEST_BUG: 8222027 - java/util/logging/LogManager/TestLoggerNames.java generates intermittent ClassCastException

Hi Daniel, Thanks, have created a new webrev [1] Once it is merged I will then work on getting it back-ported to jdk11u. Thanks for all your assistance in getting this done. [1] http://cr.openjdk.java.net/~sgroeger/8222027/webrev.04/ Thanks Steve Groeger IBM Runtime Technologies Hursley, Winche

Re: RFR: 8221397 Support implementation-defined Map Modes

On 08/04/2019 14:59, Alan Bateman wrote: > On 08/04/2019 10:52, Andrew Dinn wrote: >> : >> I created the following CSR >> >>    https://bugs.openjdk.java.net/browse/JDK-8222107 >> >> Apologies if I got anything wrong (first time I have done this :-). >> >> Do I need to solicit reviews or does this

Re: RFR: 8221397 Support implementation-defined Map Modes

On 08/04/2019 10:52, Andrew Dinn wrote: : I created the following CSR https://bugs.openjdk.java.net/browse/JDK-8222107 Apologies if I got anything wrong (first time I have done this :-). Do I need to solicit reviews or does this automatically get dispatched to the relevant component/sub-com

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

On 4/8/19 3:23 PM, Aleksey Shipilev wrote: > On 4/8/19 3:08 PM, David Holmes wrote: >> +1 > > Thanks, I am going to push this under triviality rule. jdk-submit cleared the change. Pushed. -Aleksey

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

On 4/8/19 3:08 PM, David Holmes wrote: > +1 Thanks, I am going to push this under triviality rule. > Strange that an old gcc complains but the newer ones that add more and more > warnings each time, let > this slip through :( Yeah, compilers :/ > Arguably all the exit(-1) in the main method sh

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

+1 Strange that an old gcc complains but the newer ones that add more and more warnings each time, let this slip through :( Arguably all the exit(-1) in the main method should just be return -1 instead - but that's a different matter. Thanks, David On 8/04/2019 10:08 pm, Alan Bateman wrote

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 1:40 PM, Aleksey Shipilev wrote: On 4/8/19 1:28 PM, Peter Levart wrote: The reasoning is very similar as with just one field. With one field (hash) the thread sees either the default value (0) or a non-zero value calculated either by this thread sometime before or by a concurrent

Re: RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

On 08/04/2019 12:54, Aleksey Shipilev wrote: Bug: https://bugs.openjdk.java.net/browse/JDK-8222111 Fix: diff -r 0d7fb7f07134 test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c --- a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c Mon Apr 08 06:56

RFR (XS) 8222111: exeCallerAccessTest.c fails to build: control reaches end of non-void function

Bug: https://bugs.openjdk.java.net/browse/JDK-8222111 Fix: diff -r 0d7fb7f07134 test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c --- a/test/jdk/java/lang/reflect/exeCallerAccessTest/exeCallerAccessTest.c Mon Apr 08 06:56:37 2019 +0100 +++ b/test/jdk/java/lang/reflect/

Re: RFR: TEST_BUG: 8222027 - java/util/logging/LogManager/TestLoggerNames.java generates intermittent ClassCastException

Hi Steve, On 08/04/2019 12:21, Steve Groeger wrote: Hi Daniel, Thanks for agreeing to sponsor this update. Have created a new webrev [1] according to the instructions you pointed me at. Would be grateful if you could re-review and then do whatever is needed to get this into the code. [1] _h

RFR: JDK-8221749: Error messages

Please review the jpackage fix for bug [1] at [2]. This is a fix for the JDK-8200758-branch branch of the open sandbox repository (jpackage). [1] http://cr.openjdk.java.net/~herrick/8221749/ [2] https://bugs.openjdk.java.net/browse/JDK-8221749/ /Andy

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 1:28 PM, Peter Levart wrote: > On 4/8/19 12:28 PM, Aleksey Shipilev wrote: >>> However, you also said in your opening criticism >>> >>> "I had hard time convincing myself that code is concurrency-safe" >>> >>> I think that is a more telling complaint. Can you elaborate on why you >>> fo

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 12:28 PM, Aleksey Shipilev wrote: However, you also said in your opening criticism "I had hard time convincing myself that code is concurrency-safe" I think that is a more telling complaint. Can you elaborate on why you found it hard to convince yourself of this? (I know what I

Re: RFR: TEST_BUG: 8222027 - java/util/logging/LogManager/TestLoggerNames.java generates intermittent ClassCastException

Hi Daniel, Thanks for agreeing to sponsor this update. Have created a new webrev [1] according to the instructions you pointed me at. Would be grateful if you could re-review and then do whatever is needed to get this into the code. [1] http://cr.openjdk.java.net/~sgroeger/8222027/webrev.03/

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 1:00 PM, Claes Redestad wrote: > On 2019-04-08 12:28, Aleksey Shipilev wrote: >> Because the whole thing in current code is "benign data race" on hash field. >> Pulling in another >> field into race needs careful consideration if it breaks the benignity. It >> apparently does not, but >

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 2019-04-08 12:28, Aleksey Shipilev wrote: Because the whole thing in current code is "benign data race" on hash field. Pulling in another field into race needs careful consideration if it breaks the benignity. It apparently does not, but the cognitive complexity involved in reading that code

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 2019-04-08 12:15, Andrew Dinn wrote: Claes, is there a reason why you named the argument to method hash_is_set 'string' when every other method uses the name 'java_string'? Is you 'j' key a tad sticky? I took the cue from set_hash which uses the 'string' naming, but yes, you're right to poin

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 12:15 PM, Andrew Dinn wrote: > On 08/04/2019 10:42, Claes Redestad wrote: >> On 2019-04-08 11:35, Aleksey Shipilev wrote: Sure, String::hashCode/hash_code locally becomes a bit more complex, but I view this as being a net improvement on the total amount of special handling

Re: RFR: 8221836: Avoid recalculating String.hash when zero

I think the most benefit in this patch is the emptyString.hashCode() speedup. By holding a boolean flag in the String object itself, there is one less de-reference to be made on fast-path in case of empty string. Which shows in microbenchmark and would show even more if code iterated many diffe

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 08/04/2019 10:42, Claes Redestad wrote: > On 2019-04-08 11:35, Aleksey Shipilev wrote: >>> Sure, String::hashCode/hash_code locally becomes a bit more complex, but >>> I view this as being a net improvement on the total amount of special >>> handling we need to do for Strings and their hash co

Re: RFR: 8221397 Support implementation-defined Map Modes

On 06/04/2019 19:05, Alan Bateman wrote: > On 05/04/2019 11:50, Andrew Dinn wrote: >> : >> Ah ok, got it now. I think I was confused by a missing 'not' in your >> original response. >> >> I have now removed the @throws for NPE from the MapMode constructor >> javadoc as well as letting the call to l

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 2019-04-08 11:35, Aleksey Shipilev wrote: Sure, String::hashCode/hash_code locally becomes a bit more complex, but I view this as being a net improvement on the total amount of special handling we need to do for Strings and their hash codes. I don't see it. The change *added* new handling

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 11:25 AM, Claes Redestad wrote: > On 2019-04-08 10:56, Aleksey Shipilev wrote: >> Regardless, I think this change does not carry its weight. Introducing >> special paths for handling >> something as obscure as zero hash code, which then raises questions about >> correctness (I had hard

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 2019-04-08 10:56, Aleksey Shipilev wrote: Regardless, I think this change does not carry its weight. Introducing special paths for handling something as obscure as zero hash code, which then raises questions about correctness (I had hard time convincing myself that code is concurrency-safe

Re: RFR: 8221836: Avoid recalculating String.hash when zero

On 4/8/19 10:41 AM, Claes Redestad wrote: > by adding a bit to String that is true iff String.hash has been calculated as > being 0, we can get > rid of the corner case where such hash > codes are recalculated on every call. > > Peter Levart came up with a elegant scheme for ensuring that we can

RFR: 8221836: Avoid recalculating String.hash when zero

Hi, by adding a bit to String that is true iff String.hash has been calculated as being 0, we can get rid of the corner case where such hash codes are recalculated on every call. Peter Levart came up with a elegant scheme for ensuring that we can keep using non-volatile stores without explicit

Re: RFR: [8u] 8205432: Replace the placeholder Japanese era name

On 4/8/19 5:40 AM, Andrew John Hughes wrote: > Bug: https://bugs.openjdk.java.net/browse/JDK-8205432 > Webrev: https://cr.openjdk.java.net/~andrew/openjdk8/8205432/webrev.01/ OK, thanks. -- Andrew Haley Java Platform Lead Engineer Red Hat UK Ltd. EAC8 43EB D3EF DB98 CC77

RE: RFR: [8u] 8205432: Replace the placeholder Japanese era name

Hi Andrew, Thanks for working on this. Please find below few minor comments:- 1>. For "src/share/classes/java/util/JapaneseImperialCalendar.java" and "src/share/classes/sun/util/calendar/Era.java" please changes the date time format to be consistent with existing defined eras:- * 4