You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by GitBox <gi...@apache.org> on 2022/03/14 12:14:22 UTC
[GitHub] [pulsar] lhotari opened a new pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
lhotari opened a new pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679
### Motivation
The use of Guava's ComparisonChain isn't consistent with the style of performance optimized code used in Pulsar in hotspots. There's usually a strict goal to minimize object allocations.
- remove ComparisonChain from some hotspots since ComparisonChain creates object instances when it is used
### Modifications
- replace ComparisonChain usage with plain Java code
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
lhotari commented on pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679#issuecomment-1067110999
> > remove ComparisonChain from some hotspots since ComparisonChain creates object instances when it is used
>
> @lhotari Change looks good and code looks simpler, though the `ComparisonChain` doesn't actually involve any allocation :)
@merlimat Thanks. I missed that detail. Pretty well designed in that sense.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679#discussion_r826530600
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -149,7 +148,15 @@ public long getEntryId() {
@Override
public int compareTo(EntryImpl other) {
- return ComparisonChain.start().compare(ledgerId, other.ledgerId).compare(entryId, other.entryId).result();
+ if (this.ledgerId != other.ledgerId) {
+ return this.ledgerId < other.ledgerId ? -1 : 1;
Review comment:
Yep, one more equals check. But i believe it's most likely will be optimized by JIT :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] Jason918 commented on a change in pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
Jason918 commented on a change in pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679#discussion_r825930957
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -149,7 +148,15 @@ public long getEntryId() {
@Override
public int compareTo(EntryImpl other) {
- return ComparisonChain.start().compare(ledgerId, other.ledgerId).compare(entryId, other.entryId).result();
+ if (this.ledgerId != other.ledgerId) {
+ return this.ledgerId < other.ledgerId ? -1 : 1;
Review comment:
`java.lang.Long#compare(this.ledgerId, other.ledgerId)` is more readable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] lhotari commented on a change in pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
lhotari commented on a change in pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679#discussion_r825958570
##########
File path: managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/EntryImpl.java
##########
@@ -149,7 +148,15 @@ public long getEntryId() {
@Override
public int compareTo(EntryImpl other) {
- return ComparisonChain.start().compare(ledgerId, other.ledgerId).compare(entryId, other.entryId).result();
+ if (this.ledgerId != other.ledgerId) {
+ return this.ledgerId < other.ledgerId ? -1 : 1;
Review comment:
It wouldn't be optimal to this case.
Long.compare has one additional condition:
```
public static int compare(long x, long y) {
return (x < y) ? -1 : ((x == y) ? 0 : 1);
}
```
That's not optimal for the style used in this change.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat commented on pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
merlimat commented on pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679#issuecomment-1067051170
> remove ComparisonChain from some hotspots since ComparisonChain creates object instances when it is used
@lhotari Change looks good and code looks simpler, though the `ComparisonChain` doesn't actually involve any allocation :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [pulsar] merlimat merged pull request #14679: [Performance] Optimize comparisons by replacing ComparisonChain in hotspots
Posted by GitBox <gi...@apache.org>.
merlimat merged pull request #14679:
URL: https://github.com/apache/pulsar/pull/14679
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org