-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Replace String.format(…, args)
with "…".formatted(args)
#2752
Closed
Closed
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
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.
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 do not think this is a good change.
Without a supplier, we are needlessly constructing a string.
This assertion is almost always true, so it should be optimised for a happy path.
The same applies to other similar changes.
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.
Thank you for the feedback. I agree.
I wrote a simple JMH Benchmark today comparing
String.formatted(..)
with and without the use of aSupplier
to the assertion, such asisTrue(..)
above. It turns out that, without theSupplier
the invocation performance (throughput & average time) is much worse over a large number of iterations, even despite the added Object allocations forSuppliers
. I did not look at the Heap memory map, but I suspect it will create a bit more garbage than necessary, at least until JIT compiler optimizations kick in.However, I will point out that, I was careful to only remove the
Suppliers
in non-critical code paths, such as in this case... "Configuration".Configuration is only really ever in invoked on Spring container startup, once, when the beans are constructed, configured and initialized.
In other cases, I believe I only removed the use of a
Supplier
in tests.I will re-review the changes to make sure and introduce any necessary
Supplier
that may have been mistakenly removed.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 forget to mention, keep in mind that the
Supplier
Lambda in this case is NOT a non-capturing Lambda since it uses arguments from the enclosing method. Therefore, it, too, results in Object construction every time the method is called (thoughRedisSentinelConfiguration.setDatabase(:int)
is only likely to be called once in this case).Still, let's be clear here what we are talking about in terms of "optimizations".
You must always consider resource utilization when considering performance as often they are related. You must also measure. And finally, you must take into consideration the code path. Sacrificing readability, correctness or other factors for the sake of performance is usually not the best decision.
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.
In reviewing my changes for this PR, along with writing a JMH Benchmark, I definitely think the use of a
Supplier
in assertions is necessary in SD Redis command classes, where the codepath will be hotter at runtime than it will be in configuration classes, for instance, such as here, as well as in similar places.In other cases, such as the KeyspaceIdentifier and BinaryKeyspaceIdentifier, I introduced the use of a
Supplier
inside the assertion where aSupplier
was not used before.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 have updated this PR to (re)introduce the use of the
Supplier
inside assertions. Only very few were inside critical codepaths, like command classes (specifically Jedis and Lettuce ZSet commands). The rest were in configuration classes (non-critical codepaths).It should be noted, that most of the String formatting occurs inside Exception construction (e.g.
InvalidDataAccessApiUsageException
) and Logger statements. There is no option for using aSupplier
in most of these cases, not without extra guards; for example:This may make for some nice optimizations down the road (and in certain cases, I noticed I have introduced log helper methods, just like described above), but that is not a concern in this PR.
In conclusion, I still feel the appearance of using
String.formatted(..)
is much nicer thanSpring.format(..)
, as it calls attention to the Exception/Log/Whatever message immediately.