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

On Nov 24, 2015, at 6:17 AM, Aleksey Shipilev wrote: > > Oh no, I don't. Pre-integration JPRT runs exposed an opaque dependency > on Integer.sizeTable field from C2 OptimizeStringConcat. I added the > field declaration back: > http://cr.openjdk.java.net/~shade/8136500/webrev.07/ >

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

Hi Aleksey, Am 24.11.2015 um 23:44 schrieb Aleksey Shipilev: On 11/24/2015 07:50 PM, Ulf wrote: Am 24.11.2015 um 12:07 schrieb Aleksey Shipilev: Thanks for reviews, Ivan, John, Sherman and Paul! I am pushing this change now. I know, I'm late, but have you ever tried? : No, but you can try i

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

On 11/24/2015 07:50 PM, Ulf wrote: > Am 24.11.2015 um 12:07 schrieb Aleksey Shipilev: >> Thanks for reviews, Ivan, John, Sherman and Paul! >> >> I am pushing this change now. > > I know, I'm late, but have you ever tried? : No, but you can try it yourself. OpenJDK workspace has tests covering tha

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

On 11/24/2015 09:08 PM, Ulf wrote: Hi, Am 24.11.2015 um 18:27 schrieb Peter Levart: On 11/24/2015 06:22 PM, Peter Levart wrote: .. for addressing DigitOnes and DigitTens it is ok and might be faster, as you say. If DigitOnes and DigitTens are extended for 28 ignored zero slots, it may s

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

On 11/24/2015 06:22 PM, Peter Levart wrote: On 11/24/2015 06:11 PM, Peter Levart wrote: byte r = (byte)((q * 100 - i) & 0x7F); // byte allows the compiler to use byte wide addressing opcodes // which have smaller footprint and are potentially faster

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

On 11/24/2015 06:11 PM, Peter Levart wrote: byte r = (byte)((q * 100 - i) & 0x7F); // byte allows the compiler to use byte wide addressing opcodes // which have smaller footprint and are potentially faster // .. & 0x7F may additionally save nega

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

On 11/24/2015 05:50 PM, Ulf wrote: Hi, Am 24.11.2015 um 12:07 schrieb Aleksey Shipilev: Thanks for reviews, Ivan, John, Sherman and Paul! I am pushing this change now. I know, I'm late, but have you ever tried? : static void getChars(int i, int index, byte[] buf) { // int q, r

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

Hi, Am 24.11.2015 um 12:07 schrieb Aleksey Shipilev: Thanks for reviews, Ivan, John, Sherman and Paul! I am pushing this change now. I know, I'm late, but have you ever tried? : static void getChars(int i, int index, byte[] buf) { // int q, r;// superfluous // int

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

On 11/24/2015 05:20 PM, Paul Sandoz wrote: >> On 24 Nov 2015, at 15:17, Aleksey Shipilev >> wrote: >> Oh no, I don't. Pre-integration JPRT runs exposed an opaque dependency >> on Integer.sizeTable field from C2 OptimizeStringConcat. I added the >> field declaration back: >> http://cr.openjdk.java

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

Hi, Am 24.11.2015 um 18:27 schrieb Peter Levart: On 11/24/2015 06:22 PM, Peter Levart wrote: .. for addressing DigitOnes and DigitTens it is ok and might be faster, as you say. If DigitOnes and DigitTens are extended for 28 ignored zero slots, it may save the upper bound check too. Another

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

Am 24.11.2015 um 17:50 schrieb Ulf: ... and better use char/short[] to save footprint and the compiler calculating the table offset with charPos<<1. Note additionally: In this case the raw table length will be = 200, so compiler can use cheap byte-wide addressing mode opcodes. -Ulf

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

On 11/24/2015 02:07 PM, Aleksey Shipilev wrote: > On 11/24/2015 03:35 AM, John Rose wrote: >> On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev >> mailto:aleksey.shipi...@oracle.com>> wrote: >>> >>> Okay, here it is (only tests changed): >>> http://cr.openjdk.java.net/~shade/8136500/webrev.06/ >>>

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

> On 24 Nov 2015, at 15:17, Aleksey Shipilev > wrote: > > On 11/24/2015 02:07 PM, Aleksey Shipilev wrote: >> On 11/24/2015 03:35 AM, John Rose wrote: >>> On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev >>> mailto:aleksey.shipi...@oracle.com>> wrote: Okay, here it is (only tests changed

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

On 11/24/2015 03:35 AM, John Rose wrote: > On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev > mailto:aleksey.shipi...@oracle.com>> wrote: >> >> Okay, here it is (only tests changed): >> http://cr.openjdk.java.net/~shade/8136500/webrev.06/ >> >

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

On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev wrote: > > Okay, here it is (only tests changed): > http://cr.openjdk.java.net/~shade/8136500/webrev.06/ > Thanks; that's a solid test. Cleanups are good too. You can count me as a reviewer.

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

Ah, the good 'ole switch statement earning more optimization jiras :). sent from my phone On Nov 23, 2015 7:36 PM, "John Rose" wrote: > On Nov 23, 2015, at 1:42 PM, Aleksey Shipilev > wrote: > > > > Okay, here it is (only tests changed): > > http://cr.openjdk.java.net/~shade/8136500/webrev.06/

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

On 11/23/2015 08:58 PM, John Rose wrote: > On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov > wrote: >> >> Though, it may be better to get yet another pair of eyes. >> >> One minor nit: In the tests, in the summary, it is written, "Test >> Integer.toString method*s*",

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

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

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

On 11/23/2015 07:08 AM, Aleksey Shipilev wrote: On 11/23/2015 04:34 PM, Ivan Gerasimov wrote: With this fixed patch: http://cr.openjdk.java.net/~igerasim/8136500/8136500-addition-1.patch all tests from test/lang pass. Would you give it another chance? Okay, but this is better be the last non-c

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

On 11/23/2015 09:05 PM, Aleksey Shipilev wrote: > On 11/23/2015 08:58 PM, John Rose wrote: >> On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov > > wrote: >>> >>> Though, it may be better to get yet another pair of eyes. >>> >>> One minor nit: In the tests, in the summar

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

On 11/23/2015 08:58 PM, John Rose wrote: > On Nov 23, 2015, at 8:23 AM, Ivan Gerasimov > wrote: >> >> Though, it may be better to get yet another pair of eyes. >> >> One minor nit: In the tests, in the summary, it is written, "Test >> Integer.toString method*s*",

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

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

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

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

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

Great! Now I like it the best. Though, it may be better to get yet another pair of eyes. One minor nit: In the tests, in the summary, it is written, "Test Integer.toString method*s*", but only one of the overloads is tested. Sincerely yours, Ivan On 23.11.2015 18:08, Aleksey Shipilev wrote:

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

What sanity checks you had in mind? Because java/lang/Integer/IntegerDecode.java fails, and it fails *in compiler*. Oh, how embarrassing! Overlooked a sign :( I had tested it only with new lang/Integer/ToString.java, in a hope it gives enough coverage for sanity, but I was wrong. I tried to

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

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

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

Hi Aleksey! I think the code looks much better now! A couple of concerns, though: 1) Wouldn't it be clearer to have "(byte)('0' + q);" instead of "(byte)(48 + q); // 48 is ASCII '0'"? I see that '0' constant is used in arithmetic in some other places. 2) What still annoys me is the necessity

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

On 11/22/2015 01:55 AM, Ivan Gerasimov wrote: >>> Second, I think, the code of stringSize() might be better inlined, if it >>> was used only once. >> No-no! That's a helpful little fella. Not-yet-integrated Indify String >> Concat uses Integer/Long.stringSize. > > I didn't really propose to manual

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

Hi Aleksey, > > It might be reasonable to reorganize the code a bit: > > > > int size = 0, i1 = i; > > if (i < 0) { > > if (i == Integer.MIN_VALUE) > > return "-2147483648"; > > size = 1; > > i1 = -i; > > } > > size += stringSize(i1); > > > > Fi

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

Hi Ivan! Update: http://cr.openjdk.java.net/~shade/8136500/webrev.02/ Performance data updated (shaves off 0.5 ns at most): http://cr.openjdk.java.net/~shade/8136500/notes.txt On 11/21/2015 11:38 PM, Ivan Gerasimov wrote: > Just by a coincidence, I was experimenting with the same piece of code

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

Thank you Aleksey! Second, I think, the code of stringSize() might be better inlined, if it was used only once. No-no! That's a helpful little fella. Not-yet-integrated Indify String Concat uses Integer/Long.stringSize. I didn't really propose to manually inline the stringSize() into the cal

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

Hi Aleksey! Just by a coincidence, I was experimenting with the same piece of code :) I haven't come up with the complete solution so far, but here are a few considerations, which you may find useful. 1) When benchmarking different implementation of Integer.stringSize() the fastest (on my ma

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

Hi, We have discovered this both in Compact Strings and Indy String Concat work, but it deserves to be treated separately. Integer/Long getChars code seems to be very old (Josh Bloch estimated circa 1994) and written under the assumption no compiler is here to help us. Fast-forward 20 years, and