On Feb 1 2013, at 10:13 , Steven Schlansker wrote:

On Jan 30, 2013, at 11:42 AM, Mike Duigou <mike.duigou@oracle.com> wrote:

...

Good work Steven!

Thanks for the review :)

...

Some initial comments;

- The changes to Long should be in a separate issue. It's generally encouraged for changesets to focus on only one change. Sorry, yes, it's additional overhead.

That's unfortunate. I don't have the ability / know how to file a second, separate issue, and I've already spent a lot more time trying to get this small change through than I'd expected on overhead. Will it be preferable to have someone open a second issue, make two web revs, and code review both, or should I just abandon the attempt at code-sharing between UUID and Long? In the original patch that Aleksey reviewed I had not attempted this; I did it on his suggestion.

I have created another issue 8007398 for the changes to Long. We can even test and push the two issues at the same time. Separating them into two changesets makes the intent easier to follow for future maintainers. We can use the same webrev. There's no need to create another.

...

- I would like to see if performed of toString() can be improved further by using String(char[] value, boolean share) constructor via a sun.miscSharedSecret.JavaLangAccesss method to construct the string directly from the character array. You could test to see if this has positive benefit by temporarily using a static char array.

I will incorporate this into my next revision

...

- public static String toString(long msb, long lsb) should be private. There's no compelling reason to add this to the API.

I can remove this. I added it on the theory that if you wish to ship UUIDs around as two longs, it may be useful to omit UUID object instantiation just to format as a string. Maybe this is too far out there to make another public method acceptable.

...

- Have you run this code against any of the existing regression tests?

Yes, I ran the jtreg UUID and Long tests, all pass. I ran the Apache Harmony UUID test cases against the pre-integrated version of the code. (There should only have been minor modifications since then, variable renamings, whatnot…)

OK, once we have a final webrev then I will run final tests and push this! Mike

...

Mike

On Jan 28 2013, at 19:23 , Steven Schlansker wrote:

...

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8006627

I have created a patch that dramatically improves UUID to/from string performance. Please find below a webrev with my proposed changes.

Thanks in advance for any feedback on the contents. I do not believe I have a committer lined up yet. My company has a signed OCA on file, "Ness Computing".

http://dl.dropbox.com/u/1422321/uuid_webrev/index.html http://dl.dropbox.com/u/1422321/uuid_webrev.zip

This looks fairly much complete to me. I will run it through the some testing here and then push your patch. Great work and thank you! Mike On Feb 3 2013, at 21:26 , Steven Schlansker wrote:

On Feb 1, 2013, at 11:42 AM, Mike Duigou <mike.duigou@oracle.com> wrote:

...

I have created another issue 8007398 for the changes to Long. We can even test and push the two issues at the same time. Separating them into two changesets makes the intent easier to follow for future maintainers.

We can use the same webrev. There's no need to create another.

...

...

- I would like to see if performed of toString() can be improved further by using String(char[] value, boolean share) constructor via a sun.miscSharedSecret.JavaLangAccesss method to construct the string directly from the character array. You could test to see if this has positive benefit by temporarily using a static char array.

I will incorporate this into my next revision

...

- public static String toString(long msb, long lsb) should be private. There's no compelling reason to add this to the API.

...

- Have you run this code against any of the existing regression tests?

Yes, I ran the jtreg UUID and Long tests, all pass. I ran the Apache Harmony UUID test cases against the pre-integrated version of the code. (There should only have been minor modifications since then, variable renamings, whatnot…)

OK, once we have a final webrev then I will run final tests and push this!

Hi,

Here are the updates to the webrev. I hope the changes are in line with what you'd had in mind: http://dl.dropbox.com/u/1422321/uuid_webrev/index.html http://dl.dropbox.com/u/1422321/uuid_webrev.zip

Please let me know if there are any further modifications I should make.

Thanks! Steven

On Feb 11, 2013, at 9:04 PM, Mike Duigou <mike.duigou@oracle.com> wrote:

I have been following up on this issue. I am going to have to adapt the code a bit because there have been some changes in JDK8.

I won't forget this issue though. It is possible I may not have time to backport it to Java 7.

Please let me know if I can be of any help in a way that doesn't actually cost more work than I end up contributing :-)

Mile

On Feb 3 2013, at 21:26 , Steven Schlansker wrote:

...

On Feb 1, 2013, at 11:42 AM, Mike Duigou <mike.duigou@oracle.com> wrote:

...

I have created another issue 8007398 for the changes to Long. We can even test and push the two issues at the same time. Separating them into two changesets makes the intent easier to follow for future maintainers.

We can use the same webrev. There's no need to create another.

...

...

- I would like to see if performed of toString() can be improved further by using String(char[] value, boolean share) constructor via a sun.miscSharedSecret.JavaLangAccesss method to construct the string directly from the character array. You could test to see if this has positive benefit by temporarily using a static char array.

I will incorporate this into my next revision

...

- public static String toString(long msb, long lsb) should be private. There's no compelling reason to add this to the API.

...

- Have you run this code against any of the existing regression tests?

Yes, I ran the jtreg UUID and Long tests, all pass. I ran the Apache Harmony UUID test cases against the pre-integrated version of the code. (There should only have been minor modifications since then, variable renamings, whatnot…)

OK, once we have a final webrev then I will run final tests and push this!

Hi,

Here are the updates to the webrev. I hope the changes are in line with what you'd had in mind: http://dl.dropbox.com/u/1422321/uuid_webrev/index.html http://dl.dropbox.com/u/1422321/uuid_webrev.zip

Please let me know if there are any further modifications I should make.

Thanks! Steven

Am 13.02.2013 00:30, schrieb Mike Duigou:

Hi Steven;

I have updated the patch for Java 8. There's somewhat less code sharing and a bit of refactoring than your last version but the performance should be about the same or a little better.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/0/webrev/

Couldn't you use String(buf, true) for all to(Unsigned)String(...) methods ? Instead of calculating the mask each time, you could use: 309 private static String toUnsignedString(int i, int shift, int mask) { Additionally in e.g. Integer.toString(int i, int radix) you could provide the fast version with: 134 /* Use the faster version */ 135 switch (radix) { 136 case 2: return toString(i, 1, 0x1); 136 case 4: return toString(i, 2, 0x3); 136 case 8: return toString(i, 3, 0x7); 136 case 10: return toString(i); 136 case 16: return toString(i, 4, 0xF); 136 case 32: return toString(i, 5, 0x1F); 137 } -Ulf

Am 13.02.2013 02:34, schrieb Mike Duigou:

Thank you for the comments Ulf.

On Feb 12 2013, at 17:24 , Ulf Zibis wrote:

...

Am 13.02.2013 00:30, schrieb Mike Duigou:

...

Hi Steven;

I have updated the patch for Java 8. There's somewhat less code sharing and a bit of refactoring than your last version but the performance should be about the same or a little better.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/0/webrev/ Couldn't you use String(buf, true) for all to(Unsigned)String(...) methods ? Possibly. I didn't go looking too far with looking for additional improvements.

...

Instead of calculating the mask each time, you could use: 309 private static String toUnsignedString(int i, int shift, int mask) { I think that would actually be inefficient. I haven't looked at the JITed code but the mask calculation is pretty cheap relative to parameter passing.

I believe, JIT will inline the code, so there would be no parameter passing. Additionally the calculation of char count could be faster, if you would have short cuts like: numberOfLeading2Zeros(i) numberOfLeading4Zeros(i) numberOfLeading8Zeros(i) ... So the optimum would be with separate toString methods: String toBase2String(int i); String toBase4String(int i); String toBase8String(int i); ... In any case I would save 2 lines: 311 int mag = Integer.SIZE - Long.numberOfLeadingZeros(i); 312 int charCount = Math.max(((mag + (shift - 1)) / shift), 1); 313 char[] buf = new char[charCount]; 316 int mask = (1 << shift) - 1; -Ulf

Am 13.02.2013 23:45, schrieb Mike Duigou:

I have updated the patch with some of Ulf's feedback and corrected one cut-and-paste error that I made.

The updated webrev is at:

http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/

Fine, but I still see the typo in j.l.System. -Ulf

Finally some follow up on this! On Feb 13 2013, at 23:43 , Martin Buchholz wrote:

I like the optimizations in this change, especially the avoidance of copies. Was there some good reason the jdk never before used JavaLangAccess to avoid String creation copies?

Not that I am aware of. I did work on String and thereafter the idea occurred to me. Copying...we hates it.

I am tempted to micro-optimize this some more, e.g. specialize the hex printing code. I might get rid of the digits table and compute ASCII characters simply:

((i < 10) ? '0' : ('a' - 10)) + i

I wanted to play with JMH so used this as an excuse. It turns out that using a calculation is a performance loser: Benchmark Thr Cnt Sec Mean Mean error Var Units o.o.j.s.g.t.DigitsTableVsCalc.digitsCalc 1 8 1 8463376.310 2516.169 4136965.190 ops/sec o.o.j.s.g.t.DigitsTableVsCalc.digitsTable 1 8 1 11838194.938 4936.538 15923812.479 ops/sec The source of the benchmarks: @OutputTimeUnit(TimeUnit.SECONDS) @GenerateMicroBenchmark(BenchmarkType.OpsPerTimeUnit) public void digitsTable() throws InterruptedException { int end = testData.length; for (int each = 0; each < end; each++) { int i = testData[each]; output[each] = digits[i]; } } @OutputTimeUnit(TimeUnit.SECONDS) @GenerateMicroBenchmark(BenchmarkType.OpsPerTimeUnit) public void digitsCalc() throws InterruptedException { int end = testData.length; for (int each = 0; each < end; each++) { int i = testData[each]; int base = i < 10 ? 48 : 87; output[each] = (char) (base + i); } } Where testData is a 100 element static int array of random digit values (0-35).

Why not formatUnsignedInt?

Now added and it seems to help.

Why not make the new format methods public?

They seemed too specialized to me, particularly with the constraints on radix.

Is there a better name than createStringSharedChars? We hope those chars will *not* be shared! How about newStringUnsafe(char[] chars)

The spec for + int formatUnsignedLong(long val, int shift, char[] buf, int offset, int len); should make it clearer whose responsibility getting the size right is. It looks like the caller has to ensure there will be enough space, so perhaps the caller should just pass one offset instead of offset plus len.

Seems reasonable.

+ * @return the lowest character location used stray space

Corrected.

On Wed, Feb 13, 2013 at 2:45 PM, Mike Duigou <mike.duigou@oracle.com> wrote: I have updated the patch with some of Ulf's feedback and corrected one cut-and-paste error that I made.

The updated webrev is at:

http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/

Mike

On Feb 12 2013, at 18:25 , Ulf Zibis wrote:

...

Am 13.02.2013 02:34, schrieb Mike Duigou:

...

Thank you for the comments Ulf.

On Feb 12 2013, at 17:24 , Ulf Zibis wrote:

...

Am 13.02.2013 00:30, schrieb Mike Duigou:

...

Hi Steven;

I have updated the patch for Java 8. There's somewhat less code sharing and a bit of refactoring than your last version but the performance should be about the same or a little better.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/0/webrev/ Couldn't you use String(buf, true) for all to(Unsigned)String(...) methods ? Possibly. I didn't go looking too far with looking for additional improvements.

...

Instead of calculating the mask each time, you could use: 309 private static String toUnsignedString(int i, int shift, int mask) { I think that would actually be inefficient. I haven't looked at the JITed code but the mask calculation is pretty cheap relative to parameter passing.

I believe, JIT will inline the code, so there would be no parameter passing.

Additionally the calculation of char count could be faster, if you would have short cuts like: numberOfLeading2Zeros(i) numberOfLeading4Zeros(i) numberOfLeading8Zeros(i) ... So the optimum would be with separate toString methods: String toBase2String(int i); String toBase4String(int i); String toBase8String(int i); ...

In any case I would save 2 lines:

311 int mag = Integer.SIZE - Long.numberOfLeadingZeros(i); 312 int charCount = Math.max(((mag + (shift - 1)) / shift), 1); 313 char[] buf = new char[charCount]; 316 int mask = (1 << shift) - 1;

-Ulf

Hi Steven; I haven't forgotten and it's still in my queue for Java 8. I haven't had time to respond to Martin Buchholz's feedback. http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-February/014528.ht... I suspect he is right about computing digits vs the table being a better choice but I would want to test that first with a proper microbenchmark (and then fix a bunch of other places that use the same table). If you have time to follow up on any of this things will go quicker. :-) Mike On Mar 20 2013, at 15:39 , Steven Schlansker wrote:

On Feb 13, 2013, at 2:45 PM, Mike Duigou <mike.duigou@oracle.com> wrote:

...

I have updated the patch with some of Ulf's feedback and corrected one cut-and-paste error that I made.

The updated webrev is at:

http://cr.openjdk.java.net/~mduigou/JDK-8006627/2/webrev/

One last ping to make sure this doesn't slip through the cracks, as I haven't seen the commit go through the public JDK8 repo… or maybe I missed it.

Thanks again for looking at this :-)

...

Mike

On Feb 12 2013, at 18:25 , Ulf Zibis wrote:

...

Am 13.02.2013 02:34, schrieb Mike Duigou:

...

Thank you for the comments Ulf.

On Feb 12 2013, at 17:24 , Ulf Zibis wrote:

...

Am 13.02.2013 00:30, schrieb Mike Duigou:

...

Hi Steven;

I have updated the patch for Java 8. There's somewhat less code sharing and a bit of refactoring than your last version but the performance should be about the same or a little better.

http://cr.openjdk.java.net/~mduigou/JDK-8007398/0/webrev/ Couldn't you use String(buf, true) for all to(Unsigned)String(...) methods ? Possibly. I didn't go looking too far with looking for additional improvements.

...

Instead of calculating the mask each time, you could use: 309 private static String toUnsignedString(int i, int shift, int mask) { I think that would actually be inefficient. I haven't looked at the JITed code but the mask calculation is pretty cheap relative to parameter passing.

I believe, JIT will inline the code, so there would be no parameter passing.

Additionally the calculation of char count could be faster, if you would have short cuts like: numberOfLeading2Zeros(i) numberOfLeading4Zeros(i) numberOfLeading8Zeros(i) ... So the optimum would be with separate toString methods: String toBase2String(int i); String toBase4String(int i); String toBase8String(int i); ...

In any case I would save 2 lines:

311 int mag = Integer.SIZE - Long.numberOfLeadingZeros(i); 312 int charCount = Math.max(((mag + (shift - 1)) / shift), 1); 313 char[] buf = new char[charCount]; 316 int mask = (1 << shift) - 1;

-Ulf