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
[9.4.x] REPL read opt perf #7777
Conversation
reverted to get baseline with 9.4.x as well. |
run performance tests please |
f111d91
to
5e76619
Compare
run performance tests please |
run performance tests please |
Performance tests didn't finish successfully. @diegolovison, can you review it? Additional info: |
run performance tests please |
@wburns please take a look: perfrepo.mw.lab.eng.bos.redhat.com/reports/tableComparisonReport/18128 |
run performance tests please |
Performance tests didn't finish successfully. @diegolovison, can you review it? Additional info: |
Odd that is shows no improvement. I have a JMH test that I have been running that shows almost a 25% increase in throughput for get with the if branch commented out. I will have to look closer in a bit. |
run performance tests please |
Performance seems pretty good in the latest run. For some reason mean response time is still higher, however the actual throughput is higher. I will try another to see what it says. |
run performance tests please |
* Make sure to not calculate segment if segmentation is disabled * Add in interface to prevent additional if/else blocks
a44ae6e
to
cf6c34b
Compare
Rebased and should be ready if it is deemed okay. |
run performance tests please |
Merged |
} | ||
return (CacheEntry<K, V>) ice; | ||
}).toCompletableFuture(); | ||
if (CompletionStages.join(stage)) { |
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.
Shouldn't we check if (isCompletedSuccessfully(stage))
first?
No need to. This is a blocking method call and there is no lambda to worry
about. I am guessing the diff probably tricked you into thinking it was a
non blocking method 😁
…On Fri, Feb 7, 2020, 12:04 PM Dan Berindei ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In core/src/main/java/org/infinispan/factories/InternalCacheFactory.java
<#7777 (comment)>
:
> CompletionStage<Boolean> stage = expirationManager.handlePossibleExpiration(ice, segment, false);
- if (CompletionStages.isCompletedSuccessfully(stage)) {
- if (CompletionStages.join(stage)) {
- return CompletableFutures.completedNull();
- }
- return CompletableFuture.completedFuture(ice);
- } else {
- return stage.thenApply(expired -> {
- if (expired == Boolean.TRUE) {
- return null;
- }
- return (CacheEntry<K, V>) ice;
- }).toCompletableFuture();
+ if (CompletionStages.join(stage)) {
Shouldn't we check if (isCompletedSuccessfully(stage)) first?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7777?email_source=notifications&email_token=AAE6H33HUL74KP3O4PGDTZDRBWPERA5CNFSM4KKOEBC2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCUXAC2I#pullrequestreview-355336553>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAE6H33C54HW76XUXBZSEB3RBWPERANCNFSM4KKOEBCQ>
.
|
It did :) |
No description provided.