Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use JDK UUID#fromString for Java >= 15 #15
base: main
Are you sure you want to change the base?
Use JDK UUID#fromString for Java >= 15 #15
Changes from 3 commits
6517514
513e598
e549a52
80e604f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to self: we should probably pull this out into a property so we don't have to change the version in three different places. Let's definitely leave that for a separate effort, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can do that, no problem. Will extract when I get the chance to open it again on IntelliJ.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'm missing something: why do we need this if we also have
parseUUID(CharSequence)
, which also checks if theCharSequence
is aString
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because JDK's method only works with
String
.So we can:
parseUUID(String)
and possibly break compatibility;CharSequence#toString()
but that will lead to memory copies in any non-String
objects that implementCharSequence
;String
(so it can be passed to JDK's in case we're running on >= 15) and keep theCharSequence
there too and shared the common code path between the two (for JDKs prior to 15, butCharSequence
would always go there since we don't want to copy memory);CharSequence
is aString
so we can pass it to JDK's, but we keep using our implementation in case it's not (since it performs closely anyway).instanceof
has basically no cost on modern JVMs anyway, so I chose to go with 4.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah—I think I see where you're going here. If I can reword it to make sure we're on the same page, I think you're saying that you want to add a
String
-specific variant because that will lower the cost of passing through toUUID#fromString
under Java 15 and newer.If I'm understanding that right, I'd gently push back that I think it's okay to continue using fast-uuid's parser even under Java 15. My rationale is that if somebody is working with non-
String
CharSequences
, they probably have some specific thing they're doing that means they want to avoidString
conversion in the first place, and using the sliiiiightly slowerFastUuid#parseUUID(CharSequence)
will still be faster for those users than converting to aString
, then using the slightly fasterUUID#fromString(String)
.As you say, using
instanceof
inFastUuid#parseUUID(CharSequence)
is a negligible cost, so most users would still get the benefits of using the built-in parser under Java 15 and newer, and it's only the oddballs (like me!) who have aCharSequence
-specific use case that would be staying in the fast-uuid space.Does that make sense? Am I missing your point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more of a compile-time optimization and maybe being able to deal with JVMs where
instanceof
isn't super-duper cheap.If you upgrade from existing code to this, your compiler will link your calls that use String-typed references to the new method. So
If
uuidChars
happens to actually be a String, we can still leverage the speed boost by checking that and calling JDK'sUUID#fromString
.Providing the overload with String allows us to skip that step without the user changing any code because upon compilation, they'd link to the new method if the reference is String-typed.
A CharSequence reference to a CharSequence non-String object will still just use the existing parser just fine. There is no conversion involved.
I see this as a win-win change. The only downside is that
FastUUID.parseUUID(null)
would not compile anymore. You'd have to cast the null to either CharSequence or String.For someone using fast-uuid indirectly, the type-check to String inside the CharSequence overload still allows for the performance boost in Java 15 even without recompilation.