On Feb 1 2013, at 10:13 , Steven Schlansker wrote: > > On Jan 30, 2013, at 11:42 AM, Mike Duigou <mike.dui...@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 >>> >> >
